Allow producing complex jmx based metrics#18782
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR extends the JMX metrics library to support “complex” metrics produced by custom, user-provided logic via an SPI (JmxMetricHandler) referenced from YAML rules, and wires handler discovery through an injectable ComponentLoader/service classloader.
Changes:
- Add
JmxMetricHandlerSPI and YAML support (handler/handlers) to bind rules to custom metric producers. - Plumb a
ComponentLoaderfromJmxTelemetryBuilderinto the engine so handlers can be discovered via SPI at runtime. - Add lifecycle management for created instruments (returning
AutoCloseablehandles and closing registered instruments).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation/jmx-metrics/library/src/test/resources/jmx/rules/handler.yaml | Adds a test rule referencing a custom handler by name. |
| instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/rules/HandlerTest.java | New test validating handler discovery via SPI and metric emission. |
| instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricAggregationTest.java | Updates MetricDef construction for the new handlers parameter. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/JmxTelemetryBuilder.java | Adds service classloader injection and passes a ComponentLoader into JmxTelemetry. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/JmxTelemetry.java | Passes ComponentLoader into the engine and changes start* to return AutoCloseable. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/JmxMetricHandler.java | Introduces the public SPI for custom metric creation. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/yaml/RuleParser.java | Parses handler(s) keys and allows rules with handlers but no mapping. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/yaml/JmxRule.java | Stores handler references and builds MetricDef with handler names. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricRegistrar.java | Tracks created instruments and closes them; adds handler enrollment. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricHandlerHolder.java | New holder for handler + detection status to support (intended) refresh. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricExtractor.java | Makes status updates synchronized and returns “first enrollment” flag. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricDef.java | Adds handler list to metric definitions. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/JmxMetricInsight.java | Loads handlers via ComponentLoader and returns an AutoCloseable lifecycle handle. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/BeanFinder.java | Validates handler names and enrolls handlers when beans are found. |
| instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/BeanAttributeExtractor.java | Minor modernization (toArray(new String[0])) and annotation cleanup. |
Comments suppressed due to low confidence (1)
instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/BeanFinder.java:205
resolveHandlers()creates a newMetricHandlerHolderon everyrefreshState()run. SincerefreshState()is rescheduled indefinitely, this makesMetricRegistrar.enrollHandler()treat every run as a first enrollment, repeatedly creating/registering the same handler instruments and leaking resources/duplicating metrics. Persist the holder(s) across refresh cycles (e.g., storeMetricHandlerHolderinstances inMetricDefor inBeanFinderkeyed by rule+handlerName) and only create instruments once, updating status on subsequent refreshes (similar toMetricExtractor).
private void resolveHandlers(
Set<ObjectName> objectNames, MBeanServerConnection connection, MetricDef metricDef) {
for (String handlerName : metricDef.getHandlers()) {
JmxMetricHandler handler = handlers.get(handlerName);
// we remove rules that reference unknown handlers during construction, so this should never
// happen
if (handler == null) {
throw new IllegalStateException("Unknown handler " + handlerName);
}
MetricHandlerHolder holder = new MetricHandlerHolder(handler);
registrar.enrollHandler(connection, objectNames, holder);
}
| /** Starts JMX metrics collection on current JVM */ | ||
| public AutoCloseable start() { | ||
| return start(() -> MBeanServerFactory.findMBeanServer(null)); | ||
| } | ||
|
|
||
| /** | ||
| * Starts JMX metrics collection on provided (local or remote) connections | ||
| * | ||
| * @param connections connection provider | ||
| * @return this | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public JmxTelemetry start(Supplier<List<? extends MBeanServerConnection>> connections) { | ||
| service.start(metricConfiguration, connections); | ||
| return this; | ||
| public AutoCloseable start(Supplier<List<? extends MBeanServerConnection>> connections) { | ||
| return service.start(metricConfiguration, connections, componentLoader); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricHandlerHolder.java:27
- [Naming]
handlerClassName/getHandlerClassName()is misleading: the YAMLhandlervalue is matched againstJmxMetricHandler.getName(), not a class name. Renaming this tohandlerName(and corresponding accessor) would make the intent clearer and reduce confusion for future maintenance.
private final String handlerClassName;
@Nullable private JmxMetricHandler handler;
@Nullable private volatile DetectionStatus status;
public MetricHandlerHolder(String handlerClassName) {
this.handlerClassName = handlerClassName;
}
String getHandlerClassName() {
return handlerClassName;
}
| public class MetricHandlerHolder { | ||
|
|
||
| private final String handlerName; | ||
| @Nullable private JmxMetricHandler handler; | ||
| @Nullable private volatile DetectionStatus status; | ||
|
|
||
| public MetricHandlerHolder(String handlerName) { | ||
| this.handlerName = handlerName; | ||
| } | ||
|
|
||
| String getHandlerName() { | ||
| return handlerName; | ||
| } | ||
|
|
||
| JmxMetricHandler getHandler() { | ||
| return handler; | ||
| } |
There was a problem hiding this comment.
MetricHandlerHolder is used from a different package and can't be package private
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/internal/engine/MetricRegistrar.java:255
close()catches allExceptions from instrument cleanup but doesn’t preserve the thread interrupt status when anInterruptedExceptionis caught (it will be swallowed and the interrupt flag cleared). Consider special-casingInterruptedExceptionto re-interrupt the thread after logging.
@Override
public void close() {
for (AutoCloseable instrument : instruments) {
try {
instrument.close();
} catch (Exception e) {
logger.log(WARNING, "Failed to close metric instrument", e);
}
}
instruments.clear();
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| void setHandler(JmxMetricHandler handler) { | ||
| if (this.handler != null) { | ||
| throw new IllegalStateException("Handler is already set"); | ||
| } |
There was a problem hiding this comment.
This isn't enough to make calling start multiple times work.
|
I think that it would be better to make the Then, all the available |
I chose to do it the way it is under the assumption that the vast majority of metrics can be expressed in yaml and there are going to be only a few that are too complicated. Since we have one yaml for each target that describes all the metrics I thought it would nice to have a reference from the yaml to these complex metrics.
Currently the Would it help if I named |
I think this is fine to keep it this way, I was just thinking about the possibility to have such an abstraction. The only use-case I would have for this is to provide an alternate YAML format, for example using the one used by weaver/semconv.
Yes, given we likely need to validate in a few iterations the design of this API with a few user-provided implementations then it would be a good idea to make it internal and have "Experimental" in its name. |
Resolves #17433
Some metrics are too complex to be expressed in yaml. This PR allows implementing such metrics in code.