Merge pull request #838 from DataDog/tyler/jmx-fetch

Enable JMXFetch by default and add configs for more standard custom metrics

dd-java-agent/agent-jmxfetch/agent-jmxfetch.gradle

@@ -4,7 +4,10 @@ plugins {
 apply from: "${rootDir}/gradle/java.gradle"
 
 dependencies {
-  compile 'com.datadoghq:jmxfetch:0.27.0'
+  compile('com.datadoghq:jmxfetch:0.29.0'){
+    exclude group: 'org.slf4j', module: 'slf4j-log4j12'
+    exclude group: 'log4j', module: 'log4j'
+  }
   compile deps.slf4j
   compile project(':dd-trace-api')
 }
@@ -32,12 +35,23 @@ tasks.register("submodulesUpdate", Exec) {
   group 'Build Setup'
   description 'Initializes and updates integrations-core git submodule'
   commandLine 'git', 'submodule', 'update', '--init', 'integrations-core'
+  def submoduleHead = file("${project.rootDir}/.git/modules/dd-java-agent/agent-jmxfetch/integrations-core/HEAD")
+  if (submoduleHead.exists()) {
+    inputs.file "${project.rootDir}/.git/modules/dd-java-agent/agent-jmxfetch/integrations-core/HEAD"
+  }
+  def integrationsCore = file("$projectDir/integrations-core")
+  outputs.dir integrationsCore
+  if (integrationsCore.list().length == 0) {
+    outputs.upToDateWhen { false }
+  }
 }
 
 tasks.register("copyMetricConfigs", Exec) {
   group 'Build Setup'
   description 'Copy metrics.yaml files from integrations-core into resources'
   commandLine './copy-metric-configs.sh', 'integrations-core', sourceSets.main.output.resourcesDir
+  inputs.dir file("$projectDir/integrations-core")
+  outputs.dir sourceSets.main.output.resourcesDir
   doFirst {
     // Ensure the resources directory is available.
     file(sourceSets.main.output.resourcesDir).mkdirs()

dd-java-agent/agent-jmxfetch/integrations-core

@@ -1 +1 @@
-Subproject commit 3e38b4e75edcee3ca84f022ea50240b0fc0537f2
+Subproject commit e6a01f9e885ac9b71c0ffec8c28dc75668570b15

dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java

@@ -1,5 +1,7 @@
 package datadog.trace.agent.jmxfetch;
 
+import static org.datadog.jmxfetch.AppConfig.ACTION_COLLECT;
+
 import com.google.common.collect.ImmutableList;
 import datadog.trace.api.Config;
 import java.io.IOException;
@@ -16,6 +18,7 @@ import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.io.IOUtils;
 import org.datadog.jmxfetch.App;
 import org.datadog.jmxfetch.AppConfig;
+import org.datadog.jmxfetch.reporter.ReporterFactory;
 
 @Slf4j
 public class JMXFetch {
@@ -38,6 +41,14 @@ public class JMXFetch {
       return;
     }
 
+    if (!log.isDebugEnabled()
+        && System.getProperty("org.slf4j.simpleLogger.log.org.datadog.jmxfetch") == null) {
+      // Reduce noisiness of jmxfetch logging.
+      System.setProperty("org.slf4j.simpleLogger.log.org.datadog.jmxfetch", "warn");
+    }
+
+    final String jmxFetchConfigDir = config.getJmxFetchConfigDir();
+    final List<String> jmxFetchConfigs = config.getJmxFetchConfigs();
     final List<String> internalMetricsConfigs = getInternalMetricFiles();
     final List<String> metricsConfigs = config.getJmxFetchMetricsConfigs();
     final Integer checkPeriod = config.getJmxFetchCheckPeriod();
@@ -48,18 +59,9 @@ public class JMXFetch {
     final String logLevel = getLogLevel();
 
     log.info(
-        "JMXFetch config: {} {} {} {} {} {} {} {}",
+        "JMXFetch config: {} {} {} {} {} {} {} {} {} {}",
-        internalMetricsConfigs,
+        jmxFetchConfigDir,
-        metricsConfigs,
+        jmxFetchConfigs,
-        checkPeriod,
-        refreshBeansPeriod,
-        globalTags,
-        reporter,
-        logLocation,
-        logLevel);
-    final AppConfig appConfig =
-        AppConfig.create(
-            DEFAULT_CONFIGS,
         internalMetricsConfigs,
         metricsConfigs,
         checkPeriod,
@@ -69,6 +71,24 @@ public class JMXFetch {
         logLocation,
         logLevel);
 
+    final AppConfig.AppConfigBuilder configBuilder =
+        AppConfig.builder()
+            .action(ImmutableList.of(ACTION_COLLECT))
+            .confdDirectory(jmxFetchConfigDir)
+            .yamlFileList(jmxFetchConfigs)
+            .targetDirectInstances(true)
+            .instanceConfigResources(DEFAULT_CONFIGS)
+            .metricConfigResources(internalMetricsConfigs)
+            .metricConfigFiles(metricsConfigs)
+            .refreshBeansPeriod(refreshBeansPeriod)
+            .globalTags(globalTags)
+            .reporter(ReporterFactory.getReporter(reporter));
+
+    if (checkPeriod != null) {
+      configBuilder.checkPeriod(checkPeriod);
+    }
+    final AppConfig appConfig = configBuilder.build();
+
     final Thread thread =
         new Thread(
             new Runnable() {
@@ -131,7 +151,7 @@ public class JMXFetch {
         for (final String config : split) {
           integrationName.clear();
           integrationName.add(config.replace(".yaml", ""));
-          if (Config.integrationEnabled(integrationName, false)) {
+          if (Config.jmxFetchIntegrationEnabled(integrationName, false)) {
             final URL resource = JMXFetch.class.getResource("metricconfigs/" + config);
             result.add(resource.getPath().split("\\.jar!/")[1]);
           }

dd-java-agent/agent-jmxfetch/src/main/resources/jmxfetch-config.yaml

@@ -3,6 +3,6 @@ init_config:
   new_gc_metrics: true
 
 instances:
-- jmx_url: service:jmx:local:///
+  - jvm_direct: true
-  conf:
+    name: dd-java-agent default
-    # Intentionally left empty for now
+    conf: [] # Intentionally left empty for now

dd-java-agent/dd-java-agent.gradle

@@ -107,9 +107,11 @@ dependencies {
 }
 
 tasks.withType(Test).configureEach {
-  jvmArgs "-Ddd.writer.type=LogWriter", "-Ddd.service.name=java-app"
+  jvmArgs "-Ddd.service.name=java-agent-tests"
-  jvmArgs "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug"
+  jvmArgs "-Ddd.writer.type=LoggingWriter"
-  jvmArgs "-Dorg.slf4j.simpleLogger.defaultLogLevel=debug"
+  // Multi-threaded logging seems to be causing deadlocks with Gradle's log capture.
+//  jvmArgs "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug"
+//  jvmArgs "-Dorg.slf4j.simpleLogger.defaultLogLevel=debug"
 
   doFirst {
     // Defining here to allow jacoco to be first on the command line.

dd-java-agent/src/test/groovy/datadog/trace/agent/JMXFetchTest.groovy

@@ -52,7 +52,7 @@ class JMXFetchTest extends Specification {
   def "test jmxfetch config"() {
     setup:
     names.each {
-      System.setProperty("dd.integration.${it}.enabled", "$enable")
+      System.setProperty("dd.jmxfetch.${it}.enabled", "$enable")
     }
     def classLoader = IntegrationTestUtils.getJmxFetchClassLoader()
     // Have to set this so JMXFetch knows where to find resources

dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java

@@ -17,6 +17,8 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.jar.Attributes;
 import java.util.jar.JarEntry;
 import java.util.jar.JarOutputStream;
@@ -201,7 +203,8 @@ public class IntegrationTestUtils {
     final ProcessBuilder processBuilder = new ProcessBuilder(commands.toArray(new String[0]));
     processBuilder.environment().putAll(envVars);
     final Process process = processBuilder.start();
-    final int result = process.waitFor();
+
+    waitFor(process, 30, TimeUnit.SECONDS);
 
     if (printOutputStreams) {
       final BufferedReader stdInput =
@@ -221,6 +224,25 @@ public class IntegrationTestUtils {
       }
       System.out.println("--- stderr end ---");
     }
-    return result;
+    return process.exitValue();
+  }
+
+  private static void waitFor(final Process process, final long timeout, final TimeUnit unit)
+      throws InterruptedException, TimeoutException {
+    final long startTime = System.nanoTime();
+    long rem = unit.toNanos(timeout);
+
+    do {
+      try {
+        process.exitValue();
+        return;
+      } catch (final IllegalThreadStateException ex) {
+        if (rem > 0) {
+          Thread.sleep(Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
+        }
+      }
+      rem = unit.toNanos(timeout) - (System.nanoTime() - startTime);
+    } while (rem > 0);
+    throw new TimeoutException();
   }
 }

dd-trace-api/src/main/java/datadog/trace/api/Config.java

@@ -65,6 +65,8 @@ public class Config {
   public static final String PROPAGATION_STYLE_INJECT = "propagation.style.inject";
 
   public static final String JMX_FETCH_ENABLED = "jmxfetch.enabled";
+  public static final String JMX_FETCH_CONFIG_DIR = "jmxfetch.config.dir";
+  public static final String JMX_FETCH_CONFIG = "jmxfetch.config";
   public static final String JMX_FETCH_METRICS_CONFIGS = "jmxfetch.metrics-configs";
   public static final String JMX_FETCH_CHECK_PERIOD = "jmxfetch.check-period";
   public static final String JMX_FETCH_REFRESH_BEANS_PERIOD = "jmxfetch.refresh-beans-period";
@@ -100,7 +102,7 @@ public class Config {
   private static final int DEFAULT_PARTIAL_FLUSH_MIN_SPANS = 1000;
   private static final String DEFAULT_PROPAGATION_STYLE_EXTRACT = PropagationStyle.DATADOG.name();
   private static final String DEFAULT_PROPAGATION_STYLE_INJECT = PropagationStyle.DATADOG.name();
-  private static final boolean DEFAULT_JMX_FETCH_ENABLED = false;
+  private static final boolean DEFAULT_JMX_FETCH_ENABLED = true;
 
   public static final int DEFAULT_JMX_FETCH_STATSD_PORT = 8125;
 
@@ -147,7 +149,9 @@ public class Config {
   @Getter private final Set<PropagationStyle> propagationStylesToInject;
 
   @Getter private final boolean jmxFetchEnabled;
-  @Getter private final List<String> jmxFetchMetricsConfigs;
+  @Getter private final String jmxFetchConfigDir;
+  @Getter private final List<String> jmxFetchConfigs;
+  @Deprecated @Getter private final List<String> jmxFetchMetricsConfigs;
   @Getter private final Integer jmxFetchCheckPeriod;
   @Getter private final Integer jmxFetchRefreshBeansPeriod;
   @Getter private final String jmxFetchStatsdHost;
@@ -220,6 +224,8 @@ public class Config {
 
     jmxFetchEnabled =
         getBooleanSettingFromEnvironment(JMX_FETCH_ENABLED, DEFAULT_JMX_FETCH_ENABLED);
+    jmxFetchConfigDir = getSettingFromEnvironment(JMX_FETCH_CONFIG_DIR, null);
+    jmxFetchConfigs = getListSettingFromEnvironment(JMX_FETCH_CONFIG, null);
     jmxFetchMetricsConfigs = getListSettingFromEnvironment(JMX_FETCH_METRICS_CONFIGS, null);
     jmxFetchCheckPeriod = getIntegerSettingFromEnvironment(JMX_FETCH_CHECK_PERIOD, null);
     jmxFetchRefreshBeansPeriod =
@@ -300,6 +306,8 @@ public class Config {
 
     jmxFetchEnabled =
         getPropertyBooleanValue(properties, JMX_FETCH_ENABLED, parent.jmxFetchEnabled);
+    jmxFetchConfigDir = properties.getProperty(JMX_FETCH_CONFIG_DIR, parent.jmxFetchConfigDir);
+    jmxFetchConfigs = getPropertyListValue(properties, JMX_FETCH_CONFIG, parent.jmxFetchConfigs);
     jmxFetchMetricsConfigs =
         getPropertyListValue(properties, JMX_FETCH_METRICS_CONFIGS, parent.jmxFetchMetricsConfigs);
     jmxFetchCheckPeriod =
@@ -326,7 +334,7 @@ public class Config {
     final Map<String, String> result = new HashMap<>(runtimeTags);
 
     if (reportHostName) {
-      String hostName = getHostName();
+      final String hostName = getHostName();
       if (null != hostName && !hostName.isEmpty()) {
         result.put(INTERNAL_HOST_NAME, hostName);
       }
@@ -391,6 +399,23 @@ public class Config {
     return anyEnabled;
   }
 
+  public static boolean jmxFetchIntegrationEnabled(
+      final SortedSet<String> integrationNames, final boolean defaultEnabled) {
+    // If default is enabled, we want to enable individually,
+    // if default is disabled, we want to disable individually.
+    boolean anyEnabled = defaultEnabled;
+    for (final String name : integrationNames) {
+      final boolean configEnabled =
+          getBooleanSettingFromEnvironment("jmxfetch." + name + ".enabled", defaultEnabled);
+      if (defaultEnabled) {
+        anyEnabled &= configEnabled;
+      } else {
+        anyEnabled |= configEnabled;
+      }
+    }
+    return anyEnabled;
+  }
+
   public static boolean traceAnalyticsIntegrationEnabled(
       final SortedSet<String> integrationNames, final boolean defaultEnabled) {
     // If default is enabled, we want to enable individually,
@@ -674,7 +699,7 @@ public class Config {
   private String getHostName() {
     try {
       return InetAddress.getLocalHost().getHostName();
-    } catch (UnknownHostException e) {
+    } catch (final UnknownHostException e) {
       // If we are not able to detect the hostname we do not throw an exception.
     }
 

dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy

@@ -35,8 +35,8 @@ import static datadog.trace.api.Config.SERVICE_MAPPING
 import static datadog.trace.api.Config.SERVICE_NAME
 import static datadog.trace.api.Config.SPAN_TAGS
 import static datadog.trace.api.Config.TRACE_AGENT_PORT
-import static datadog.trace.api.Config.TRACE_REPORT_HOSTNAME
 import static datadog.trace.api.Config.TRACE_ENABLED
+import static datadog.trace.api.Config.TRACE_REPORT_HOSTNAME
 import static datadog.trace.api.Config.TRACE_RESOLVER_ENABLED
 import static datadog.trace.api.Config.WRITER_TYPE
 
@@ -84,7 +84,7 @@ class ConfigTest extends Specification {
     config.runtimeContextFieldInjection == true
     config.propagationStylesToExtract.toList() == [Config.PropagationStyle.DATADOG]
     config.propagationStylesToInject.toList() == [Config.PropagationStyle.DATADOG]
-    config.jmxFetchEnabled == false
+    config.jmxFetchEnabled == true
     config.jmxFetchMetricsConfigs == []
     config.jmxFetchCheckPeriod == null
     config.jmxFetchRefreshBeansPeriod == null
@@ -125,7 +125,7 @@ class ConfigTest extends Specification {
     prop.setProperty(RUNTIME_CONTEXT_FIELD_INJECTION, "false")
     prop.setProperty(PROPAGATION_STYLE_EXTRACT, "Datadog, B3")
     prop.setProperty(PROPAGATION_STYLE_INJECT, "B3, Datadog")
-    prop.setProperty(JMX_FETCH_ENABLED, "true")
+    prop.setProperty(JMX_FETCH_ENABLED, "false")
     prop.setProperty(JMX_FETCH_METRICS_CONFIGS, "/foo.yaml,/bar.yaml")
     prop.setProperty(JMX_FETCH_CHECK_PERIOD, "100")
     prop.setProperty(JMX_FETCH_REFRESH_BEANS_PERIOD, "200")
@@ -156,7 +156,7 @@ class ConfigTest extends Specification {
     config.runtimeContextFieldInjection == false
     config.propagationStylesToExtract.toList() == [Config.PropagationStyle.DATADOG, Config.PropagationStyle.B3]
     config.propagationStylesToInject.toList() == [Config.PropagationStyle.B3, Config.PropagationStyle.DATADOG]
-    config.jmxFetchEnabled == true
+    config.jmxFetchEnabled == false
     config.jmxFetchMetricsConfigs == ["/foo.yaml", "/bar.yaml"]
     config.jmxFetchCheckPeriod == 100
     config.jmxFetchRefreshBeansPeriod == 200
@@ -188,7 +188,7 @@ class ConfigTest extends Specification {
     System.setProperty(PREFIX + RUNTIME_CONTEXT_FIELD_INJECTION, "false")
     System.setProperty(PREFIX + PROPAGATION_STYLE_EXTRACT, "Datadog, B3")
     System.setProperty(PREFIX + PROPAGATION_STYLE_INJECT, "B3, Datadog")
-    System.setProperty(PREFIX + JMX_FETCH_ENABLED, "true")
+    System.setProperty(PREFIX + JMX_FETCH_ENABLED, "false")
     System.setProperty(PREFIX + JMX_FETCH_METRICS_CONFIGS, "/foo.yaml,/bar.yaml")
     System.setProperty(PREFIX + JMX_FETCH_CHECK_PERIOD, "100")
     System.setProperty(PREFIX + JMX_FETCH_REFRESH_BEANS_PERIOD, "200")
@@ -219,7 +219,7 @@ class ConfigTest extends Specification {
     config.runtimeContextFieldInjection == false
     config.propagationStylesToExtract.toList() == [Config.PropagationStyle.DATADOG, Config.PropagationStyle.B3]
     config.propagationStylesToInject.toList() == [Config.PropagationStyle.B3, Config.PropagationStyle.DATADOG]
-    config.jmxFetchEnabled == true
+    config.jmxFetchEnabled == false
     config.jmxFetchMetricsConfigs == ["/foo.yaml", "/bar.yaml"]
     config.jmxFetchCheckPeriod == 100
     config.jmxFetchRefreshBeansPeriod == 200
@@ -478,6 +478,40 @@ class ConfigTest extends Specification {
     integrationNames = new TreeSet<>(names)
   }
 
+  def "verify integration jmxfetch config"() {
+    setup:
+    environmentVariables.set("DD_JMXFETCH_ORDER_ENABLED", "false")
+    environmentVariables.set("DD_JMXFETCH_TEST_ENV_ENABLED", "true")
+    environmentVariables.set("DD_JMXFETCH_DISABLED_ENV_ENABLED", "false")
+
+    System.setProperty("dd.jmxfetch.order.enabled", "true")
+    System.setProperty("dd.jmxfetch.test-prop.enabled", "true")
+    System.setProperty("dd.jmxfetch.disabled-prop.enabled", "false")
+
+    expect:
+    Config.jmxFetchIntegrationEnabled(integrationNames, defaultEnabled) == expected
+
+    where:
+    names                          | defaultEnabled | expected
+    []                             | true           | true
+    []                             | false          | false
+    ["invalid"]                    | true           | true
+    ["invalid"]                    | false          | false
+    ["test-prop"]                  | false          | true
+    ["test-env"]                   | false          | true
+    ["disabled-prop"]              | true           | false
+    ["disabled-env"]               | true           | false
+    ["other", "test-prop"]         | false          | true
+    ["other", "test-env"]          | false          | true
+    ["order"]                      | false          | true
+    ["test-prop", "disabled-prop"] | false          | true
+    ["disabled-env", "test-env"]   | false          | true
+    ["test-prop", "disabled-prop"] | true           | false
+    ["disabled-env", "test-env"]   | true           | false
+
+    integrationNames = new TreeSet<>(names)
+  }
+
   def "verify integration trace analytics config"() {
     setup:
     environmentVariables.set("DD_ORDER_ANALYTICS_ENABLED", "false")

gradle/java.gradle

@@ -1,3 +1,5 @@
+import java.time.Duration
+
 apply plugin: 'java'
 apply plugin: 'groovy'
 
@@ -301,8 +303,11 @@ for (def env : System.getenv().entrySet()) {
   }
 }
 
-// Disable all tests if skipTests property was specified
 tasks.withType(Test).configureEach {
+  // All tests must complete within 2 minutes.
+  timeout = Duration.ofMinutes(2)
+  
+  // Disable all tests if skipTests property was specified
   onlyIf { !project.rootProject.hasProperty("skipTests") }
 }