post-review: JmxRemoteClient > JmxConnectorBuilder

open-telemetry · Sep 25, 2024 · ffc05c1 · ffc05c1
1 parent 7a7fed7
commit ffc05c1
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 37 deletions.
diff --git a/...mxscraper/client/JmxRemoteClientTest.java → ...b/jmxscraper/JmxConnectorBuilderTest.java b/...mxscraper/client/JmxRemoteClientTest.java → ...b/jmxscraper/JmxConnectorBuilderTest.java
@@ -3,12 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.contrib.jmxscraper.client;
+package io.opentelemetry.contrib.jmxscraper;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-import io.opentelemetry.contrib.jmxscraper.TestApp;
-import io.opentelemetry.contrib.jmxscraper.TestAppContainer;
 import java.io.IOException;
 import javax.management.ObjectName;
 import javax.management.remote.JMXConnector;
@@ -17,7 +15,7 @@
 import org.junit.jupiter.api.Test;
 import org.testcontainers.containers.Network;
 
-public class JmxRemoteClientTest {
+public class JmxConnectorBuilderTest {
 
   private static Network network;
 
@@ -36,7 +34,7 @@ void noAuth() {
     try (TestAppContainer app = new TestAppContainer().withNetwork(network).withJmxPort(9990)) {
       app.start();
       testConnector(
-          () -> JmxRemoteClient.createNew(app.getHost(), app.getMappedPort(9990)).connect());
+          () -> JmxConnectorBuilder.createNew(app.getHost(), app.getMappedPort(9990)).build());
     }
   }
 
@@ -49,9 +47,9 @@ void loginPwdAuth() {
       app.start();
       testConnector(
           () ->
-              JmxRemoteClient.createNew(app.getHost(), app.getMappedPort(9999))
+              JmxConnectorBuilder.createNew(app.getHost(), app.getMappedPort(9999))
                   .userCredentials(login, pwd)
-                  .connect());
+                  .build());
     }
   }
 

diff --git a/.../java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java b/.../java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java
@@ -12,7 +12,7 @@
 import com.linecorp.armeria.testing.junit5.server.ServerExtension;
 import io.grpc.stub.StreamObserver;
 import io.opentelemetry.contrib.jmxscraper.JmxScraperContainer;
-import io.opentelemetry.contrib.jmxscraper.client.JmxRemoteClient;
+import io.opentelemetry.contrib.jmxscraper.JmxConnectorBuilder;
 import io.opentelemetry.proto.collector.metrics.v1.ExportMetricsServiceRequest;
 import io.opentelemetry.proto.collector.metrics.v1.ExportMetricsServiceResponse;
 import io.opentelemetry.proto.collector.metrics.v1.MetricsServiceGrpc;
