post-review changes

Author: SylvainJuge

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

@@ -31,17 +31,17 @@ public void afterAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
     if (config.getBoolean("otel.jmx.enabled", true)) {
       JmxTelemetryBuilder jmx =
           JmxTelemetry.builder(GlobalOpenTelemetry.get())
-              .beanDiscoveryDelay(beanDiscoveryDelay(config).toMillis());
+              .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 (IllegalArgumentException e) {
+      } 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);
       }
 
-      jmx.build().startLocal();
+      jmx.build().start();
     }
   }
 

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

@@ -5,18 +5,22 @@
 
 package io.opentelemetry.instrumentation.jmx;
 
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.jmx.engine.JmxMetricInsight;
 import io.opentelemetry.instrumentation.jmx.engine.MetricConfiguration;
 import java.util.List;
 import java.util.function.Supplier;
 import javax.management.MBeanServerConnection;
+import javax.management.MBeanServerFactory;
 
+/** Entrypoint for JMX metrics Insights */
 public final class JmxTelemetry {
 
   private final JmxMetricInsight service;
   private final MetricConfiguration metricConfiguration;
 
+  /** Returns a new {@link JmxTelemetryBuilder} configured with the given {@link OpenTelemetry}. */
   public static JmxTelemetryBuilder builder(OpenTelemetry openTelemetry) {
     return new JmxTelemetryBuilder(openTelemetry);
   }
@@ -27,12 +31,25 @@ public static JmxTelemetryBuilder builder(OpenTelemetry openTelemetry) {
     this.metricConfiguration = metricConfiguration;
   }
 
-  @SuppressWarnings("unused") // used by jmx-scraper with remote connection
-  public void startRemote(Supplier<List<? extends MBeanServerConnection>> connections) {
-    service.startRemote(metricConfiguration, connections);
+  /**
+   * Starts JMX metrics collection on current JVM
+   *
+   * @return this
+   */
+  @CanIgnoreReturnValue
+  public JmxTelemetry start() {
+    return this.start(() -> MBeanServerFactory.findMBeanServer(null));
   }
 
-  public void startLocal() {
-    service.startLocal(metricConfiguration);
+  /**
+   * 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;
   }
 }

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

@@ -15,8 +15,10 @@
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.time.Duration;
 import java.util.logging.Logger;
 
+/** Builder for {@link JmxTelemetry} */
 public final class JmxTelemetryBuilder {
 
   private static final Logger logger = Logger.getLogger(JmxTelemetryBuilder.class.getName());
@@ -34,15 +36,15 @@ public final class JmxTelemetryBuilder {
   /**
    * Sets initial delay for MBean discovery
    *
-   * @param delayMs delay in milliseconds
+   * @param delay delay
    * @return builder instance
    */
   @CanIgnoreReturnValue
-  public JmxTelemetryBuilder beanDiscoveryDelay(long delayMs) {
-    if (delayMs < 0) {
-      throw new IllegalArgumentException("delay must be greater than 0");
+  public JmxTelemetryBuilder beanDiscoveryDelay(Duration delay) {
+    if (delay.isNegative()) {
+      throw new IllegalArgumentException("delay must be positive or zero");
     }
-    this.discoveryDelayMs = delayMs;
+    this.discoveryDelayMs = delay.toMillis();
     return this;
   }
 

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

@@ -58,7 +58,13 @@ public void startRemote(
     start(conf, connections);
   }
 
-  private void start(
+  /**
+   * Starts metric registration on the provided list of connections
+   *
+   * @param conf metric configuration
+   * @param connections supplier for list of connections (remote or local)
+   */
+  public void start(
       MetricConfiguration conf, Supplier<List<? extends MBeanServerConnection>> connections) {
     if (conf.isEmpty()) {
       logger.log(

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

@@ -12,6 +12,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.time.Duration;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -54,7 +55,7 @@ void invalidExternalYaml(@TempDir Path dir) throws Exception {
   @Test
   void invalidStartDelay() {
     JmxTelemetryBuilder builder = JmxTelemetry.builder(OpenTelemetry.noop());
-    assertThatThrownBy(() -> builder.beanDiscoveryDelay(-1))
+    assertThatThrownBy(() -> builder.beanDiscoveryDelay(Duration.ofMillis(-1)))
         .isInstanceOf(IllegalArgumentException.class);
   }
 }