Cleanup InstrumenterModule enablement

* All instrumentations now declare a target system which they fall under
* We introduce COMMON as the system for instrumentations shared across all
  systems and SECURITY for instrumentations shared between APPSEC and IAST
* We now log when an instrumentation has been disabled
* Special enablement of security instrumentations shared between APPSEC and
  IAST has been moved to `isEnabled` which is now told the enabled systems

Author: mcculls

dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java

@@ -200,12 +200,18 @@ public static ClassFileTransformer installBytebuddyAgent(
 
     int installedCount = 0;
     for (InstrumenterModule module : instrumenterModules) {
-      if (!module.isApplicable(enabledSystems)) {
+      if (!enabledSystems.contains(module.targetSystem())) {
         if (DEBUG) {
           log.debug("Not applicable - instrumentation.class={}", module.getClass().getName());
         }
         continue;
       }
+      if (!module.isEnabled(enabledSystems)) {
+        if (DEBUG) {
+          log.debug("Not enabled - instrumentation.class={}", module.getClass().getName());
+        }
+        continue;
+      }
       if (DEBUG) {
         log.debug("Loading - instrumentation.class={}", module.getClass().getName());
       }
@@ -291,7 +297,7 @@ public InstrumenterModule next() {
 
   public static Set<InstrumenterModule.TargetSystem> getEnabledSystems() {
     EnumSet<InstrumenterModule.TargetSystem> enabledSystems =
-        EnumSet.noneOf(InstrumenterModule.TargetSystem.class);
+        EnumSet.of(InstrumenterModule.TargetSystem.COMMON);
     InstrumenterConfig cfg = InstrumenterConfig.get();
     if (cfg.isTraceEnabled()) {
       enabledSystems.add(InstrumenterModule.TargetSystem.TRACING);
@@ -300,9 +306,11 @@ public static Set<InstrumenterModule.TargetSystem> getEnabledSystems() {
       enabledSystems.add(InstrumenterModule.TargetSystem.PROFILING);
     }
     if (cfg.getAppSecActivation() != ProductActivation.FULLY_DISABLED) {
+      enabledSystems.add(InstrumenterModule.TargetSystem.SECURITY);
       enabledSystems.add(InstrumenterModule.TargetSystem.APPSEC);
     }
     if (cfg.getIastActivation() != ProductActivation.FULLY_DISABLED) {
+      enabledSystems.add(InstrumenterModule.TargetSystem.SECURITY);
       enabledSystems.add(InstrumenterModule.TargetSystem.IAST);
     }
     if (cfg.isCiVisibilityEnabled()) {

dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/CombiningTransformerBuilder.java

@@ -93,17 +93,15 @@ public CombiningTransformerBuilder(
 
   /** Builds matchers and transformers for an instrumentation module and its members. */
   public void applyInstrumentation(InstrumenterModule module) {
-    if (module.isEnabled()) {
-      int instrumentationId = instrumenterIndex.instrumentationId(module);
-      if (instrumentationId < 0) {
-        // this is a non-indexed instrumentation configured at runtime
-        instrumentationId = nextRuntimeInstrumentationId++;
-      }
-      InstrumenterState.registerInstrumentation(module, instrumentationId);
-      prepareInstrumentation(module, instrumentationId);
-      for (Instrumenter member : module.typeInstrumentations()) {
-        buildTypeInstrumentation(member);
-      }
+    int instrumentationId = instrumenterIndex.instrumentationId(module);
+    if (instrumentationId < 0) {
+      // this is a non-indexed instrumentation configured at runtime
+      instrumentationId = nextRuntimeInstrumentationId++;
+    }
+    InstrumenterState.registerInstrumentation(module, instrumentationId);
+    prepareInstrumentation(module, instrumentationId);
+    for (Instrumenter member : module.typeInstrumentations()) {
+      buildTypeInstrumentation(member);
     }
   }
 

dd-java-agent/agent-tooling/src/jmh/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteBenchmarkInstrumentation.java

@@ -18,18 +18,18 @@ public CallSiteBenchmarkInstrumentation() {
   }
 
   @Override
-  public ElementMatcher<TypeDescription> callerType() {
-    return CallSiteMatcher.INSTANCE;
+  public TargetSystem targetSystem() {
+    return TargetSystem.COMMON; // always active during this benchmark
   }
 
   @Override
-  public boolean isEnabled() {
-    return "callSite".equals(System.getProperty("dd.benchmark.instrumentation", ""));
+  public ElementMatcher<TypeDescription> callerType() {
+    return CallSiteMatcher.INSTANCE;
   }
 
   @Override
-  public boolean isApplicable(final Set<TargetSystem> enabledSystems) {
-    return true;
+  public boolean isEnabled(Set<TargetSystem> enabledSystems) {
+    return "callSite".equals(System.getProperty("dd.benchmark.instrumentation", ""));
   }
 
   @Override

dd-java-agent/agent-tooling/src/jmh/java/datadog/trace/agent/tooling/bytebuddy/csi/CalleeBenchmarkInstrumentation.java

@@ -34,13 +34,13 @@ public String[] helperClassNames() {
   }
 
   @Override
-  public boolean isEnabled() {
-    return "callee".equals(System.getProperty("dd.benchmark.instrumentation", ""));
+  public TargetSystem targetSystem() {
+    return TargetSystem.COMMON; // always active during this benchmark
   }
 
   @Override
-  public boolean isApplicable(final Set<TargetSystem> enabledSystems) {
-    return true;
+  public boolean isEnabled(Set<TargetSystem> enabledSystems) {
+    return "callee".equals(System.getProperty("dd.benchmark.instrumentation", ""));
   }
 
   public static class Muzzle extends ReferenceMatcher {}

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

@@ -32,20 +32,13 @@ public abstract class InstrumenterModule implements Instrumenter {
   /**
    * Since several systems share the same instrumentation infrastructure in order to enable only the
    * applicable {@link Instrumenter instrumenters} on startup each {@linkplain InstrumenterModule}
-   * must declare its target system. The systems currently supported include:
-   *
-   * <ul>
-   *   <li>{@link TargetSystem#TRACING tracing}
-   *   <li>{@link TargetSystem#PROFILING profiling}
-   *   <li>{@link TargetSystem#APPSEC appsec}
-   *   <li>{@link TargetSystem#IAST iast}
-   *   <li>{@link TargetSystem#CIVISIBILITY ci-visibility}
-   *   <li>{@link TargetSystem#USM usm}
-   * </ul>
+   * must declare its target system.
    */
   public enum TargetSystem {
+    COMMON, // instrumentation common to every system
     TRACING,
     PROFILING,
+    SECURITY, // instrumentation shared between APPSEC and IAST
     APPSEC,
     IAST,
     CIVISIBILITY,
@@ -177,25 +170,16 @@ public Map<String, String> contextStore() {
     return emptyMap();
   }
 
+  public abstract TargetSystem targetSystem();
+
   protected boolean defaultEnabled() {
     return InstrumenterConfig.get().isIntegrationsEnabled();
   }
 
-  public boolean isEnabled() {
+  public boolean isEnabled(Set<TargetSystem> enabledSystems) {
     return enabled;
   }
 
-  /**
-   * Indicates the applicability of an {@linkplain InstrumenterModule} to the given system.<br>
-   *
-   * @param enabledSystems a set of all the enabled target systems
-   * @return {@literal true} if the set of enabled systems contains all the ones required by this
-   *     particular {@linkplain InstrumenterModule}
-   */
-  public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-    return false;
-  }
-
   protected final boolean isShortcutMatchingEnabled(boolean defaultToShortcut) {
     return InstrumenterConfig.get()
         .isIntegrationShortcutMatchingEnabled(singletonList(name()), defaultToShortcut);
@@ -208,8 +192,8 @@ public Tracing(String instrumentationName, String... additionalNames) {
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-      return enabledSystems.contains(TargetSystem.TRACING);
+    public TargetSystem targetSystem() {
+      return TargetSystem.TRACING;
     }
   }
 
@@ -220,13 +204,13 @@ public Profiling(String instrumentationName, String... additionalNames) {
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-      return enabledSystems.contains(TargetSystem.PROFILING);
+    public TargetSystem targetSystem() {
+      return TargetSystem.PROFILING;
     }
 
     @Override
-    public boolean isEnabled() {
-      return super.isEnabled()
+    public boolean isEnabled(Set<TargetSystem> enabledSystems) {
+      return super.isEnabled(enabledSystems)
           && !ConfigProvider.getInstance()
               .getBoolean(ProfilingConfig.PROFILING_ULTRA_MINIMAL, false);
     }
@@ -239,8 +223,8 @@ public AppSec(String instrumentationName, String... additionalNames) {
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-      return enabledSystems.contains(TargetSystem.APPSEC);
+    public TargetSystem targetSystem() {
+      return TargetSystem.APPSEC;
     }
   }
 
@@ -251,14 +235,22 @@ public Iast(String instrumentationName, String... additionalNames) {
       super(instrumentationName, additionalNames);
     }
 
+    @Override
+    public TargetSystem targetSystem() {
+      return TargetSystem.SECURITY;
+    }
+
     @Override
     public List<Instrumenter> typeInstrumentations() {
       preloadClassNames();
       return super.typeInstrumentations();
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
+    public boolean isEnabled(Set<TargetSystem> enabledSystems) {
+      if (!super.isEnabled(enabledSystems)) {
+        return false;
+      }
       if (enabledSystems.contains(TargetSystem.IAST)) {
         return true;
       }
@@ -307,8 +299,8 @@ public Usm(String instrumentationName, String... additionalNames) {
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-      return enabledSystems.contains(TargetSystem.USM);
+    public TargetSystem targetSystem() {
+      return TargetSystem.USM;
     }
   }
 
@@ -319,8 +311,8 @@ public CiVisibility(String instrumentationName, String... additionalNames) {
     }
 
     @Override
-    public boolean isApplicable(Set<TargetSystem> enabledSystems) {
-      return enabledSystems.contains(TargetSystem.CIVISIBILITY);
+    public TargetSystem targetSystem() {
+      return TargetSystem.CIVISIBILITY;
     }
   }
 }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteInstrumentation.java

@@ -21,6 +21,11 @@ public CallSiteInstrumentation(
     super(name, additionalNames);
   }
 
+  @Override
+  public TargetSystem targetSystem() {
+    return TargetSystem.SECURITY;
+  }
+
   @Override
   public void typeAdvice(TypeTransformer transformer) {
     transformer.applyAdvice(new CallSiteTransformer(name(), advices()));

dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/InstrumenterIndexTest.groovy

@@ -6,7 +6,12 @@ import spock.lang.Shared
 class InstrumenterIndexTest extends DDSpecification {
 
   @Shared
-  def unknownInstrumentation = new InstrumenterModule('unknown') {}
+  def unknownInstrumentation = new InstrumenterModule('unknown') {
+    @Override
+    InstrumenterModule.TargetSystem targetSystem() {
+      return InstrumenterModule.TargetSystem.COMMON
+    }
+  }
 
   @Shared
   def unknownTransformation = new Instrumenter() {}

dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/TestIndexFirstModule.java

@@ -7,6 +7,11 @@ public TestIndexFirstModule() {
     super("test-index-priority");
   }
 
+  @Override
+  public TargetSystem targetSystem() {
+    return TargetSystem.COMMON;
+  }
+
   @Override
   public int order() {
     return -100; // lower-values applied first

dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/TestIndexLastModule.java

@@ -7,6 +7,11 @@ public TestIndexLastModule() {
     super("test-index-priority");
   }
 
+  @Override
+  public TargetSystem targetSystem() {
+    return TargetSystem.COMMON;
+  }
+
   @Override
   public int order() {
     return 100; // higher-values applied last

dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/TestIndexMultiModule.java

@@ -11,6 +11,11 @@ public TestIndexMultiModule() {
     super("test-index-multi");
   }
 
+  @Override
+  public TargetSystem targetSystem() {
+    return TargetSystem.COMMON;
+  }
+
   @Override
   public List<Instrumenter> typeInstrumentations() {
     return asList(