Iterate over InstrumenterModule services instead of Instrumenters

Author: mcculls

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/java/concurrent/ExcludeFilter.java

@@ -117,7 +117,7 @@ private static EnumSet<ExcludeType> exclude(Class<?> clazz) {
 
   /**
    * This should only be called during startup to initialize this based on information gathered from
-   * the {@code Instrumenter} instances.
+   * the loaded {@code InstrumenterModule}s.
    *
    * @param excludeTypes the types to exclude
    */

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

@@ -89,7 +89,7 @@ public static void installBytebuddyAgent(final Instrumentation inst) {
   }
 
   /**
-   * Install the core bytebuddy agent along with all implementations of {@link Instrumenter}.
+   * Install the core bytebuddy agent along with all registered {@link InstrumenterModule}s.
    *
    * @return the agent's class transformer
    */
@@ -167,24 +167,23 @@ public static ClassFileTransformer installBytebuddyAgent(
       agentBuilder = agentBuilder.with(listener);
     }
 
-    Instrumenters instrumenters = Instrumenters.load(AgentInstaller.class.getClassLoader());
-    int maxInstrumentationId = instrumenters.maxInstrumentationId();
+    InstrumenterModules modules = InstrumenterModules.load();
+    int maxInstrumentationId = modules.maxInstrumentationId();
 
     // pre-size state before registering instrumentations to reduce number of allocations
     InstrumenterState.setMaxInstrumentationId(maxInstrumentationId);
 
-    // This needs to be a separate loop through all the instrumenters before we start adding
+    // This needs to be a separate loop through all instrumentations before we start adding
     // advice so that we can exclude field injection, since that will try to check exclusion
     // immediately and we don't have the ability to express dependencies between different
-    // instrumenters to control the load order.
-    for (Instrumenter instrumenter : instrumenters) {
-      if (instrumenter instanceof ExcludeFilterProvider) {
-        ExcludeFilterProvider provider = (ExcludeFilterProvider) instrumenter;
+    // instrumentations to control the load order.
+    for (InstrumenterModule module : modules) {
+      if (module instanceof ExcludeFilterProvider) {
+        ExcludeFilterProvider provider = (ExcludeFilterProvider) module;
         ExcludeFilter.add(provider.excludedClasses());
         if (DEBUG) {
           log.debug(
-              "Adding filtered classes - instrumentation.class={}",
-              instrumenter.getClass().getName());
+              "Adding filtered classes - instrumentation.class={}", module.getClass().getName());
         }
       }
     }
@@ -193,23 +192,21 @@ public static ClassFileTransformer installBytebuddyAgent(
         new CombiningTransformerBuilder(agentBuilder, maxInstrumentationId);
 
     int installedCount = 0;
-    for (Instrumenter instrumenter : instrumenters) {
-      if (instrumenter instanceof InstrumenterModule
-          && !((InstrumenterModule) instrumenter).isApplicable(enabledSystems)) {
+    for (InstrumenterModule module : modules) {
+      if (!module.isApplicable(enabledSystems)) {
         if (DEBUG) {
-          log.debug("Not applicable - instrumentation.class={}", instrumenter.getClass().getName());
+          log.debug("Not applicable - instrumentation.class={}", module.getClass().getName());
         }
         continue;
       }
       if (DEBUG) {
-        log.debug("Loading - instrumentation.class={}", instrumenter.getClass().getName());
+        log.debug("Loading - instrumentation.class={}", module.getClass().getName());
       }
       try {
-        transformerBuilder.applyInstrumentation(instrumenter);
+        transformerBuilder.applyInstrumentation(module);
         installedCount++;
       } catch (Exception | LinkageError e) {
-        log.error(
-            "Failed to load - instrumentation.class={}", instrumenter.getClass().getName(), e);
+        log.error("Failed to load - instrumentation.class={}", module.getClass().getName(), e);
       }
     }
     if (DEBUG) {

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

@@ -74,17 +74,12 @@ public CombiningTransformerBuilder(AgentBuilder agentBuilder, int maxInstrumenta
     this.nextSupplementaryId = maxInstrumentationId + 1;
   }
 
-  public void applyInstrumentation(Instrumenter instrumenter) {
-    if (instrumenter instanceof InstrumenterModule) {
-      InstrumenterModule module = (InstrumenterModule) instrumenter;
-      if (module.isEnabled()) {
-        InstrumenterState.registerInstrumentation(module);
-        for (Instrumenter member : module.typeInstrumentations()) {
-          buildInstrumentation(module, member);
-        }
+  public void applyInstrumentation(InstrumenterModule module) {
+    if (module.isEnabled()) {
+      InstrumenterState.registerInstrumentation(module);
+      for (Instrumenter member : module.typeInstrumentations()) {
+        buildInstrumentation(module, member);
       }
-    } else {
-      throw new IllegalArgumentException("Unexpected Instrumenter type");
     }
   }
 
@@ -184,7 +179,7 @@ public void applyAdvice(Instrumenter.TransformingAdvice typeAdvice) {
   }
 
   @Override
-  public void applyAdvice(ElementMatcher<? super MethodDescription> matcher, String className) {
+  public void applyAdvice(ElementMatcher<? super MethodDescription> matcher, String adviceClass) {
     Advice.WithCustomMapping customMapping = Advice.withCustomMapping();
     if (postProcessor != null) {
       customMapping = customMapping.with(postProcessor);
@@ -193,7 +188,7 @@ public void applyAdvice(ElementMatcher<? super MethodDescription> matcher, Strin
         new AgentBuilder.Transformer.ForAdvice(customMapping)
             .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader())
             .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler())
-            .advice(not(ignoredMethods).and(matcher), className));
+            .advice(not(ignoredMethods).and(matcher), adviceClass));
   }
 
   /** Counts the number of distinct context store injections registered with this builder. */

dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/MuzzleMatcher.java

@@ -10,8 +10,8 @@
 public final class MuzzleMatcher implements AgentBuilder.RawMatcher {
   private final MuzzleCheck muzzleCheck;
 
-  public MuzzleMatcher(InstrumenterModule instrumenter) {
-    this.muzzleCheck = new MuzzleCheck(instrumenter);
+  public MuzzleMatcher(InstrumenterModule module) {
+    this.muzzleCheck = new MuzzleCheck(module);
   }
 
   @Override

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

@@ -37,10 +37,8 @@ public final class AgentCLI {
   /** Prints all known integrations in alphabetical order. */
   public static void printIntegrationNames() {
     Set<String> names = new TreeSet<>();
-    for (Instrumenter instrumenter : Instrumenters.load(Instrumenter.class.getClassLoader())) {
-      if (instrumenter instanceof InstrumenterModule) {
-        names.add(((InstrumenterModule) instrumenter).name());
-      }
+    for (InstrumenterModule module : InstrumenterModules.load()) {
+      names.add(module.name());
     }
     for (String name : names) {
       System.out.println(name);

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

@@ -6,9 +6,9 @@
 import java.util.Set;
 
 /**
- * Used to allow an {@link Instrumenter} to opt out of broad instrumentations like {@link Runnable}.
+ * Allows an {@link InstrumenterModule} to opt out of broad instrumentations like {@link Runnable}.
  *
- * <p>These are looked up in a separate pass before the {@link Instrumenter} is allowed to add
+ * <p>These are looked up in a separate pass before the {@link InstrumenterModule} is allowed to add
  * instrumentations, to be able to opt out of field injection which will need to check for
  * exclusions immediately.
  */

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

@@ -13,12 +13,7 @@
 import net.bytebuddy.matcher.ElementMatcher;
 import net.bytebuddy.utility.JavaModule;
 
-/**
- * Built-in bytebuddy-based instrumentation for the datadog javaagent.
- *
- * <p>It is strongly recommended to extend {@link InstrumenterModule} rather than implement this
- * interface directly.
- */
+/** Declares bytebuddy-based type instrumentation for the datadog javaagent. */
 public interface Instrumenter {
 
   /** Instrumentation that only matches a single named type. */
@@ -39,6 +34,9 @@ interface ForTypeHierarchy {
     ElementMatcher<TypeDescription> hierarchyMatcher();
   }
 
+  /** Instrumentation that transforms types on the bootstrap class-path. */
+  interface ForBootstrap {}
+
   /**
    * Instrumentation that matches a series of types configured at runtime. This is used for last
    * minute additions in the field such as testing a new JDBC driver that is not yet in the allowed
@@ -107,9 +105,7 @@ interface HasMethodAdvice extends Instrumenter {
     void methodAdvice(MethodTransformer transformer);
   }
 
-  /** Instrumentation that transforms types on the bootstrap class-path. */
-  interface ForBootstrap {}
-
+  /** Applies type advice from an instrumentation that {@link HasTypeAdvice}. */
   interface TypeTransformer {
     void applyAdvice(TransformingAdvice typeAdvice);
 
@@ -118,10 +114,12 @@ default void applyAdvice(AsmVisitorWrapper typeVisitor) {
     }
   }
 
+  /** Applies method advice from an instrumentation that {@link HasMethodAdvice}. */
   interface MethodTransformer {
-    void applyAdvice(ElementMatcher<? super MethodDescription> matcher, String className);
+    void applyAdvice(ElementMatcher<? super MethodDescription> matcher, String adviceClass);
   }
 
+  /** Contributes a transformation step to the dynamic type builder. */
   interface TransformingAdvice {
     DynamicType.Builder<?> transform(
         DynamicType.Builder<?> builder,

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

@@ -60,7 +60,7 @@ public enum TargetSystem {
   protected final String packageName = Strings.getPackageName(getClass().getName());
 
   public InstrumenterModule(final String instrumentationName, final String... additionalNames) {
-    instrumentationId = Instrumenters.currentInstrumentationId();
+    instrumentationId = InstrumenterModules.currentInstrumentationId();
     instrumentationNames = new ArrayList<>(1 + additionalNames.length);
     instrumentationNames.add(instrumentationName);
     addAll(instrumentationNames, additionalNames);

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenters.java renamed to dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/InstrumenterModules.java

@@ -13,64 +13,65 @@
 import org.slf4j.LoggerFactory;
 
 /**
- * Provides a stable sequence of {@link Instrumenters} registered as META-INF services. The id of
- * the {@link Instrumenter} currently being installed is available during iteration by calling
- * {@link #currentInstrumentationId()}.
+ * Provides a stable sequence of {@link InstrumenterModule}s registered as META-INF services. The id
+ * of the instrumentation currently being installed is available during iteration by calling {@link
+ * #currentInstrumentationId()}.
  *
- * <p>Note: it is expected that only one thread will iterate over instrumenters at a time.
+ * <p>Note: it is expected that only one thread will iterate over this sequence at a time.
  */
-public final class Instrumenters implements Iterable<Instrumenter> {
-  static final Logger log = LoggerFactory.getLogger(Instrumenters.class);
+public final class InstrumenterModules implements Iterable<InstrumenterModule> {
+  static final Logger log = LoggerFactory.getLogger(InstrumenterModules.class);
 
   final ClassLoader loader;
   final String[] names;
 
-  final Instrumenter[] instrumenters;
+  final InstrumenterModule[] modules;
   static int currentInstrumentationId;
 
-  public static Instrumenters load(ClassLoader loader) {
-    return new Instrumenters(loader, loadInstrumenterNames(loader));
+  public static InstrumenterModules load() {
+    ClassLoader loader = InstrumenterModule.class.getClassLoader();
+    return new InstrumenterModules(loader, loadModuleNames(loader));
   }
 
-  Instrumenters(ClassLoader loader, String[] names) {
+  InstrumenterModules(ClassLoader loader, String[] names) {
     this.loader = loader;
     this.names = names;
 
-    this.instrumenters = new Instrumenter[names.length];
+    this.modules = new InstrumenterModule[names.length];
   }
 
   public int maxInstrumentationId() {
-    return instrumenters.length;
+    return modules.length;
   }
 
-  /** Returns the id of the {@link Instrumenter} currently being installed. */
+  /** Returns the id of the instrumentation currently being installed. */
   public static int currentInstrumentationId() {
     return currentInstrumentationId;
   }
 
   @Override
-  public Iterator<Instrumenter> iterator() {
-    return new Iterator<Instrumenter>() {
+  public Iterator<InstrumenterModule> iterator() {
+    return new Iterator<InstrumenterModule>() {
       private int index = 0;
 
       @Override
       public boolean hasNext() {
-        while (index < instrumenters.length) {
-          if (null != instrumenters[index]) {
+        while (index < modules.length) {
+          if (null != modules[index]) {
             currentInstrumentationId = index;
             return true;
           }
           String nextName = names[index];
           if (null != nextName) {
             try {
-              currentInstrumentationId = index; // set before loading instrumenter
+              currentInstrumentationId = index; // set before loading the next module
               @SuppressWarnings({"rawtypes", "unchecked"})
-              Class<Instrumenter> nextType = (Class) loader.loadClass(nextName);
-              instrumenters[index] = nextType.getConstructor().newInstance();
+              Class<InstrumenterModule> nextType = (Class) loader.loadClass(nextName);
+              modules[index] = nextType.getConstructor().newInstance();
               return true;
             } catch (Throwable e) {
               log.error("Failed to load - instrumentation.class={}", nextName, e);
-              names[index] = null; // only attempt to load instrumenter once
+              names[index] = null; // only attempt to load each module once
             }
           }
           index++;
@@ -79,9 +80,9 @@ public boolean hasNext() {
       }
 
       @Override
-      public Instrumenter next() {
+      public InstrumenterModule next() {
         if (hasNext()) {
-          return instrumenters[index++];
+          return modules[index++];
         } else {
           throw new NoSuchElementException();
         }
@@ -94,11 +95,11 @@ public void remove() {
     };
   }
 
-  private static String[] loadInstrumenterNames(ClassLoader loader) {
+  private static String[] loadModuleNames(ClassLoader loader) {
     Set<String> lines = new LinkedHashSet<>();
     try {
       Enumeration<URL> urls =
-          loader.getResources("META-INF/services/datadog.trace.agent.tooling.Instrumenter");
+          loader.getResources("META-INF/services/datadog.trace.agent.tooling.InstrumenterModule");
       while (urls.hasMoreElements()) {
         try (BufferedReader reader =
             new BufferedReader(

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

@@ -8,7 +8,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/** Tracks {@link Instrumenter} state, such as where it was applied and where it was blocked. */
+/** Tracks instrumentation state, such as where it was applied and where it was blocked. */
 public final class InstrumenterState {
   private static final Logger log = LoggerFactory.getLogger(InstrumenterState.class);