@@ -106,7 +106,7 @@ void endToEndTest() {
 
     // TODO : wait for metrics to be sent and add assertions on what is being captured
     // for now we just test that we can connect to remote JMX using our client.
-    try (JMXConnector connector = JmxRemoteClient.createNew(targetHost, targetPort).connect()) {
+    try (JMXConnector connector = JmxConnectorBuilder.createNew(targetHost, targetPort).build()) {
       assertThat(connector.getMBeanServerConnection()).isNotNull();
     } catch (IOException e) {
       throw new RuntimeException(e);

diff --git a/...ib/jmxscraper/client/JmxRemoteClient.java → ...ntrib/jmxscraper/JmxConnectorBuilder.java b/...ib/jmxscraper/client/JmxRemoteClient.java → ...ntrib/jmxscraper/JmxConnectorBuilder.java
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.contrib.jmxscraper.client;
+package io.opentelemetry.contrib.jmxscraper;
 
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.io.IOException;
@@ -26,9 +26,9 @@
 import javax.security.auth.callback.UnsupportedCallbackException;
 import javax.security.sasl.RealmCallback;
 
-public class JmxRemoteClient {
+public class JmxConnectorBuilder {
 
-  private static final Logger logger = Logger.getLogger(JmxRemoteClient.class.getName());
+  private static final Logger logger = Logger.getLogger(JmxConnectorBuilder.class.getName());
 
   private final JMXServiceURL url;
   @Nullable private String userName;
@@ -37,44 +37,65 @@ public class JmxRemoteClient {
   @Nullable private String realm;
   private boolean sslRegistry;
 
-  private JmxRemoteClient(JMXServiceURL url) {
+  private JmxConnectorBuilder(JMXServiceURL url) {
     this.url = url;
   }
 
-  public static JmxRemoteClient createNew(String host, int port) {
-    return new JmxRemoteClient(buildUrl(host, port));
+  public static JmxConnectorBuilder createNew(String host, int port) {
+    return new JmxConnectorBuilder(buildUrl(host, port));
   }
 
-  public static JmxRemoteClient createNew(String url) {
-    return new JmxRemoteClient(buildUrl(url));
+  public static JmxConnectorBuilder createNew(String url) {
+    return new JmxConnectorBuilder(buildUrl(url));
   }
 
   @CanIgnoreReturnValue
-  public JmxRemoteClient userCredentials(String userName, String password) {
+  public JmxConnectorBuilder userCredentials(String userName, String password) {
     this.userName = userName;
     this.password = password;
     return this;
   }
 
   @CanIgnoreReturnValue
-  public JmxRemoteClient withRemoteProfile(String profile) {
+  public JmxConnectorBuilder withRemoteProfile(String profile) {
     this.profile = profile;
     return this;
   }
 
   @CanIgnoreReturnValue
-  public JmxRemoteClient withRealm(String realm) {
+  public JmxConnectorBuilder withRealm(String realm) {
     this.realm = realm;
     return this;
   }
 
   @CanIgnoreReturnValue
-  public JmxRemoteClient withSslRegistry() {
+  public JmxConnectorBuilder withSslRegistry() {
     this.sslRegistry = true;
     return this;
   }
 
-  public JMXConnector connect() throws IOException {
+  /**
+   * Builds JMX connector instance by connecting to the remote JMX endpoint
+   *
+   * @return JMX connector
+   * @throws IOException in case of communication error
+   */
+  public JMXConnector build() throws IOException {
+    Map<String, Object> env = buildEnv();
+
+    try {
+      if (sslRegistry) {
+        return doConnectSslRegistry(url, env);
+      }
+
+      return doConnect(url, env);
+
+    } catch (IOException e) {
+      throw new IOException("Unable to connect to " + url.getHost() + ":" + url.getPort(), e);
+    }
+  }
+
+  private Map<String, Object> buildEnv() {
     Map<String, Object> env = new HashMap<>();
     if (userName != null && password != null) {
       env.put(JMXConnector.CREDENTIALS, new String[] {userName, password});
@@ -111,16 +132,7 @@ public JMXConnector connect() throws IOException {
     } catch (ReflectiveOperationException e) {
       logger.log(Level.WARNING, "SASL unsupported in current environment: " + e.getMessage());
     }
-
-    try {
-      if (sslRegistry) {
-        return doConnectSslRegistry(url, env);
-      } else {
-        return doConnect(url, env);
-      }
-    } catch (IOException e) {
-      throw new IOException("Unable to connect to " + url.getHost() + ":" + url.getPort(), e);
-    }
+    return env;
   }
 
   @SuppressWarnings("BanJNDI")

diff --git a/jmx-scraper/src/main/java/io/opentelemetry/contrib/jmxscraper/JmxScraper.java b/jmx-scraper/src/main/java/io/opentelemetry/contrib/jmxscraper/JmxScraper.java
@@ -5,7 +5,6 @@
 
 package io.opentelemetry.contrib.jmxscraper;
 
-import io.opentelemetry.contrib.jmxscraper.client.JmxRemoteClient;
 import io.opentelemetry.contrib.jmxscraper.config.ConfigurationException;
 import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfig;
 import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfigFactory;
@@ -25,7 +24,7 @@ public class JmxScraper {
   private static final Logger logger = Logger.getLogger(JmxScraper.class.getName());
   private static final String CONFIG_ARG = "-config";
 
-  private final JmxRemoteClient client;
+  private final JmxConnectorBuilder client;
 
   // TODO depend on instrumentation 2.9.0 snapshot
   // private final JmxMetricInsight service;
@@ -41,7 +40,7 @@ public static void main(String[] args) {
       JmxScraperConfig config = JmxScraper.createConfigFromArgs(Arrays.asList(args));
       // TODO: depend on instrumentation 2.9.0 snapshot
       // service = JmxMetricInsight.createService(GlobalOpenTelemetry.get(), config.getIntervalMilliseconds());
-      JmxScraper jmxScraper = new JmxScraper(JmxRemoteClient.createNew(config.getServiceUrl()));
+      JmxScraper jmxScraper = new JmxScraper(JmxConnectorBuilder.createNew(config.getServiceUrl()));
       jmxScraper.start();
 
     } catch (ArgumentsParsingException e) {
@@ -109,13 +108,13 @@ private static Properties loadPropertiesFromPath(String path)
     }
   }
 
-  JmxScraper(JmxRemoteClient client) {
+  JmxScraper(JmxConnectorBuilder client) {
     this.client = client;
   }
 
   private void start() throws IOException {
 
-    JMXConnector connector = client.connect();
+    JMXConnector connector = client.build();
 
     @SuppressWarnings("unused")
     MBeanServerConnection connection = connector.getMBeanServerConnection();