throw exception in case of parsing error

Author: SylvainJuge

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

@@ -13,12 +13,17 @@
 import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
 import io.opentelemetry.sdk.autoconfigure.internal.AutoConfigureUtil;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
+import java.nio.file.Paths;
 import java.time.Duration;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /** An {@link AgentListener} that enables JMX metrics during agent startup. */
 @AutoService(AgentListener.class)
 public class JmxMetricInsightInstaller implements AgentListener {
 
+  private static final Logger logger = Logger.getLogger(JmxMetricInsightInstaller.class.getName());
+
   @Override
   public void afterAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
     ConfigProperties config = AutoConfigureUtil.getConfig(autoConfiguredSdk);
@@ -28,10 +33,13 @@ public void afterAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
           JmxTelemetry.builder(GlobalOpenTelemetry.get())
               .beanDiscoveryDelay(beanDiscoveryDelay(config).toMillis());
 
-      config.getList("otel.jmx.config").forEach(jmx::addCustomRules);
-      config
-          .getList("otel.jmx.target.system")
-          .forEach(system -> jmx.addClasspathRules(JmxMetricInsightInstaller.class, system));
+      try {
+        config.getList("otel.jmx.config").stream().map(Paths::get).forEach(jmx::addCustomRules);
+        config.getList("otel.jmx.target.system").forEach(jmx::addClasspathRules);
+      } catch (IllegalArgumentException e) {
+        // for now only log JMX errors as they do not prevent agent startup
+        logger.log(Level.SEVERE, "Error while loading JMX configuration", e);
+      }
 
       jmx.build().startLocal();
     }

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

@@ -13,7 +13,6 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.InputStream;
-import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
@@ -39,7 +38,7 @@ void testToVerifyExistingRulesAreValid() throws Exception {
     assertThat(parser).isNotNull();
 
     Path path = Paths.get(PATH_TO_ALL_EXISTING_RULES);
-    assertThat(Files.exists(path)).isTrue();
+    assertThat(path).isNotEmptyDirectory();
 
     File existingRulesDir = path.toFile();
     File[] existingRules = existingRulesDir.listFiles();

instrumentation/jmx-metrics/library/README.md

@@ -32,16 +32,21 @@ implementation("io.opentelemetry.instrumentation:opentelemetry-jmx-metrics:OPENT
 
 ```java
 import io.opentelemetry.api.OpenTelemetry;
-import io.opentelemetry.instrumentation.jmx.engine.JmxMetricInsight;
-import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration;
+import io.opentelemetry.instrumentation.jmx.JmxTelemetry;
+import io.opentelemetry.instrumentation.jmx.JmxTelemetryBuilder;
 
 // Get an OpenTelemetry instance
-OpenTelemetry openTelemetry = ...;
-
-JmxMetricInsight jmxMetricInsight = JmxMetricInsight.createService(openTelemetry, 5000);
-
-// Configure your JMX metrics
-MetricConfiguration config = new MetricConfiguration();
-
-jmxMetricInsight.startLocal(config);
+OpenTelemetry openTelemetry = ...
+
+JmxTelemetry jmxTelemetry = JmxTelemetry.builder(openTelemetry)
+  // Configure included metrics (optional)
+  .addClasspathRules("tomcat")
+  .addClasspathRules("jetty")
+  // Configure custom metrics (optional)
+  .addCustomRules("/path/to/custom-jmx.yaml")
+  // delay bean discovery by 5 seconds
+  .beanDiscoveryDelay(5000)
+  .build();
+
+jmxTelemetry.startLocal();
 ```

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

@@ -6,15 +6,15 @@
 package io.opentelemetry.instrumentation.jmx;
 
 import static java.util.logging.Level.FINE;
-import static java.util.logging.Level.INFO;
 
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration;
 import io.opentelemetry.instrumentation.jmx.yaml.RuleParser;
+import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.Files;
-import java.nio.file.Paths;
+import java.nio.file.Path;
 import java.util.logging.Logger;
 
 public class JmxTelemetryBuilder {
@@ -31,40 +31,57 @@ public class JmxTelemetryBuilder {
     this.metricConfiguration = new MetricConfiguration();
   }
 
+  /**
+   * Sets initial delay for MBean discovery
+   *
+   * @param delayMs delay in milliseconds
+   * @return builder instance
+   */
   @CanIgnoreReturnValue
   public JmxTelemetryBuilder beanDiscoveryDelay(long delayMs) {
     this.discoveryDelayMs = delayMs;
     return this;
   }
 
-  // TODO: can we use classloader as argument instead of baseClass?
+  /**
+   * 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
+   */
   @CanIgnoreReturnValue
-  public JmxTelemetryBuilder addClasspathRules(Class<?> baseClass, String targetId) {
-    String yamlResource = String.format("/jmx/rules/%s.yaml", targetId);
-    try (InputStream inputStream = baseClass.getResourceAsStream(yamlResource)) {
-      if (inputStream != null) {
-        logger.log(FINE, "Adding JMX config from classpath for {0}", yamlResource);
-        RuleParser parserInstance = RuleParser.get();
-        parserInstance.addMetricDefsTo(metricConfiguration, inputStream, targetId);
-      } else {
-        logger.log(INFO, "No support found for {0}", targetId);
+  public JmxTelemetryBuilder addClasspathRules(String target) {
+    String yamlResource = String.format("/jmx/rules/%s.yaml", target);
+    ClassLoader classLoader = JmxTelemetryBuilder.class.getClassLoader();
+    try (InputStream inputStream = classLoader.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) {
-      logger.warning(e.getMessage());
+      throw new IllegalArgumentException("Unable to load JMX rules from classpath: " + yamlResource, 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
+   */
   @CanIgnoreReturnValue
-  public JmxTelemetryBuilder addCustomRules(String path) {
+  public JmxTelemetryBuilder addCustomRules(Path path) {
     logger.log(FINE, "Adding JMX config from file: {0}", path);
     RuleParser parserInstance = RuleParser.get();
-    try (InputStream inputStream = Files.newInputStream(Paths.get(path))) {
-      parserInstance.addMetricDefsTo(metricConfiguration, inputStream, path);
-    } catch (Exception e) {
-      // yaml parsing errors are caught and logged inside of addMetricDefsTo
-      // only file access related exceptions are expected here
-      logger.warning(e.toString());
+    try (InputStream inputStream = Files.newInputStream(path)) {
+      parserInstance.addMetricDefsTo(metricConfiguration, inputStream, path.toString());
+    } catch (IOException e) {
+      throw new IllegalArgumentException("Unable to load JMX rules in path: " + path, e);
     }
     return this;
   }

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

@@ -7,7 +7,6 @@
 
 import static java.util.Collections.emptyList;
 import static java.util.logging.Level.INFO;
-import static java.util.logging.Level.WARNING;
 
 import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration;
 import java.io.InputStream;
@@ -164,20 +163,21 @@ 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) {
     try {
       JmxConfig config = loadConfig(is);
       logger.log(INFO, "{0}: found {1} metric rules", new Object[] {id, config.getRules().size()});
       config.addMetricDefsTo(conf);
     } catch (Exception exception) {
-      logger.log(
-          WARNING,
-          "Failed to parse YAML rules from {0}: {1}",
-          new Object[] {id, rootCause(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.
-      logger.log(WARNING, exception.toString());
+      String msg =
+          String.format(
+              "Failed to parse YAML rules from %s: %s %s",
+              id, rootCause(exception), exception.getMessage());
+      throw new IllegalArgumentException(msg, exception);
     }
   }