Merge pull request #7 from robsunday/jmx-scraper-impl

Code review followup changes - added default value for OTLP endpoint

Author: SylvainJuge

jmx-scraper/src/main/java/io/opentelemetry/contrib/jmxscraper/config/JmxScraperConfig.java

@@ -34,6 +34,8 @@ public class JmxScraperConfig {
   static final String JMX_REMOTE_PROFILE = "otel.jmx.remote.profile";
   static final String JMX_REALM = "otel.jmx.realm";
 
+  static final String OTLP_METRICS_EXPORTER = "otlp";
+
   static final List<String> AVAILABLE_TARGET_SYSTEMS =
       Collections.unmodifiableList(
           Arrays.asList(
@@ -148,8 +150,10 @@ public static JmxScraperConfig fromProperties(
 
     config.metricsExporterType =
         getAndSetPropertyIfUndefined(properties, METRICS_EXPORTER_TYPE, "logging");
-    config.otlpExporterEndpoint = properties.getProperty(OTLP_ENDPOINT);
-
+    if (OTLP_METRICS_EXPORTER.equalsIgnoreCase(config.metricsExporterType)) {
+      config.otlpExporterEndpoint =
+          getAndSetPropertyIfUndefined(properties, OTLP_ENDPOINT, "http://localhost:4318");
+    }
     config.username = properties.getProperty(JMX_USERNAME);
     config.password = properties.getProperty(JMX_PASSWORD);
 
@@ -232,9 +236,8 @@ private static void validateConfig(JmxScraperConfig config) throws Configuration
               "%s must specify targets from %s", config.targetSystems, AVAILABLE_TARGET_SYSTEMS));
     }
 
-    if (isBlank(config.otlpExporterEndpoint)
-        && (!isBlank(config.metricsExporterType)
-            && config.metricsExporterType.equalsIgnoreCase("otlp"))) {
+    if (OTLP_METRICS_EXPORTER.equalsIgnoreCase(config.metricsExporterType)
+        && isBlank(config.otlpExporterEndpoint)) {
       throw new ConfigurationException(OTLP_ENDPOINT + " must be specified for otlp format.");
     }
 

jmx-scraper/src/test/java/io/opentelemetry/contrib/jmxscraper/config/JmxScraperConfigTest.java

@@ -37,7 +37,7 @@ static void setUp() {
         SERVICE_URL, "jservice:jmx:rmi:///jndi/rmi://localhost:9010/jmxrmi");
     validProperties.setProperty(CUSTOM_JMX_SCRAPING_CONFIG, "");
     validProperties.setProperty(TARGET_SYSTEM, "tomcat, activemq");
-    validProperties.setProperty(METRICS_EXPORTER_TYPE, "otel");
+    validProperties.setProperty(METRICS_EXPORTER_TYPE, "otlp");
     validProperties.setProperty(INTERVAL_MILLISECONDS, "1410");
     validProperties.setProperty(REGISTRY_SSL, "true");
     validProperties.setProperty(OTLP_ENDPOINT, "http://localhost:4317");
@@ -77,13 +77,29 @@ void shouldCreateMinimalValidConfiguration() throws ConfigurationException {
     assertThat(config.getTargetSystems()).isEmpty();
     assertThat(config.getIntervalMilliseconds()).isEqualTo(10000);
     assertThat(config.getMetricsExporterType()).isEqualTo("logging");
-    assertThat(config.getOtlpExporterEndpoint()).isNull();
+    assertThat(config.getOtlpExporterEndpoint()).isBlank();
     assertThat(config.getUsername()).isNull();
     assertThat(config.getPassword()).isNull();
     assertThat(config.getRemoteProfile()).isNull();
     assertThat(config.getRealm()).isNull();
   }
 
+  @Test
+  void shouldCreateConfig_defaultOtlEndpoint() throws ConfigurationException {
+    // Given
+    Properties properties = new Properties();
+    properties.setProperty(SERVICE_URL, "jservice:jmx:rmi:///jndi/rmi://localhost:9010/jmxrmi");
+    properties.setProperty(CUSTOM_JMX_SCRAPING_CONFIG, "/file.properties");
+    properties.setProperty(METRICS_EXPORTER_TYPE, "otlp");
+
+    // When
+    JmxScraperConfig config = fromProperties(properties, new Properties());
+
+    // Then
+    assertThat(config.getMetricsExporterType()).isEqualTo("otlp");
+    assertThat(config.getOtlpExporterEndpoint()).isEqualTo("http://localhost:4318");
+  }
+
   @Test
   @ClearSystemProperty(key = "javax.net.ssl.keyStore")
   @ClearSystemProperty(key = "javax.net.ssl.keyStorePassword")
@@ -116,7 +132,7 @@ void shouldUseValuesFromProperties() throws ConfigurationException {
     assertThat(config.getCustomJmxScrapingConfigPath()).isEqualTo("");
     assertThat(config.getTargetSystems()).containsOnly("tomcat", "activemq");
     assertThat(config.getIntervalMilliseconds()).isEqualTo(1410);
-    assertThat(config.getMetricsExporterType()).isEqualTo("otel");
+    assertThat(config.getMetricsExporterType()).isEqualTo("otlp");
     assertThat(config.getOtlpExporterEndpoint()).isEqualTo("http://localhost:4317");
     assertThat(config.getUsername()).isEqualTo("some-user");
     assertThat(config.getPassword()).isEqualTo("some-password");
@@ -196,10 +212,10 @@ void shouldFailValidation_invalidTargetSystem() {
   }
 
   @Test
-  void shouldFailValidation_missingOtlpEndpoint() {
+  void shouldFailValidation_blankOtlpEndpointProvided() {
     // Given
     Properties properties = (Properties) validProperties.clone();
-    properties.remove(OTLP_ENDPOINT);
+    properties.setProperty(OTLP_ENDPOINT, "");
     properties.setProperty(METRICS_EXPORTER_TYPE, "otlp");
 
     // When and Then