jmx-metrics: allow any resource path for classpath resources rules (#15413)

Author: SylvainJuge

instrumentation/jmx-metrics/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jmx/JmxMetricInsightInstaller.java

@@ -13,6 +13,8 @@
 import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
 import io.opentelemetry.sdk.autoconfigure.internal.AutoConfigureUtil;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
+import java.io.InputStream;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Duration;
 import java.util.logging.Level;
@@ -33,18 +35,37 @@ public void afterAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
           JmxTelemetry.builder(GlobalOpenTelemetry.get())
               .beanDiscoveryDelay(beanDiscoveryDelay(config));
 
-      try {
-        config.getList("otel.jmx.config").stream().map(Paths::get).forEach(jmx::addCustomRules);
-        config.getList("otel.jmx.target.system").forEach(jmx::addClassPathRules);
-      } catch (RuntimeException e) {
-        // for now only log JMX errors as they do not prevent agent startup
-        logger.log(Level.SEVERE, "Error while loading JMX configuration", e);
-      }
+      config.getList("otel.jmx.config").stream()
+          .map(Paths::get)
+          .forEach(path -> addFileRules(path, jmx));
+      config.getList("otel.jmx.target.system").forEach(target -> addClasspathRules(target, jmx));
 
       jmx.build().start();
     }
   }
 
+  private static void addFileRules(Path path, JmxTelemetryBuilder builder) {
+    try {
+      builder.addRules(path);
+    } catch (RuntimeException e) {
+      // for now only log JMX metric configuration errors as they do not prevent agent startup
+      logger.log(Level.SEVERE, "Error while loading JMX configuration from " + path, e);
+    }
+  }
+
+  private static void addClasspathRules(String target, JmxTelemetryBuilder builder) {
+    ClassLoader classLoader = JmxTelemetryBuilder.class.getClassLoader();
+    String resource = String.format("jmx/rules/%s.yaml", target);
+    InputStream input = classLoader.getResourceAsStream(resource);
+    try {
+      builder.addRules(input);
+    } catch (RuntimeException e) {
+      // for now only log JMX metric configuration errors as they do not prevent agent startup
+      logger.log(
+          Level.SEVERE, "Error while loading JMX configuration from classpath " + resource, e);
+    }
+  }
+
   private static Duration beanDiscoveryDelay(ConfigProperties configProperties) {
     Duration discoveryDelay = configProperties.getDuration("otel.jmx.discovery.delay");
     if (discoveryDelay != null) {

instrumentation/jmx-metrics/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jmx/JmxMetricInsightInstallerTest.java

@@ -41,7 +41,7 @@ void testToVerifyExistingRulesAreValid() throws Exception {
       assertThat(filePath).isRegularFile();
 
       // loading rules from direct file access
-      JmxTelemetry.builder(OpenTelemetry.noop()).addCustomRules(filePath);
+      JmxTelemetry.builder(OpenTelemetry.noop()).addRules(filePath);
     }
   }
 }

instrumentation/jmx-metrics/library/README.md

@@ -35,18 +35,20 @@ import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.jmx.JmxTelemetry;
 import io.opentelemetry.instrumentation.jmx.JmxTelemetryBuilder;
 
+import java.time.Duration;
+
 // Get an OpenTelemetry instance
-OpenTelemetry openTelemetry = ...
+OpenTelemetry openTelemetry = ...;
 
 JmxTelemetry jmxTelemetry = JmxTelemetry.builder(openTelemetry)
   // Configure included metrics (optional)
-  .addClasspathRules("tomcat")
-  .addClasspathRules("jetty")
+  .addRules(JmxTelemetry.class.getClassLoader().getResourceAsStream("jmx/rules/jetty.yaml"), "jetty")
+  .addRules(JmxTelemetry.class.getClassLoader().getResourceAsStream("jmx/rules/tomcat.yaml"), "tomcat")
   // Configure custom metrics (optional)
-  .addCustomRules("/path/to/custom-jmx.yaml")
+  .addRules(Paths.get("/path/to/custom-jmx.yaml"))
   // delay bean discovery by 5 seconds
-  .beanDiscoveryDelay(5000)
+  .beanDiscoveryDelay(Duration.ofSeconds(5))
   .build();
 
