Use LoadScope to list services where a monitor should be loaded#18321
Use LoadScope to list services where a monitor should be loaded#18321kfaraz merged 4 commits intoapache:masterfrom
Conversation
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
The main change LGTM. Can you please also update docs/configuration/index.md? The section "Metrics monitors" makes various references to needing service-specific configuration, which shouldn't be necessary anymore. It could be simplified to say that all monitors should be specified in common.runtime.properties, and they will only be active on the services that support them.
| */ | ||
| private boolean shouldLoadMonitor(Class<?> monitorClass, Set<NodeRole> nodeRoles) | ||
| { | ||
| final LoadScope loadScope = monitorClass.getAnnotation(LoadScope.class); |
There was a problem hiding this comment.
Any desire to get fancy here and check superclass annotations too? Probably not necessary but just something that came to mind.
There was a problem hiding this 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?
There was a problem hiding this comment.
Yeah that's what I was imagining.
There was a problem hiding this comment.
Thanks! Updated to handle annotations on super-class and added tests.
|
Thanks for the review, @gianm ! I have updated the docs as suggested. |
4 participants
Description
Defining a load scope safeguards against loading monitors on services where they are not supported.
It also simplifies configuration so that
druid.monitoring.monitorscan always be defined incommon.runtime.propertieswithout requiring to override in service-specificruntime.properties.Changes
LoadScopewith monitors, same as withDruidModuleMetricsModuleTestEmbeddedKafkaClusterMetricsTestThis PR has: