Load InstrumentationModules using InstrumentationLoader (#2971)

* Load InstrumentationModules using InstrumentationLoader

* writing-instrumentation-module doc improvement

* spotless

docs/contributing/writing-instrumentation-module.md

@@ -22,7 +22,7 @@ the javaagent jar. The easiest way to do it is using `@AutoService`:
 
 ```java
 
-@AutoService(AgentExtension.class)
+@AutoService(InstrumentationModule.class)
 class MyLibraryInstrumentationModule extends InstrumentationModule {
   // ...
 }
@@ -42,6 +42,22 @@ public MyLibraryInstrumentationModule() {
 For detailed information on `InstrumentationModule` names please read the
 `InstrumentationModule#InstrumentationModule(String, String...)` Javadoc.
 
+### `order()`
+
+If you need to have instrumentations applied in a specific order (for example your custom
+instrumentation enriches the built-in servlet one and thus needs to run after it) you can override
+the `order()` method to specify the ordering:
+
+```java
+@Override
+public int order() {
+  return 1;
+}
+```
+
+Higher `order()` means that the instrumentation module will be applied later. The default value is
+`0`.
+
 ### `isHelperClass()`
 
 The OpenTelemetry javaagent picks up helper classes used in the instrumentation/advice classes and

javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationExtensionImplementation.java

@@ -1,30 +0,0 @@
-/*
- * Copyright The OpenTelemetry Authors
- * SPDX-License-Identifier: Apache-2.0
- */
-
-package io.opentelemetry.javaagent.extension.instrumentation;
-
-import java.util.Iterator;
-import java.util.ServiceLoader;
-import net.bytebuddy.agent.builder.AgentBuilder;
-
-abstract class InstrumentationExtensionImplementation {
-  private static final InstrumentationExtensionImplementation INSTANCE;
-
-  static {
-    Iterator<InstrumentationExtensionImplementation> impls =
-        ServiceLoader.load(InstrumentationExtensionImplementation.class).iterator();
-    if (!impls.hasNext()) {
-      throw new IllegalStateException("The application is running without OpenTelemetry javaagent");
-    }
-    INSTANCE = impls.next();
-  }
-
-  static InstrumentationExtensionImplementation get() {
-    return INSTANCE;
-  }
-
-  abstract AgentBuilder extend(
-      InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder);
-}

javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java

@@ -10,14 +10,12 @@ import static net.bytebuddy.matcher.ElementMatchers.any;
 
 import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.javaagent.extension.muzzle.Reference;
-import io.opentelemetry.javaagent.extension.spi.AgentExtension;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import net.bytebuddy.agent.builder.AgentBuilder;
 import net.bytebuddy.matcher.ElementMatcher;
 
 /**
@@ -28,10 +26,11 @@ import net.bytebuddy.matcher.ElementMatcher;
  * <p>Classes extending {@link InstrumentationModule} should be public and non-final so that it's
  * possible to extend and reuse them in vendor distributions.
  *
- * <p>WARNING: using {@link InstrumentationModule} as SPI is now deprecated; please use {@link
+ * <p>{@link InstrumentationModule} is an SPI, you need to ensure that a proper {@code
- * AgentExtension} instead.
+ * META-INF/services/} provider file is created for it to be picked up by the agent. See {@link
+ * java.util.ServiceLoader} for more details.
  */
-public abstract class InstrumentationModule implements AgentExtension {
+public abstract class InstrumentationModule {
   private static final String[] EMPTY = new String[0];
   private static final Reference[] EMPTY_REFS = new Reference[0];
 
@@ -85,18 +84,10 @@ public abstract class InstrumentationModule implements AgentExtension {
   }
 
   /**
-   * Add this instrumentation to an AgentBuilder.
+   * Returns the main instrumentation name. See {@link #InstrumentationModule(String, String...)}
-   *
+   * for more details about instrumentation names.
-   * @param parentAgentBuilder AgentBuilder to base instrumentation config off of.
-   * @return the original agentBuilder and this instrumentation
    */
-  @Override
+  public final String instrumentationName() {
-  public final AgentBuilder extend(AgentBuilder parentAgentBuilder) {
-    return InstrumentationExtensionImplementation.get().extend(this, parentAgentBuilder);
-  }
-
-  @Override
-  public final String extensionName() {
     return instrumentationNames.iterator().next();
   }
 
@@ -113,6 +104,15 @@ public abstract class InstrumentationModule implements AgentExtension {
     return DEFAULT_ENABLED;
   }
 
+  /**
+   * Returns the order of adding instrumentation modules to the javaagent. Higher values are added
+   * later, for example: an instrumentation module with order=1 will run after a module with
+   * order=0.
+   */
+  public int order() {
+    return 0;
+  }
+
   /**
    * Instrumentation modules can override this method to specify additional packages (or classes)
    * that should be treated as "library instrumentation" packages. Classes from those packages will

javaagent-extension-api/src/test/groovy/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModuleTest.groovy

@@ -7,7 +7,6 @@ package io.opentelemetry.javaagent.extension.instrumentation
 
 import io.opentelemetry.instrumentation.api.config.Config
 import io.opentelemetry.instrumentation.api.config.ConfigBuilder
-import net.bytebuddy.agent.builder.AgentBuilder
 import spock.lang.Specification
 
 class InstrumentationModuleTest extends Specification {
@@ -20,20 +19,14 @@ class InstrumentationModuleTest extends Specification {
   def "default enabled"() {
     setup:
     def target = new TestInstrumentationModule(["test"])
-    target.extend(new AgentBuilder.Default())
 
     expect:
     target.enabled
-    target.applyCalled
   }
 
   def "default enabled override"() {
-    setup:
-    target.extend(new AgentBuilder.Default())
-
     expect:
     target.enabled == enabled
-    target.applyCalled == enabled
 
     where:
     enabled | target
@@ -62,11 +55,9 @@ class InstrumentationModuleTest extends Specification {
         return false
       }
     }
-    target.extend(new AgentBuilder.Default())
 
     expect:
     target.enabled == enabled
-    target.applyCalled == enabled
 
     cleanup:
     Config.INSTANCE = null
@@ -76,15 +67,12 @@ class InstrumentationModuleTest extends Specification {
   }
 
   static class TestInstrumentationModule extends InstrumentationModule {
-    boolean applyCalled = false
-
     TestInstrumentationModule(List<String> instrumentationNames) {
       super(instrumentationNames)
     }
 
     @Override
     List<TypeInstrumentation> typeInstrumentations() {
-      applyCalled = true
       return []
     }
   }

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java

@@ -157,19 +157,25 @@ public class AgentInstaller {
               .with(new TransformLoggingListener());
     }
 
-    int numInstrumenters = 0;
+    int numberOfLoadedExtensions = 0;
-
     for (AgentExtension agentExtension : loadAgentExtensions()) {
-      log.debug("Loading extension {}", agentExtension.getClass().getName());
+      log.debug(
+          "Loading extension {} [class {}]",
+          agentExtension.extensionName(),
+          agentExtension.getClass().getName());
       try {
         agentBuilder = agentExtension.extend(agentBuilder);
-        numInstrumenters++;
+        numberOfLoadedExtensions++;
       } catch (Exception | LinkageError e) {
-        log.error("Unable to load extension {}", agentExtension.getClass().getName(), e);
+        log.error(
+            "Unable to load extension {} [class {}]",
+            agentExtension.extensionName(),
+            agentExtension.getClass().getName(),
+            e);
       }
     }
+    log.debug("Installed {} extension(s)", numberOfLoadedExtensions);
 
-    log.debug("Installed {} instrumenter(s)", numInstrumenters);
     ResettableClassFileTransformer resettableClassFileTransformer = agentBuilder.installOn(inst);
     installComponentsAfterByteBuddy(componentInstallers, config);
     return resettableClassFileTransformer;
@@ -234,16 +240,9 @@ public class AgentInstaller {
     return new NoopIgnoreMatcherProvider();
   }
 
-  private static List<? extends AgentExtension> loadAgentExtensions() {
+  private static List<AgentExtension> loadAgentExtensions() {
-    // TODO: InstrumentationModule should no longer be an SPI
+    return SafeServiceLoader.load(AgentExtension.class, AgentInstaller.class.getClassLoader())
-    Stream<? extends AgentExtension> extensions =
+        .stream()
-        Stream.concat(
-            SafeServiceLoader.load(
-                InstrumentationModule.class, AgentInstaller.class.getClassLoader())
-                .stream(),
-            SafeServiceLoader.load(AgentExtension.class, AgentInstaller.class.getClassLoader())
-                .stream());
-    return extensions
         .sorted(Comparator.comparingInt(AgentExtension::order))
         .collect(Collectors.toList());
   }

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/context/FieldBackedProvider.java

@@ -14,12 +14,12 @@ import static net.bytebuddy.matcher.ElementMatchers.not;
 import io.opentelemetry.instrumentation.api.caching.Cache;
 import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.javaagent.bootstrap.FieldBackedContextStoreAppliedMarker;
-import io.opentelemetry.javaagent.extension.instrumentation.ActualInstrumentationExtensionImplementation;
 import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
 import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
 import io.opentelemetry.javaagent.tooling.HelperInjector;
 import io.opentelemetry.javaagent.tooling.TransformSafeLogger;
 import io.opentelemetry.javaagent.tooling.Utils;
+import io.opentelemetry.javaagent.tooling.instrumentation.InstrumentationModuleInstaller;
 import java.lang.reflect.Method;
 import java.security.ProtectionDomain;
 import java.util.Arrays;
@@ -389,7 +389,7 @@ public class FieldBackedProvider implements InstrumentationContextProvider {
               builder
                   .type(not(isAbstract()).and(safeHasSuperType(named(entry.getKey()))))
                   .and(safeToInjectFieldsMatcher())
-                  .and(ActualInstrumentationExtensionImplementation.NOT_DECORATOR_MATCHER)
+                  .and(InstrumentationModuleInstaller.NOT_DECORATOR_MATCHER)
                   .transform(NoOpTransformer.INSTANCE);
 
           /*

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/ConstantAdjuster.java

@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.javaagent.extension.instrumentation;
+package io.opentelemetry.javaagent.tooling.instrumentation;
 
 import net.bytebuddy.agent.builder.AgentBuilder;
 import net.bytebuddy.asm.TypeConstantAdjustment;
@@ -12,8 +12,8 @@ import net.bytebuddy.dynamic.DynamicType;
 import net.bytebuddy.utility.JavaModule;
 
 /**
- * This {@link net.bytebuddy.agent.builder.AgentBuilder.Transformer} ensures that class files of a
+ * This {@link AgentBuilder.Transformer} ensures that class files of a version previous to Java 5 do
- * version previous to Java 5 do not store class entries in the generated class's constant pool.
+ * not store class entries in the generated class's constant pool.
  *
  * @see ConstantAdjuster The ASM visitor that does the actual work.
  */

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationLoader.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation;
+
+import com.google.auto.service.AutoService;
+import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
+import io.opentelemetry.javaagent.extension.spi.AgentExtension;
+import io.opentelemetry.javaagent.instrumentation.api.SafeServiceLoader;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Collectors;
+import net.bytebuddy.agent.builder.AgentBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@AutoService(AgentExtension.class)
+public class InstrumentationLoader implements AgentExtension {
+  private static final Logger log = LoggerFactory.getLogger(InstrumentationLoader.class);
+
+  private final InstrumentationModuleInstaller instrumentationModuleInstaller =
+      new InstrumentationModuleInstaller();
+
+  @Override
+  public AgentBuilder extend(AgentBuilder agentBuilder) {
+    List<InstrumentationModule> instrumentationModules =
+        SafeServiceLoader.load(
+                InstrumentationModule.class, InstrumentationLoader.class.getClassLoader())
+            .stream()
+            .sorted(Comparator.comparingInt(InstrumentationModule::order))
+            .collect(Collectors.toList());
+
+    int numberOfLoadedModules = 0;
+    for (InstrumentationModule instrumentationModule : instrumentationModules) {
+      log.debug(
+          "Loading instrumentation {} [class {}]",
+          instrumentationModule.instrumentationName(),
+          instrumentationModule.getClass().getName());
+      try {
+        agentBuilder = instrumentationModuleInstaller.install(instrumentationModule, agentBuilder);
+        numberOfLoadedModules++;
+      } catch (Exception | LinkageError e) {
+        log.error(
+            "Unable to load instrumentation {} [class {}]",
+            instrumentationModule.instrumentationName(),
+            instrumentationModule.getClass().getName(),
+            e);
+      }
+    }
+    log.debug("Installed {} instrumenter(s)", numberOfLoadedModules);
+
+    return agentBuilder;
+  }
+
+  @Override
+  public String extensionName() {
+    return "instrumentation-loader";
+  }
+}

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java

@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.javaagent.extension.instrumentation;
+package io.opentelemetry.javaagent.tooling.instrumentation;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.failSafe;
 import static java.util.Arrays.asList;
@@ -11,7 +11,8 @@ import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
-import com.google.auto.service.AutoService;
+import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.tooling.HelperInjector;
 import io.opentelemetry.javaagent.tooling.TransformSafeLogger;
 import io.opentelemetry.javaagent.tooling.Utils;
@@ -34,9 +35,7 @@ import net.bytebuddy.utility.JavaModule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-@AutoService(InstrumentationExtensionImplementation.class)
+public final class InstrumentationModuleInstaller {
-public final class ActualInstrumentationExtensionImplementation
-    extends InstrumentationExtensionImplementation {
   private static final TransformSafeLogger log =
       TransformSafeLogger.getLogger(InstrumentationModule.class);
   private static final Logger muzzleLog = LoggerFactory.getLogger("muzzleMatcher");
@@ -46,11 +45,10 @@ public final class ActualInstrumentationExtensionImplementation
   public static final ElementMatcher.Junction<AnnotationSource> NOT_DECORATOR_MATCHER =
       not(isAnnotatedWith(named("javax.decorator.Decorator")));
 
-  @Override
+  AgentBuilder install(
-  AgentBuilder extend(
       InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder) {
     if (!instrumentationModule.isEnabled()) {
-      log.debug("Instrumentation {} is disabled", instrumentationModule.extensionName());
+      log.debug("Instrumentation {} is disabled", instrumentationModule.instrumentationName());
       return parentAgentBuilder;
     }
     List<String> helperClassNames = asList(instrumentationModule.getMuzzleHelperClassNames());
@@ -60,7 +58,7 @@ public final class ActualInstrumentationExtensionImplementation
       if (!helperClassNames.isEmpty() || !helperResourceNames.isEmpty()) {
         log.warn(
             "Helper classes and resources won't be injected if no types are instrumented: {}",
-            instrumentationModule.extensionName());
+            instrumentationModule.instrumentationName());
       }
 
       return parentAgentBuilder;
@@ -71,7 +69,7 @@ public final class ActualInstrumentationExtensionImplementation
     MuzzleMatcher muzzleMatcher = new MuzzleMatcher(instrumentationModule, helperClassNames);
     AgentBuilder.Transformer helperInjector =
         new HelperInjector(
-            instrumentationModule.extensionName(), helperClassNames, helperResourceNames);
+            instrumentationModule.instrumentationName(), helperClassNames, helperResourceNames);
     InstrumentationContextProvider contextProvider =
         createInstrumentationContextProvider(instrumentationModule);
 
@@ -158,8 +156,8 @@ public final class ActualInstrumentationExtensionImplementation
       if (!isMatch) {
         if (muzzleLog.isWarnEnabled()) {
           muzzleLog.warn(
-              "Instrumentation skipped, mismatched references were found: {} -- {} on {}",
+              "Instrumentation skipped, mismatched references were found: {} [class {}] on {}",
-              instrumentationModule.extensionName(),
+              instrumentationModule.instrumentationName(),
               instrumentationModule.getClass().getName(),
               classLoader);
           List<Mismatch> mismatches = muzzle.getMismatchedReferenceSources(classLoader);
@@ -170,8 +168,8 @@ public final class ActualInstrumentationExtensionImplementation
       } else {
         if (log.isDebugEnabled()) {
           log.debug(
-              "Applying instrumentation: {} -- {} on {}",
+              "Applying instrumentation: {} [class {}] on {}",
-              instrumentationModule.extensionName(),
+              instrumentationModule.instrumentationName(),
               instrumentationModule.getClass().getName(),
               classLoader);
         }