-jmxTelemetry.startLocal();
+jmxTelemetry.start();
 ```

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/JmxTelemetryBuilder.java

@@ -49,47 +49,73 @@ public JmxTelemetryBuilder beanDiscoveryDelay(Duration delay) {
   }
 
   /**
-   * Adds built-in JMX rules from classpath resource
+   * Adds built-in JMX rules from classpath resource.
    *
    * @param target name of target in /jmx/rules/{target}.yaml classpath resource
    * @return builder instance
    * @throws IllegalArgumentException when classpath resource does not exist or can't be parsed
    */
+  // TODO: deprecate this method after 2.23.0 release in favor of addRules
   @CanIgnoreReturnValue
   public JmxTelemetryBuilder addClassPathRules(String target) {
-    String yamlResource = String.format("jmx/rules/%s.yaml", target);
-    try (InputStream inputStream =
-        JmxTelemetryBuilder.class.getClassLoader().getResourceAsStream(yamlResource)) {
-      if (inputStream == null) {
-        throw new IllegalArgumentException("JMX rules not found in classpath: " + yamlResource);
-      }
-      logger.log(FINE, "Adding JMX config from classpath for {0}", yamlResource);
-      RuleParser parserInstance = RuleParser.get();
-      parserInstance.addMetricDefsTo(metricConfiguration, inputStream, target);
-    } catch (Exception e) {
+    String resourcePath = String.format("jmx/rules/%s.yaml", target);
+    ClassLoader classLoader = JmxTelemetryBuilder.class.getClassLoader();
+    logger.log(FINE, "Adding JMX config from classpath {0}", resourcePath);
+    try (InputStream inputStream = classLoader.getResourceAsStream(resourcePath)) {
+      return addRules(inputStream);
+    } catch (IOException e) {
       throw new IllegalArgumentException(
-          "Unable to load JMX rules from classpath: " + yamlResource, e);
+          "Unable to load JMX rules from resource " + resourcePath, e);
     }
+  }
+
+  /**
+   * Adds JMX rules from input stream
+   *
+   * @param input input to read rules from
+   * @throws IllegalArgumentException when input is {@literal null} or can't be parsed
+   */
+  @CanIgnoreReturnValue
+  public JmxTelemetryBuilder addRules(InputStream input) {
+    if (input == null) {
+      throw new IllegalArgumentException("missing JMX rules");
+    }
+    RuleParser parserInstance = RuleParser.get();
+    parserInstance.addMetricDefsTo(metricConfiguration, input);
     return this;
   }
 
   /**
-   * Adds custom JMX rules from file system path
+   * Adds JMX rules from file system path
    *
    * @param path path to yaml file
    * @return builder instance
-   * @throws IllegalArgumentException when classpath resource does not exist or can't be parsed
+   * @throws IllegalArgumentException in case of parsing errors or when file does not exist
    */
   @CanIgnoreReturnValue
-  public JmxTelemetryBuilder addCustomRules(Path path) {
-    logger.log(FINE, "Adding JMX config from file: {0}", path);
-    RuleParser parserInstance = RuleParser.get();
+  public JmxTelemetryBuilder addRules(Path path) {
+    if (path == null) {
+      throw new IllegalArgumentException("missing JMX rules");
+    }
     try (InputStream inputStream = Files.newInputStream(path)) {
-      parserInstance.addMetricDefsTo(metricConfiguration, inputStream, path.toString());
+      logger.log(FINE, "Adding JMX config from file {0}", path);
+      return addRules(inputStream);
     } catch (IOException e) {
-      throw new IllegalArgumentException("Unable to load JMX rules in path: " + path, e);
+      throw new IllegalArgumentException("Unable to load JMX rules from: " + path, e);
     }
-    return this;
+  }
+
+  /**
+   * Adds custom JMX rules from file system path
+   *
+   * @param path path to yaml file
+   * @return builder instance
+   * @throws IllegalArgumentException when classpath resource does not exist or can't be parsed
+   */
+  // TODO: deprecate this method after 2.23.0 release in favor of addRules
+  @CanIgnoreReturnValue
+  public JmxTelemetryBuilder addCustomRules(Path path) {
+    return addRules(path);
   }
 
   public JmxTelemetry build() {

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/RuleParser.java

@@ -6,7 +6,7 @@
 package io.opentelemetry.instrumentation.jmx.yaml;
 
 import static java.util.Collections.emptyList;
-import static java.util.logging.Level.INFO;
+import static java.util.logging.Level.FINE;
 
 import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration;
 import java.io.InputStream;
@@ -162,21 +162,19 @@ private static void failOnExtraKeys(Map<String, Object> yaml) {
    *
    * @param conf the metric configuration
    * @param is the InputStream with the YAML rules
-   * @param id identifier of the YAML ruleset, such as a filename
    * @throws IllegalArgumentException when unable to parse YAML
    */
-  public void addMetricDefsTo(MetricConfiguration conf, InputStream is, String id) {
+  public void addMetricDefsTo(MetricConfiguration conf, InputStream is) {
     try {
       JmxConfig config = loadConfig(is);
-      logger.log(INFO, "{0}: found {1} metric rules", new Object[] {id, config.getRules().size()});
+      logger.log(FINE, "found {1} metric rules", config.getRules().size());
       config.addMetricDefsTo(conf);
     } catch (Exception exception) {
       // It is essential that the parser exception is made visible to the user.
       // It contains contextual information about any syntax issues found by the parser.
       String msg =
           String.format(
-              "Failed to parse YAML rules from %s: %s %s",
-              id, rootCause(exception), exception.getMessage());
+              "Failed to parse YAML rules : %s %s", rootCause(exception), exception.getMessage());
       throw new IllegalArgumentException(msg, exception);
     }
   }

instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/JmxTelemetryTest.java

@@ -9,6 +9,7 @@
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import io.opentelemetry.api.OpenTelemetry;
+import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -25,30 +26,39 @@ void createDefault() {
   }
 
   @Test
-  void missingClasspathTarget() {
+  void throwsExceptionOnNullInput() {
     JmxTelemetryBuilder builder = JmxTelemetry.builder(OpenTelemetry.noop());
-    assertThatThrownBy(() -> builder.addClassPathRules("should-not-exist"))
+    assertThatThrownBy(() -> builder.addRules((InputStream) null))
+        .isInstanceOf(IllegalArgumentException.class);
+    assertThatThrownBy(() -> builder.addRules((Path) null))
         .isInstanceOf(IllegalArgumentException.class);
   }
 
   @Test
   void invalidClasspathTarget() {
     JmxTelemetryBuilder builder = JmxTelemetry.builder(OpenTelemetry.noop());
-    assertThatThrownBy(() -> builder.addClassPathRules("invalid"))
+    assertThatThrownBy(() -> addClasspathRules(builder, "jmx/rules/invalid.yaml"))
         .isInstanceOf(IllegalArgumentException.class);
   }
 
   @Test
-  void knownClassPathTarget() {
-    JmxTelemetry.builder(OpenTelemetry.noop()).addClassPathRules("jvm").build();
+  void knownValidYaml() {
+    JmxTelemetryBuilder jmxtelemetry = JmxTelemetry.builder(OpenTelemetry.noop());
+    addClasspathRules(jmxtelemetry, "jmx/rules/jvm.yaml");
+    assertThat(jmxtelemetry.build()).isNotNull();
+  }
+
+  private static void addClasspathRules(JmxTelemetryBuilder builder, String path) {
+    InputStream input = JmxTelemetryTest.class.getClassLoader().getResourceAsStream(path);
+    builder.addRules(input);
   }
 
   @Test
   void invalidExternalYaml(@TempDir Path dir) throws Exception {
     Path invalid = Files.createTempFile(dir, "invalid", ".yaml");
     Files.write(invalid, ":this !is /not YAML".getBytes(StandardCharsets.UTF_8));
     JmxTelemetryBuilder builder = JmxTelemetry.builder(OpenTelemetry.noop());
-    assertThatThrownBy(() -> builder.addCustomRules(invalid))
+    assertThatThrownBy(() -> builder.addRules(invalid))
         .isInstanceOf(IllegalArgumentException.class);
   }