You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2025/07/24 15:31:08 UTC
[PR] Use LoadScope to list services where a monitor should be loaded (druid)
kfaraz opened a new pull request, #18321:
URL: https://github.com/apache/druid/pull/18321
### Description
Defining a load scope safeguards against loading monitors on services where they are not supported.
It also simplifies configuration so that `druid.monitoring.monitors` can always be defined in
`common.runtime.properties` without requiring to override in service-specific `runtime.properties`.
### Changes
- Use `LoadScope` with monitors, same as with `DruidModule`
- Add test in `MetricsModuleTest`
- Use service-specific monitor in `EmbeddedKafkaClusterMetricsTest`
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18321:
URL: https://github.com/apache/druid/pull/18321#discussion_r2229404903
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -218,4 +217,22 @@ public OshiSysMonitor getOshiSysMonitor(
return new OshiSysMonitor(dimensions, oshiSysConfig);
}
}
+
+ /**
+ * Checks if a monitor needs to be loaded on this service based on its node role.
+ */
+ private boolean shouldLoadMonitor(Class<?> monitorClass, Set<NodeRole> nodeRoles)
+ {
+ final LoadScope loadScope = monitorClass.getAnnotation(LoadScope.class);
Review Comment:
Yeah that's what I was imagining.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18321:
URL: https://github.com/apache/druid/pull/18321#discussion_r2229005393
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -218,4 +217,22 @@ public OshiSysMonitor getOshiSysMonitor(
return new OshiSysMonitor(dimensions, oshiSysConfig);
}
}
+
+ /**
+ * Checks if a monitor needs to be loaded on this service based on its node role.
+ */
+ private boolean shouldLoadMonitor(Class<?> monitorClass, Set<NodeRole> nodeRoles)
+ {
+ final LoadScope loadScope = monitorClass.getAnnotation(LoadScope.class);
Review Comment:
Sure, that can be done.
I guess we would need to check the superclass recursively if there is no annotation defined at the current class level. Would that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #18321:
URL: https://github.com/apache/druid/pull/18321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18321:
URL: https://github.com/apache/druid/pull/18321#issuecomment-3114135014
Thanks for the review, @gianm ! I have updated the docs as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18321:
URL: https://github.com/apache/druid/pull/18321#discussion_r2230058041
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -218,4 +217,22 @@ public OshiSysMonitor getOshiSysMonitor(
return new OshiSysMonitor(dimensions, oshiSysConfig);
}
}
+
+ /**
+ * Checks if a monitor needs to be loaded on this service based on its node role.
+ */
+ private boolean shouldLoadMonitor(Class<?> monitorClass, Set<NodeRole> nodeRoles)
+ {
+ final LoadScope loadScope = monitorClass.getAnnotation(LoadScope.class);
Review Comment:
Thanks! Updated to handle annotations on super-class and added tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain closed pull request #18321: Use LoadScope to list services where a monitor should be loaded
URL: https://github.com/apache/druid/pull/18321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use LoadScope to list services where a monitor should be loaded (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18321:
URL: https://github.com/apache/druid/pull/18321#discussion_r2228961925
##########
server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java:
##########
@@ -218,4 +217,22 @@ public OshiSysMonitor getOshiSysMonitor(
return new OshiSysMonitor(dimensions, oshiSysConfig);
}
}
+
+ /**
+ * Checks if a monitor needs to be loaded on this service based on its node role.
+ */
+ private boolean shouldLoadMonitor(Class<?> monitorClass, Set<NodeRole> nodeRoles)
+ {
+ final LoadScope loadScope = monitorClass.getAnnotation(LoadScope.class);
Review Comment:
Any desire to get fancy here and check superclass annotations too? Probably not necessary but just something that came to mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org