Merge pull request #696 from DataDog/tyler/jmxfetch-config

JMXFetch bundled integrations disabled by default

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

@@ -117,8 +117,11 @@ public class JMXFetch {
         final String[] split = configs.split("\n");
         final List<String> result = new ArrayList<>(split.length);
         for (final String config : split) {
-          final URL resource = JMXFetch.class.getResource("metricconfigs/" + config);
+          if (Config.integrationEnabled(
-          result.add(resource.getPath().split("\\.jar!/")[1]);
+              Collections.singleton(config.replace(".yaml", "")), false)) {
+            final URL resource = JMXFetch.class.getResource("metricconfigs/" + config);
+            result.add(resource.getPath().split("\\.jar!/")[1]);
+          }
         }
         return result;
       }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java

@@ -1,13 +1,13 @@
 package datadog.trace.agent.tooling;
 
 import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.failSafe;
-import static datadog.trace.agent.tooling.Utils.getConfigEnabled;
 import static net.bytebuddy.matcher.ElementMatchers.any;
 
 import datadog.trace.agent.tooling.context.FieldBackedProvider;
 import datadog.trace.agent.tooling.context.InstrumentationContextProvider;
 import datadog.trace.agent.tooling.muzzle.Reference;
 import datadog.trace.agent.tooling.muzzle.ReferenceMatcher;
+import datadog.trace.api.Config;
 import java.security.ProtectionDomain;
 import java.util.Arrays;
 import java.util.Collections;
@@ -52,20 +52,7 @@ public interface Instrumenter {
       instrumentationNames.add(instrumentationName);
       instrumentationPrimaryName = instrumentationName;
 
-      // If default is enabled, we want to enable individually,
+      enabled = Config.integrationEnabled(instrumentationNames, defaultEnabled());
-      // if default is disabled, we want to disable individually.
-      final boolean defaultEnabled = defaultEnabled();
-      boolean anyEnabled = defaultEnabled;
-      for (final String name : instrumentationNames) {
-        final boolean configEnabled =
-            getConfigEnabled("dd.integration." + name + ".enabled", defaultEnabled);
-        if (defaultEnabled) {
-          anyEnabled &= configEnabled;
-        } else {
-          anyEnabled |= configEnabled;
-        }
-      }
-      enabled = anyEnabled;
       contextProvider = new FieldBackedProvider(this);
     }
 
@@ -225,7 +212,7 @@ public interface Instrumenter {
     }
 
     protected boolean defaultEnabled() {
-      return getConfigEnabled("dd.integrations.enabled", true);
+      return Config.getBooleanSettingFromEnvironment("integrations.enabled", true);
     }
   }
 }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java

@@ -84,12 +84,5 @@ public class Utils {
     return type.getDeclaredMethods().filter(named(methodName)).getOnly();
   }
 
-  static boolean getConfigEnabled(final String name, final boolean fallback) {
-    final String property =
-        System.getProperty(
-            name, System.getenv(name.toUpperCase().replaceAll("[^a-zA-Z0-9_]", "_")));
-    return property == null ? fallback : Boolean.parseBoolean(property);
-  }
-
   private Utils() {}
 }

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

@@ -49,4 +49,36 @@ class JMXFetchTest extends Specification {
     Thread.currentThread().setContextClassLoader(currentContextLoader)
   }
 
+  def "test jmxfetch config"() {
+    setup:
+    names.each {
+      System.setProperty("dd.integration.${it}.enabled", "$enable")
+    }
+    def classLoader = IntegrationTestUtils.getJmxFetchClassLoader()
+    // Have to set this so JMXFetch knows where to find resources
+    Thread.currentThread().setContextClassLoader(classLoader)
+    final Class<?> jmxFetchAgentClass =
+      classLoader.loadClass("datadog.trace.agent.jmxfetch.JMXFetch")
+    final Method jmxFetchInstallerMethod = jmxFetchAgentClass.getDeclaredMethod("getInternalMetricFiles")
+    jmxFetchInstallerMethod.setAccessible(true)
+
+    expect:
+    jmxFetchInstallerMethod.invoke(null).sort() == result.sort()
+
+    cleanup:
+    names.each {
+      System.clearProperty("dd.integration.${it}.enabled")
+    }
+
+    where:
+    names               | enable | result
+    []                  | true   | []
+    ["tomcat"]          | false  | []
+    ["tomcat"]          | true   | ["datadog/trace/agent/jmxfetch/metricconfigs/tomcat.yaml"]
+    ["kafka"]           | true   | ["datadog/trace/agent/jmxfetch/metricconfigs/kafka.yaml"]
+    ["tomcat", "kafka"] | true   | ["datadog/trace/agent/jmxfetch/metricconfigs/tomcat.yaml", "datadog/trace/agent/jmxfetch/metricconfigs/kafka.yaml"]
+    ["tomcat", "kafka"] | false  | []
+    ["invalid"]         | true   | []
+  }
+
 }

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

@@ -6,7 +6,9 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 import java.util.UUID;
+import java.util.regex.Pattern;
 import lombok.Getter;
 import lombok.ToString;
 import lombok.extern.slf4j.Slf4j;
@@ -26,6 +28,8 @@ public class Config {
   /** Config keys below */
   private static final String PREFIX = "dd.";
 
+  private static final Pattern ENV_REPLACEMENT = Pattern.compile("[^a-zA-Z0-9_]");
+
   private static final Config INSTANCE = new Config();
 
   public static final String SERVICE_NAME = "service.name";
@@ -256,6 +260,23 @@ public class Config {
     return Collections.unmodifiableMap(result);
   }
 
+  public static boolean integrationEnabled(
+      final Set<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("integration." + name + ".enabled", defaultEnabled);
+      if (defaultEnabled) {
+        anyEnabled &= configEnabled;
+      } else {
+        anyEnabled |= configEnabled;
+      }
+    }
+    return anyEnabled;
+  }
+
   /**
    * Helper method that takes the name, adds a "dd." prefix then checks for System Properties of
    * that name. If none found, the name is converted to an Environment Variable and used to check
@@ -303,7 +324,7 @@ public class Config {
   }
 
   private static String propertyToEnvironmentName(final String name) {
-    return name.toUpperCase().replace(".", "_").replace("-", "_");
+    return ENV_REPLACEMENT.matcher(name.toUpperCase()).replaceAll("_");
   }
 
   private static Map<String, String> getPropertyMapValue(

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

@@ -281,6 +281,40 @@ class ConfigTest extends Specification {
     config.writerType == "DDAgentWriter"
   }
 
+  def "verify integration config"() {
+    setup:
+    environmentVariables.set("DD_INTEGRATION_ORDER_ENABLED", "false")
+    environmentVariables.set("DD_INTEGRATION_TEST_ENV_ENABLED", "true")
+    environmentVariables.set("DD_INTEGRATION_DISABLED_ENV_ENABLED", "false")
+
+    System.setProperty("dd.integration.order.enabled", "true")
+    System.setProperty("dd.integration.test-prop.enabled", "true")
+    System.setProperty("dd.integration.disabled-prop.enabled", "false")
+
+    expect:
+    Config.integrationEnabled(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 = names.toSet()
+  }
+
   def "verify mapping configs on tracer"() {
     setup:
     System.setProperty(PREFIX + SERVICE_MAPPING, mapString)