From 38d26b75cf9b965f0eac7ce090e807a4f6ec1054 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 5 Sep 2024 14:56:48 +0200 Subject: [PATCH 1/6] allow to use remote JMX connections --- .../jmx/JmxMetricInsightInstaller.java | 2 +- .../jmx/engine/BeanAttributeExtractor.java | 41 ++++++------- .../jmx/engine/BeanFinder.java | 58 +++++++++++++------ .../jmx/engine/DetectionStatus.java | 12 ++-- .../jmx/engine/JmxMetricInsight.java | 29 +++++++++- .../jmx/engine/MetricAttribute.java | 6 +- .../jmx/engine/MetricAttributeExtractor.java | 14 ++--- .../jmx/engine/MetricRegistrar.java | 31 +++++----- 8 files changed, 119 insertions(+), 74 deletions(-) diff --git a/instrumentation/jmx-metrics/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java b/instrumentation/jmx-metrics/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java index 74a6f75ffb4f..39d32e69e4a8 100644 --- a/instrumentation/jmx-metrics/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java +++ b/instrumentation/jmx-metrics/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java @@ -36,7 +36,7 @@ public void afterAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) { JmxMetricInsight.createService( GlobalOpenTelemetry.get(), beanDiscoveryDelay(config).toMillis()); MetricConfiguration conf = buildMetricConfiguration(config); - service.start(conf); + service.startLocal(conf); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanAttributeExtractor.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanAttributeExtractor.java index 1581dc23ae3f..baeecec3bdca 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanAttributeExtractor.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanAttributeExtractor.java @@ -16,7 +16,7 @@ import javax.management.InstanceNotFoundException; import javax.management.MBeanAttributeInfo; import javax.management.MBeanInfo; -import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; import javax.management.ObjectName; import javax.management.openmbean.CompositeData; import javax.management.openmbean.TabularData; @@ -132,18 +132,17 @@ public String getAttributeName() { * including the internals of CompositeData and TabularData, if applicable, and that the provided * values will be numerical. * - * @param server the MBeanServer that reported knowledge of the ObjectName - * @param objectName the ObjectName identifying the MBean - * @return AttributeInfo if the attribute is properly recognized, or null + * @param connection the {@link MBeanServerConnection} that reported knowledge of the ObjectName + * @param objectName the {@link ObjectName} identifying the MBean */ @Nullable - AttributeInfo getAttributeInfo(MBeanServer server, ObjectName objectName) { + AttributeInfo getAttributeInfo(MBeanServerConnection connection, ObjectName objectName) { if (logger.isLoggable(FINE)) { logger.log(FINE, "Resolving {0} for {1}", new Object[] {getAttributeName(), objectName}); } try { - MBeanInfo info = server.getMBeanInfo(objectName); + MBeanInfo info = connection.getMBeanInfo(objectName); MBeanAttributeInfo[] allAttributes = info.getAttributes(); for (MBeanAttributeInfo attr : allAttributes) { @@ -152,7 +151,7 @@ AttributeInfo getAttributeInfo(MBeanServer server, ObjectName objectName) { // Verify correctness of configuration by attempting to extract the metric value. // The value will be discarded, but its type will be checked. - Object sampleValue = extractAttributeValue(server, objectName, logger); + Object sampleValue = extractAttributeValue(connection, objectName, logger); // Only numbers can be used to generate metric values if (sampleValue instanceof Number) { @@ -199,18 +198,20 @@ AttributeInfo getAttributeInfo(MBeanServer server, ObjectName objectName) { * Extracts the specified attribute value. In case the value is a CompositeData, drills down into * it to find the correct singleton value (usually a Number or a String). * - * @param server the MBeanServer to use - * @param objectName the ObjectName specifying the MBean to use, it should not be a pattern + * @param connection the {@link MBeanServerConnection} to use + * @param objectName the {@link ObjectName} specifying the MBean to use, it should not be a + * pattern * @param logger the logger to use, may be null. Typically we want to log any issues with the * attributes during MBean discovery, but once the attribute is successfully detected and * confirmed to be eligble for metric evaluation, any further attribute extraction * malfunctions will be silent to avoid flooding the log. - * @return the attribute value, if found, or null if an error occurred + * @return the attribute value, if found, or {@literal null} if an error occurred */ @Nullable - private Object extractAttributeValue(MBeanServer server, ObjectName objectName, Logger logger) { + private Object extractAttributeValue( + MBeanServerConnection connection, ObjectName objectName, Logger logger) { try { - Object value = server.getAttribute(objectName, baseName); + Object value = connection.getAttribute(objectName, baseName); int k = 0; while (k < nameChain.length) { @@ -247,13 +248,13 @@ private Object extractAttributeValue(MBeanServer server, ObjectName objectName, } @Nullable - private Object extractAttributeValue(MBeanServer server, ObjectName objectName) { - return extractAttributeValue(server, objectName, null); + private Object extractAttributeValue(MBeanServerConnection connection, ObjectName objectName) { + return extractAttributeValue(connection, objectName, null); } @Nullable - Number extractNumericalAttribute(MBeanServer server, ObjectName objectName) { - Object value = extractAttributeValue(server, objectName); + Number extractNumericalAttribute(MBeanServerConnection connection, ObjectName objectName) { + Object value = extractAttributeValue(connection, objectName); if (value instanceof Number) { return (Number) value; } @@ -262,13 +263,13 @@ Number extractNumericalAttribute(MBeanServer server, ObjectName objectName) { @Override @Nullable - public String extractValue(MBeanServer server, ObjectName objectName) { - return extractStringAttribute(server, objectName); + public String extractValue(MBeanServerConnection connection, ObjectName objectName) { + return extractStringAttribute(connection, objectName); } @Nullable - private String extractStringAttribute(MBeanServer server, ObjectName objectName) { - Object value = extractAttributeValue(server, objectName); + private String extractStringAttribute(MBeanServerConnection connection, ObjectName objectName) { + Object value = extractAttributeValue(connection, objectName); if (value instanceof String) { return (String) value; } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java index b9856f1dc98c..0e2e1c867d9b 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java @@ -5,6 +5,7 @@ package io.opentelemetry.instrumentation.jmx.engine; +import java.io.IOException; import java.lang.management.ManagementFactory; import java.util.ArrayList; import java.util.HashSet; @@ -13,7 +14,10 @@ import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import javax.management.MBeanServer; +import java.util.function.Supplier; +import java.util.logging.Level; +import java.util.logging.Logger; +import javax.management.MBeanServerConnection; import javax.management.MBeanServerFactory; import javax.management.ObjectName; @@ -23,6 +27,8 @@ */ class BeanFinder { + private static final Logger logger = Logger.getLogger(BeanFinder.class.getName()); + private final MetricRegistrar registrar; private MetricConfiguration conf; private final ScheduledExecutorService exec = @@ -42,12 +48,19 @@ class BeanFinder { this.maxDelay = Math.max(60000, discoveryDelay); } - void discoverBeans(MetricConfiguration conf) { + /** + * Starts bean discovery on a list of local or remote connections + * + * @param conf metric configuration + * @param connections supplier for instances of {@link MBeanServerConnection} + */ + void discoverBeans( + MetricConfiguration conf, Supplier> connections) { this.conf = conf; exec.schedule( () -> { - // Issue 9336: Corner case: PlatformMBeanServer will remain unitialized until a direct + // Issue 9336: Corner case: PlatformMBeanServer will remain uninitialized until a direct // reference to it is made. This call makes sure that the PlatformMBeanServer will be in // the set of MBeanServers reported by MBeanServerFactory. // Issue 11143: This call initializes java.util.logging.LogManager. We should not call it @@ -62,7 +75,7 @@ void discoverBeans(MetricConfiguration conf) { new Runnable() { @Override public void run() { - refreshState(); + refreshState(connections); // Use discoveryDelay as the increment for the actual delay delay = Math.min(delay + discoveryDelay, maxDelay); exec.schedule(this, delay, TimeUnit.MILLISECONDS); @@ -77,9 +90,11 @@ public void run() { * found for a given metric definition, submit the definition to MetricRegistrar for further * handling. Successive invocations of this method may find matches that were previously * unavailable, in such cases MetricRegistrar will extend the coverage for the new MBeans + * + * @param connections supplier providing {@link MBeanServerConnection} instances to query */ - private void refreshState() { - List servers = MBeanServerFactory.findMBeanServer(null); + private void refreshState(Supplier> connections) { + List servers = connections.get(); for (MetricDef metricDef : conf.getMetricDefs()) { resolveBeans(metricDef, servers); @@ -92,22 +107,28 @@ private void refreshState() { * collection of corresponding metrics. * * @param metricDef the MetricDef used to find matching MBeans - * @param servers the list of MBeanServers to query + * @param connections the list of {@link MBeanServerConnection} to query */ - private void resolveBeans(MetricDef metricDef, List servers) { + private void resolveBeans( + MetricDef metricDef, List connections) { BeanGroup beans = metricDef.getBeanGroup(); - for (MBeanServer server : servers) { + for (MBeanServerConnection connection : connections) { // The set of all matching ObjectNames recognized by the server Set allObjectNames = new HashSet<>(); for (ObjectName pattern : beans.getNamePatterns()) { - Set objectNames = server.queryNames(pattern, beans.getQueryExp()); - allObjectNames.addAll(objectNames); + Set objectNames = null; + try { + objectNames = connection.queryNames(pattern, beans.getQueryExp()); + allObjectNames.addAll(objectNames); + } catch (IOException e) { + logger.log(Level.WARNING, "IO error while querying mbean", e); + } } if (!allObjectNames.isEmpty()) { - resolveAttributes(allObjectNames, server, metricDef); + resolveAttributes(allObjectNames, connection, metricDef); // Assuming that only one MBeanServer has the required MBeans break; @@ -119,19 +140,20 @@ private void resolveBeans(MetricDef metricDef, List servers) { * Go over the collection of matching MBeans and try to find all matching attributes. For every * successful match, activate metric value collection. * - * @param objectNames the collection of ObjectNames identifying the MBeans - * @param server the MBeanServer which recognized the collection of ObjectNames - * @param metricDef the MetricDef describing the attributes to look for + * @param objectNames the collection of {@link ObjectName}s identifying the MBeans + * @param connection the {@link MBeanServerConnection} which recognized the collection of + * ObjectNames + * @param metricDef the {@link MetricDef} describing the attributes to look for */ private void resolveAttributes( - Set objectNames, MBeanServer server, MetricDef metricDef) { + Set objectNames, MBeanServerConnection connection, MetricDef metricDef) { for (MetricExtractor extractor : metricDef.getMetricExtractors()) { // For each MetricExtractor, find the subset of MBeans that have the required attribute List validObjectNames = new ArrayList<>(); AttributeInfo attributeInfo = null; for (ObjectName objectName : objectNames) { AttributeInfo attr = - extractor.getMetricValueExtractor().getAttributeInfo(server, objectName); + extractor.getMetricValueExtractor().getAttributeInfo(connection, objectName); if (attr != null) { if (attributeInfo == null) { attributeInfo = attr; @@ -143,7 +165,7 @@ private void resolveAttributes( } if (!validObjectNames.isEmpty()) { // Ready to collect metric values - registrar.enrollExtractor(server, validObjectNames, extractor, attributeInfo); + registrar.enrollExtractor(connection, validObjectNames, extractor, attributeInfo); } } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/DetectionStatus.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/DetectionStatus.java index e012295fda19..d47da424bc84 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/DetectionStatus.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/DetectionStatus.java @@ -6,7 +6,7 @@ package io.opentelemetry.instrumentation.jmx.engine; import java.util.Collection; -import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; import javax.management.ObjectName; /** @@ -15,16 +15,16 @@ */ class DetectionStatus { - private final MBeanServer server; + private final MBeanServerConnection connection; private final Collection objectNames; - DetectionStatus(MBeanServer server, Collection objectNames) { - this.server = server; + DetectionStatus(MBeanServerConnection connection, Collection objectNames) { + this.connection = connection; this.objectNames = objectNames; } - MBeanServer getServer() { - return server; + MBeanServerConnection getConnection() { + return connection; } Collection getObjectNames() { diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/JmxMetricInsight.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/JmxMetricInsight.java index 1817c79997d0..acc2f5725fe3 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/JmxMetricInsight.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/JmxMetricInsight.java @@ -8,7 +8,11 @@ import static java.util.logging.Level.FINE; import io.opentelemetry.api.OpenTelemetry; +import java.util.List; +import java.util.function.Supplier; import java.util.logging.Logger; +import javax.management.MBeanServerConnection; +import javax.management.MBeanServerFactory; /** Collecting and exporting JMX metrics. */ public class JmxMetricInsight { @@ -33,7 +37,28 @@ private JmxMetricInsight(OpenTelemetry openTelemetry, long discoveryDelay) { this.discoveryDelay = discoveryDelay; } - public void start(MetricConfiguration conf) { + /** + * Starts metric registration for local JVM + * + * @param conf metric configuration + */ + public void startLocal(MetricConfiguration conf) { + start(conf, () -> MBeanServerFactory.findMBeanServer(null)); + } + + /** + * Starts metric registration for a remote JVM connection + * + * @param conf metric configuration + * @param connections supplier for list of remote connections + */ + public void startRemote( + MetricConfiguration conf, Supplier> connections) { + start(conf, connections); + } + + private void start( + MetricConfiguration conf, Supplier> connections) { if (conf.isEmpty()) { logger.log( FINE, @@ -42,7 +67,7 @@ public void start(MetricConfiguration conf) { } else { MetricRegistrar registrar = new MetricRegistrar(openTelemetry, INSTRUMENTATION_SCOPE); BeanFinder finder = new BeanFinder(registrar, discoveryDelay); - finder.discoverBeans(conf); + finder.discoverBeans(conf, connections); } } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttribute.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttribute.java index 2dfff5509752..882f1049c62d 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttribute.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttribute.java @@ -5,7 +5,7 @@ package io.opentelemetry.instrumentation.jmx.engine; -import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; import javax.management.ObjectName; /** @@ -26,7 +26,7 @@ public String getAttributeName() { return name; } - String acquireAttributeValue(MBeanServer server, ObjectName objectName) { - return extractor.extractValue(server, objectName); + String acquireAttributeValue(MBeanServerConnection connection, ObjectName objectName) { + return extractor.extractValue(connection, objectName); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java index 61c9bc03c283..c359aacfe764 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java @@ -6,7 +6,7 @@ package io.opentelemetry.instrumentation.jmx.engine; import javax.annotation.Nullable; -import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; import javax.management.ObjectName; /** @@ -18,28 +18,24 @@ public interface MetricAttributeExtractor { /** * Provide a String value to be used as the value of a metric attribute. * - * @param server MBeanServer to query, must not be null if the extraction is from an MBean + * @param connection MBeanServer to query, must not be null if the extraction is from an MBean * attribute * @param objectName the identifier of the MBean to query, must not be null if the extraction is * from an MBean attribute, or from the ObjectName parameter * @return the value of the attribute, can be null if extraction failed */ @Nullable - String extractValue(@Nullable MBeanServer server, @Nullable ObjectName objectName); + String extractValue(@Nullable MBeanServerConnection connection, @Nullable ObjectName objectName); static MetricAttributeExtractor fromConstant(String constantValue) { - return (a, b) -> { - return constantValue; - }; + return (a, b) -> constantValue; } static MetricAttributeExtractor fromObjectNameParameter(String parameterKey) { if (parameterKey.isEmpty()) { throw new IllegalArgumentException("Empty parameter name"); } - return (dummy, objectName) -> { - return objectName.getKeyProperty(parameterKey); - }; + return (dummy, objectName) -> objectName.getKeyProperty(parameterKey); } static MetricAttributeExtractor fromBeanAttribute(String attributeName) { diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index 58bec6fb3887..1fd5cc52393e 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -19,7 +19,7 @@ import java.util.Collection; import java.util.function.Consumer; import java.util.logging.Logger; -import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; import javax.management.ObjectName; /** A class responsible for maintaining the set of metrics to collect and report. */ @@ -36,18 +36,19 @@ class MetricRegistrar { /** * Accepts a MetricExtractor for registration and activation. * - * @param server the MBeanServer to use to query for metric values - * @param objectNames the Objectnames that are known to the server and that know the attribute - * that is required to get the metric values - * @param extractor the MetricExtractor responsible for getting the metric values + * @param connection the {@link MBeanServerConnection} to use to query for metric values + * @param objectNames the {@link ObjectName} that are known to the server and that know the + * attribute that is required to get the metric values + * @param extractor the {@link MetricExtractor} responsible for getting the metric values + * @param attributeInfo the {@link AttributeInfo} */ void enrollExtractor( - MBeanServer server, + MBeanServerConnection connection, Collection objectNames, MetricExtractor extractor, AttributeInfo attributeInfo) { // For the first enrollment of the extractor we have to build the corresponding Instrument - DetectionStatus status = new DetectionStatus(server, objectNames); + DetectionStatus status = new DetectionStatus(connection, objectNames); boolean firstEnrollment; synchronized (extractor) { firstEnrollment = extractor.getStatus() == null; @@ -139,13 +140,13 @@ static Consumer doubleTypeCallback(MetricExtractor return measurement -> { DetectionStatus status = extractor.getStatus(); if (status != null) { - MBeanServer server = status.getServer(); + MBeanServerConnection connection = status.getConnection(); for (ObjectName objectName : status.getObjectNames()) { Number metricValue = - extractor.getMetricValueExtractor().extractNumericalAttribute(server, objectName); + extractor.getMetricValueExtractor().extractNumericalAttribute(connection, objectName); if (metricValue != null) { // get the metric attributes - Attributes attr = createMetricAttributes(server, objectName, extractor); + Attributes attr = createMetricAttributes(connection, objectName, extractor); measurement.record(metricValue.doubleValue(), attr); } } @@ -161,13 +162,13 @@ static Consumer longTypeCallback(MetricExtractor extr return measurement -> { DetectionStatus status = extractor.getStatus(); if (status != null) { - MBeanServer server = status.getServer(); + MBeanServerConnection connection = status.getConnection(); for (ObjectName objectName : status.getObjectNames()) { Number metricValue = - extractor.getMetricValueExtractor().extractNumericalAttribute(server, objectName); + extractor.getMetricValueExtractor().extractNumericalAttribute(connection, objectName); if (metricValue != null) { // get the metric attributes - Attributes attr = createMetricAttributes(server, objectName, extractor); + Attributes attr = createMetricAttributes(connection, objectName, extractor); measurement.record(metricValue.longValue(), attr); } } @@ -180,11 +181,11 @@ static Consumer longTypeCallback(MetricExtractor extr * the metric values */ static Attributes createMetricAttributes( - MBeanServer server, ObjectName objectName, MetricExtractor extractor) { + MBeanServerConnection connection, ObjectName objectName, MetricExtractor extractor) { MetricAttribute[] metricAttributes = extractor.getAttributes(); AttributesBuilder attrBuilder = Attributes.builder(); for (MetricAttribute metricAttribute : metricAttributes) { - String attributeValue = metricAttribute.acquireAttributeValue(server, objectName); + String attributeValue = metricAttribute.acquireAttributeValue(connection, objectName); if (attributeValue != null) { attrBuilder = attrBuilder.put(metricAttribute.getAttributeName(), attributeValue); } From 7eb0706cb92879699d0d16fc115136f89097c90e Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 5 Sep 2024 15:08:26 +0200 Subject: [PATCH 2/6] spotless --- .../io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java index 0e2e1c867d9b..823819b9227d 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java @@ -18,7 +18,6 @@ import java.util.logging.Level; import java.util.logging.Logger; import javax.management.MBeanServerConnection; -import javax.management.MBeanServerFactory; import javax.management.ObjectName; /** From 42e1dbf219653b73f02c1d66ffc79a5a9bc1803b Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 5 Sep 2024 17:43:01 +0200 Subject: [PATCH 3/6] minor code refactor --- .../jmx/engine/BeanFinder.java | 6 +- .../jmx/engine/MetricRegistrar.java | 131 ++++++++---------- 2 files changed, 63 insertions(+), 74 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java index 823819b9227d..2ab3babe9a6c 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java @@ -117,12 +117,10 @@ private void resolveBeans( Set allObjectNames = new HashSet<>(); for (ObjectName pattern : beans.getNamePatterns()) { - Set objectNames = null; try { - objectNames = connection.queryNames(pattern, beans.getQueryExp()); - allObjectNames.addAll(objectNames); + allObjectNames.addAll(connection.queryNames(pattern, beans.getQueryExp())); } catch (IOException e) { - logger.log(Level.WARNING, "IO error while querying mbean", e); + logger.log(Level.WARNING, "IO error while resolving mbean", e); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index 1fd5cc52393e..44961345dda8 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -17,6 +17,7 @@ import io.opentelemetry.api.metrics.ObservableDoubleMeasurement; import io.opentelemetry.api.metrics.ObservableLongMeasurement; import java.util.Collection; +import java.util.Optional; import java.util.function.Consumer; import java.util.logging.Logger; import javax.management.MBeanServerConnection; @@ -56,79 +57,69 @@ void enrollExtractor( extractor.setStatus(status); } - if (firstEnrollment) { - MetricInfo metricInfo = extractor.getInfo(); - String metricName = metricInfo.getMetricName(); - MetricInfo.Type instrumentType = metricInfo.getType(); - String description = - metricInfo.getDescription() != null - ? metricInfo.getDescription() - : attributeInfo.getDescription(); - String unit = metricInfo.getUnit(); - - switch (instrumentType) { - // CHECKSTYLE:OFF - case COUNTER: - { - // CHECKSTYLE:ON - LongCounterBuilder builder = meter.counterBuilder(metricName); - if (description != null) { - builder = builder.setDescription(description); - } - if (unit != null) { - builder = builder.setUnit(unit); - } - - if (attributeInfo.usesDoubleValues()) { - builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); - } else { - builder.buildWithCallback(longTypeCallback(extractor)); - } - logger.log(INFO, "Created Counter for {0}", metricName); + if (!firstEnrollment) { + return; + } + + MetricInfo metricInfo = extractor.getInfo(); + String metricName = metricInfo.getMetricName(); + MetricInfo.Type instrumentType = metricInfo.getType(); + String description = + metricInfo.getDescription() != null + ? metricInfo.getDescription() + : attributeInfo.getDescription(); + String unit = metricInfo.getUnit(); + + switch (instrumentType) { + // CHECKSTYLE:OFF + case COUNTER: + { + // CHECKSTYLE:ON + LongCounterBuilder builder = meter.counterBuilder(metricName); + Optional.ofNullable(description).ifPresent(builder::setDescription); + Optional.ofNullable(unit).ifPresent(builder::setUnit); + + if (attributeInfo.usesDoubleValues()) { + builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); + } else { + builder.buildWithCallback(longTypeCallback(extractor)); } - break; - - // CHECKSTYLE:OFF - case UPDOWNCOUNTER: - { - // CHECKSTYLE:ON - LongUpDownCounterBuilder builder = meter.upDownCounterBuilder(metricName); - if (description != null) { - builder = builder.setDescription(description); - } - if (unit != null) { - builder = builder.setUnit(unit); - } - - if (attributeInfo.usesDoubleValues()) { - builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); - } else { - builder.buildWithCallback(longTypeCallback(extractor)); - } - logger.log(INFO, "Created UpDownCounter for {0}", metricName); + logger.log(INFO, "Created Counter for {0}", metricName); + } + break; + + // CHECKSTYLE:OFF + case UPDOWNCOUNTER: + { + // CHECKSTYLE:ON + LongUpDownCounterBuilder builder = meter.upDownCounterBuilder(metricName); + Optional.ofNullable(description).ifPresent(builder::setDescription); + Optional.ofNullable(unit).ifPresent(builder::setUnit); + + if (attributeInfo.usesDoubleValues()) { + builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); + } else { + builder.buildWithCallback(longTypeCallback(extractor)); } - break; - - // CHECKSTYLE:OFF - case GAUGE: - { - // CHECKSTYLE:ON - DoubleGaugeBuilder builder = meter.gaugeBuilder(metricName); - if (description != null) { - builder = builder.setDescription(description); - } - if (unit != null) { - builder = builder.setUnit(unit); - } - - if (attributeInfo.usesDoubleValues()) { - builder.buildWithCallback(doubleTypeCallback(extractor)); - } else { - builder.ofLongs().buildWithCallback(longTypeCallback(extractor)); - } - logger.log(INFO, "Created Gauge for {0}", metricName); + logger.log(INFO, "Created UpDownCounter for {0}", metricName); + } + break; + + // CHECKSTYLE:OFF + case GAUGE: + { + // CHECKSTYLE:ON + DoubleGaugeBuilder builder = meter.gaugeBuilder(metricName); + Optional.ofNullable(description).ifPresent(builder::setDescription); + Optional.ofNullable(unit).ifPresent(builder::setUnit); + + if (attributeInfo.usesDoubleValues()) { + builder.buildWithCallback(doubleTypeCallback(extractor)); + } else { + builder.ofLongs().buildWithCallback(longTypeCallback(extractor)); } - } + logger.log(INFO, "Created Gauge for {0}", metricName); + } } } From 06acbd62e428437967a872a3fe6bd2be692ac657 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 5 Sep 2024 17:43:27 +0200 Subject: [PATCH 4/6] use assertj assertions --- .../jmx/JmxMetricInsightInstallerTest.java | 2 +- .../jmx/engine/AttributeExtractorTest.java | 70 +++++++++---------- 2 files changed, 35 insertions(+), 37 deletions(-) diff --git a/instrumentation/jmx-metrics/javaagent/src/test/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstallerTest.java b/instrumentation/jmx-metrics/javaagent/src/test/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstallerTest.java index 253583660ddd..55d0c51f8c1b 100644 --- a/instrumentation/jmx-metrics/javaagent/src/test/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstallerTest.java +++ b/instrumentation/jmx-metrics/javaagent/src/test/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstallerTest.java @@ -38,7 +38,7 @@ class JmxMetricInsightInstallerTest { @Test void testToVerifyExistingRulesAreValid() throws Exception { RuleParser parser = RuleParser.get(); - assertThat(parser == null).isFalse(); + assertThat(parser).isNotNull(); Path path = Paths.get(PATH_TO_ALL_EXISTING_RULES); assertThat(Files.exists(path)).isTrue(); diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/AttributeExtractorTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/AttributeExtractorTest.java index 487f4d3bc1ca..5202ceeebe67 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/AttributeExtractorTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/AttributeExtractorTest.java @@ -93,113 +93,111 @@ static void tearDown() { } @Test - void testSetup() throws Exception { + void testSetup() { Set set = theServer.queryNames(objectName, null); - assertThat(set == null).isFalse(); - assertThat(set.size() == 1).isTrue(); - assertThat(set.contains(objectName)).isTrue(); + assertThat(set).isNotNull().hasSize(1).contains(objectName); } @Test - void testByteAttribute() throws Exception { + void testByteAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ByteAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isFalse(); } @Test - void testByteAttributeValue() throws Exception { + void testByteAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ByteAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.longValue() == 10).isTrue(); + assertThat(number).isNotNull(); + assertThat(number.longValue()).isEqualTo(10); } @Test - void testShortAttribute() throws Exception { + void testShortAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ShortAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isFalse(); } @Test - void testShortAttributeValue() throws Exception { + void testShortAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ShortAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.longValue() == 11).isTrue(); + assertThat(number).isNotNull(); + assertThat(number.longValue()).isEqualTo(11); } @Test - void testIntAttribute() throws Exception { + void testIntAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("IntAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isFalse(); } @Test - void testIntAttributeValue() throws Exception { + void testIntAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("IntAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.longValue() == 12).isTrue(); + assertThat(number).isNotNull(); + assertThat(number.longValue()).isEqualTo(12); } @Test - void testLongAttribute() throws Exception { + void testLongAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("LongAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isFalse(); } @Test - void testLongAttributeValue() throws Exception { + void testLongAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("LongAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.longValue() == 13).isTrue(); + assertThat(number).isNotNull(); + assertThat(number.longValue()).isEqualTo(13); } @Test - void testFloatAttribute() throws Exception { + void testFloatAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("FloatAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isTrue(); } @Test - void testFloatAttributeValue() throws Exception { + void testFloatAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("FloatAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.doubleValue() == 14.0).isTrue(); // accurate representation + assertThat(number).isNotNull(); + assertThat(number.doubleValue()).isEqualTo(14.0); // accurate representation } @Test - void testDoubleAttribute() throws Exception { + void testDoubleAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("DoubleAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isFalse(); + assertThat(info).isNotNull(); assertThat(info.usesDoubleValues()).isTrue(); } @Test - void testDoubleAttributeValue() throws Exception { + void testDoubleAttributeValue() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("DoubleAttribute"); Number number = extractor.extractNumericalAttribute(theServer, objectName); - assertThat(number == null).isFalse(); - assertThat(number.doubleValue() == 15.0).isTrue(); // accurate representation + assertThat(number).isNotNull(); + assertThat(number.doubleValue()).isEqualTo(15.0); // accurate representation } @Test - void testStringAttribute() throws Exception { + void testStringAttribute() { BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("StringAttribute"); AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); - assertThat(info == null).isTrue(); + assertThat(info).isNull(); } } From 25cd7c0729e5a061aae8e17d21bf2db6cf43c2bc Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 13 Sep 2024 21:09:16 +0200 Subject: [PATCH 5/6] add comment to clarify why supplier is needed --- .../opentelemetry/instrumentation/jmx/engine/BeanFinder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java index 2ab3babe9a6c..b6bb6d3c00af 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java @@ -51,7 +51,8 @@ class BeanFinder { * Starts bean discovery on a list of local or remote connections * * @param conf metric configuration - * @param connections supplier for instances of {@link MBeanServerConnection} + * @param connections supplier for instances of {@link MBeanServerConnection}, will be invoked after + * delayed JMX initialization once the {@link #discoveryDelay} has expired. */ void discoverBeans( MetricConfiguration conf, Supplier> connections) { From b467041a3e2625302a45af0e537a3f0f8828c46a Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 13 Sep 2024 21:40:06 +0200 Subject: [PATCH 6/6] make spotless happy --- .../opentelemetry/instrumentation/jmx/engine/BeanFinder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java index b6bb6d3c00af..a5a05ee20660 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanFinder.java @@ -51,8 +51,8 @@ class BeanFinder { * Starts bean discovery on a list of local or remote connections * * @param conf metric configuration - * @param connections supplier for instances of {@link MBeanServerConnection}, will be invoked after - * delayed JMX initialization once the {@link #discoveryDelay} has expired. + * @param connections supplier for instances of {@link MBeanServerConnection}, will be invoked + * after delayed JMX initialization once the {@link #discoveryDelay} has expired. */ void discoverBeans( MetricConfiguration conf, Supplier> connections) {