Load InstrumentationModules using InstrumentationLoader (#2971)

* Load InstrumentationModules using InstrumentationLoader * writing-instrumentation-module doc improvement * spotless
open-telemetry · May 13, 2021 · a72a783 · a72a783
1 parent a3be8e3
commit a72a783
Show file tree

Hide file tree

Showing 9 changed files with 126 additions and 94 deletions.
diff --git a/docs/contributing/writing-instrumentation-module.md b/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

diff --git a/...telemetry/javaagent/extension/instrumentation/InstrumentationExtensionImplementation.java b/...telemetry/javaagent/extension/instrumentation/InstrumentationExtensionImplementation.java
diff --git a/...main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java b/...main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java
@@ -10,14 +10,12 @@
 
 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 @@
  * <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
- * AgentExtension} instead.
+ * <p>{@link InstrumentationModule} is an SPI, you need to ensure that a proper {@code
+ * 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 @@ private static List<String> toList(String first, String[] rest) {
   }
 
   /**
-   * Add this instrumentation to an AgentBuilder.
-   *
-   * @param parentAgentBuilder AgentBuilder to base instrumentation config off of.
-   * @return the original agentBuilder and this instrumentation
+   * Returns the main instrumentation name. See {@link #InstrumentationModule(String, String...)}
+   * for more details about instrumentation names.
    */
-  @Override
-  public final AgentBuilder extend(AgentBuilder parentAgentBuilder) {
-    return InstrumentationExtensionImplementation.get().extend(this, parentAgentBuilder);
-  }
-
-  @Override
-  public final String extensionName() {
+  public final String instrumentationName() {
     return instrumentationNames.iterator().next();
   }
 
@@ -113,6 +104,15 @@ protected boolean defaultEnabled() {
     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

diff --git a/...entation/InstrumentationModuleTest.groovy → ...entation/InstrumentationModuleTest.groovy b/...entation/InstrumentationModuleTest.groovy → ...entation/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 []
     }
   }

diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java
@@ -157,19 +157,25 @@ public static ResettableClassFileTransformer installBytebuddyAgent(
               .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 @@ private static IgnoreMatcherProvider loadIgnoreMatcherProvider() {
     return new NoopIgnoreMatcherProvider();
   }
 
-  private static List<? extends AgentExtension> loadAgentExtensions() {
-    // TODO: InstrumentationModule should no longer be an SPI
-    Stream<? extends AgentExtension> extensions =
-        Stream.concat(
-            SafeServiceLoader.load(
-                InstrumentationModule.class, AgentInstaller.class.getClassLoader())
-                .stream(),
-            SafeServiceLoader.load(AgentExtension.class, AgentInstaller.class.getClassLoader())
-                .stream());
-    return extensions
+  private static List<AgentExtension> loadAgentExtensions() {
+    return SafeServiceLoader.load(AgentExtension.class, AgentInstaller.class.getClassLoader())
+        .stream()
         .sorted(Comparator.comparingInt(AgentExtension::order))
         .collect(Collectors.toList());
   }

diff --git a/...tooling/src/main/java/io/opentelemetry/javaagent/tooling/context/FieldBackedProvider.java b/...tooling/src/main/java/io/opentelemetry/javaagent/tooling/context/FieldBackedProvider.java
@@ -14,12 +14,12 @@
 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 AgentBuilder.Identified.Extendable additionalInstrumentation(
               builder
                   .type(not(isAbstract()).and(safeHasSuperType(named(entry.getKey()))))
                   .and(safeToInjectFieldsMatcher())
-                  .and(ActualInstrumentationExtensionImplementation.NOT_DECORATOR_MATCHER)
+                  .and(InstrumentationModuleInstaller.NOT_DECORATOR_MATCHER)
                   .transform(NoOpTransformer.INSTANCE);
 
           /*

diff --git a/...ion/instrumentation/ConstantAdjuster.java → ...ing/instrumentation/ConstantAdjuster.java b/...ion/instrumentation/ConstantAdjuster.java → ...ing/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.utility.JavaModule;
 
 /**
- * This {@link net.bytebuddy.agent.builder.AgentBuilder.Transformer} ensures that class files of a
- * version previous to Java 5 do not store class entries in the generated class's constant pool.
+ * This {@link AgentBuilder.Transformer} ensures that class files of a version previous to Java 5 do
+ * not store class entries in the generated class's constant pool.
  *
  * @see ConstantAdjuster The ASM visitor that does the actual work.
  */

diff --git a/...c/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationLoader.java b/...c/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";
+  }
+}