Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OshiSysMonitor: Add ability to skip emitting metrics #16972

Merged
merged 4 commits into from
Sep 12, 2024
Merged

OshiSysMonitor: Add ability to skip emitting metrics #16972

merged 4 commits into from
Sep 12, 2024

Conversation

adithyachakilam
Copy link
Contributor

Description

While running druid in containerized (or any other customized) environments, not all of the metrics emitted by OshiSysMonitor are applicable. This PR adds ability to skip a category of metrics from this monitor so as to reduce computation in calculating/emitting/storing such metrics through specifying druid.monitoring.oshisys.skipEmitting config.

Release note

Ability to skip emitting metrics by category in OshiSysMonitor.

Key changed/added classes in this PR
  • OshiSysMonitor
  • MetricsModule

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@arunramani
Copy link
Contributor

The changes looks reasonable. The property name can be simpler and generic.

kfaraz
kfaraz reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some suggestions.

docs/operations/metrics.md Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/server/metrics/OshiSysConfig.java Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/server/metrics/OshiSysConfig.java Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java Outdated Show resolved Hide resolved
processing/src/main/java/org/apache/druid/java/util/metrics/OshiSysMonitor.java Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/server/metrics/OshiSysConfig.java Outdated Show resolved Hide resolved
@adithyachakilam adithyachakilam force-pushed the adithyac/dynamic-oshisys branch from 21ccdef to 7088d3e Compare September 9, 2024 02:06
@adithyachakilam adithyachakilam force-pushed the adithyac/dynamic-oshisys branch from 7088d3e to 7bff962 Compare September 9, 2024 02:36
abhishekagarwal87
abhishekagarwal87 requested changes Sep 10, 2024
View reviewed changes
processing/src/main/java/org/apache/druid/java/util/metrics/OshiSysMonitorConfig.java Outdated

public class OshiSysMonitorConfig
{
public static final String PREFIX = "druid.monitoring.oshisys";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need to have oshi in there. OshiSysMonitor is the default sys monitor now. We will likely get rid of SysMonitor completely and make that a symlink to OshiSysMonitor. We will not be able to change these prefixes later.

Copy link
Contributor Author

@adithyachakilam adithyachakilam Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, As of this commit what do you suggest naming it ?

We will not be able to change these prefixes later.

Because , we don't want to update the docs later ?

Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

druid.monitoring.sys

Because , we don't want to update the docs later ?

Because people would have already set the config and we can't change it without breaking their setup.

Copy link
Contributor

@LakshSingla LakshSingla Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is universally the case with sys metrics emitted in the containerized environments, should Druid automatically set the useful categories in the K8sTaskAdapter while launching the task in the container? This will modify the default behavior, but if the metrics are useless (everytime), then it should be fine to make the change.

Copy link
Contributor Author

@adithyachakilam adithyachakilam Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

druid.monitoring.sys

done.

Regarding K8sTaskAdapter I think it depends on the type of installation and requirements of user to specify which categories to specify and hence I would not enforce anything. Also K8sTaskAdapter is just for peons ?

@georgew5656
Copy link
Contributor

looks like a unrelated test failure, going to merge this

@georgew5656 georgew5656 merged commit 6ef8d5d into apache:master Sep 12, 2024
89 of 90 checks passed
cecemei pushed a commit to cecemei/druid that referenced this pull request Sep 12, 2024
@adithyachakilam @cecemei
OshiSysMonitor: Add ability to skip emitting metrics (apache#16972)
b922dc4 
* OshiSysMonitor: Add ability to skip emitting metrics

* comments

* static checks

* remove oshi
@adarshsanjeev adarshsanjeev added this to the 32.0.0 milestone Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunramani arunramani arunramani left review comments

@LakshSingla LakshSingla LakshSingla left review comments

@kfaraz kfaraz kfaraz left review comments

@georgew5656 georgew5656 georgew5656 approved these changes

@abhishekagarwal87 abhishekagarwal87 abhishekagarwal87 approved these changes

Assignees
No one assigned
Labels
Area - Documentation Area - Metrics/Event Emitting
Projects
None yet
Milestone
32.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@adithyachakilam @arunramani @georgew5656 @abhishekagarwal87 @kfaraz @LakshSingla @adarshsanjeev