Make more tests run with indy (open-telemetry#9729)

Abhishekkr3003 · Nov 7, 2023 · 5d9ab15 · 5d9ab15
1 parent 398c691
commit 5d9ab15
Show file tree

Hide file tree

Showing 13 changed files with 102 additions and 48 deletions.
diff --git a/...pentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseInstrumentationModule.java b/...pentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseInstrumentationModule.java
@@ -6,14 +6,18 @@
 package io.opentelemetry.javaagent.instrumentation.couchbase.v2_0;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
-public class CouchbaseInstrumentationModule extends InstrumentationModule {
+public class CouchbaseInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
+
   public CouchbaseInstrumentationModule() {
     super("couchbase", "couchbase-2.0");
   }
@@ -24,13 +28,12 @@ public boolean isHelperClass(String className) {
   }
 
   @Override
-  public boolean isIndyModule() {
-    // rx.__OpenTelemetryTracingUtil is used for accessing a package private field
-    return false;
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return asList(new CouchbaseBucketInstrumentation(), new CouchbaseClusterInstrumentation());
   }
 
   @Override
-  public List<TypeInstrumentation> typeInstrumentations() {
-    return asList(new CouchbaseBucketInstrumentation(), new CouchbaseClusterInstrumentation());
+  public List<String> injectedClassNames() {
+    return singletonList("rx.__OpenTelemetryTracingUtil");
   }
 }
diff --git a/...java/io/opentelemetry/javaagent/instrumentation/hystrix/HystrixInstrumentationModule.java b/...java/io/opentelemetry/javaagent/instrumentation/hystrix/HystrixInstrumentationModule.java
@@ -10,10 +10,12 @@
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
-public class HystrixInstrumentationModule extends InstrumentationModule {
+public class HystrixInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
 
   public HystrixInstrumentationModule() {
     super("hystrix", "hystrix-1.4");
@@ -25,13 +27,12 @@ public boolean isHelperClass(String className) {
   }
 
   @Override
-  public boolean isIndyModule() {
-    // rx.__OpenTelemetryTracingUtil is used for accessing a package private field
-    return false;
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return singletonList(new HystrixCommandInstrumentation());
   }
 
   @Override
-  public List<TypeInstrumentation> typeInstrumentations() {
-    return singletonList(new HystrixCommandInstrumentation());
+  public List<String> injectedClassNames() {
+    return singletonList("rx.__OpenTelemetryTracingUtil");
   }
 }
diff --git a/.../java/io/opentelemetry/javaagent/instrumentation/ktor/v2_0/KtorInstrumentationModule.java b/.../java/io/opentelemetry/javaagent/instrumentation/ktor/v2_0/KtorInstrumentationModule.java
@@ -24,14 +24,6 @@ public boolean isHelperClass(String className) {
     return className.startsWith("io.opentelemetry.extension.kotlin.");
   }
 
-  @Override
-  public boolean isIndyModule() {
-    // java.lang.LinkageError: loader constraint violation in interface itable initialization for
-    // class
-    // io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion: when selecting method 'java.lang.Object io.ktor.client.plugins.HttpClientPlugin.prepare(kotlin.jvm.functions.Function1)' the class loader 'app' for super interface io.ktor.client.plugins.HttpClientPlugin, and the class loader io.opentelemetry.javaagent.tooling.instrumentation.indy.InstrumentationModuleClassLoader @2565a7d0 of the selected method's class, io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion have different Class objects for the type kotlin.jvm.functions.Function1 used in the signature (io.ktor.client.plugins.HttpClientPlugin is in unnamed module of loader 'app'; io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion is in unnamed module of loader io.opentelemetry.javaagent.tooling.instrumentation.indy.InstrumentationModuleClassLoader @2565a7d0, parent loader io.opentelemetry.javaagent.bootstrap.AgentClassLoader @ea30797)
-    return false;
-  }
-
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(new ServerInstrumentation(), new HttpClientInstrumentation());

diff --git a/.../io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3InstrumentationModule.java b/.../io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3InstrumentationModule.java
@@ -19,13 +19,6 @@ public OkHttp3InstrumentationModule() {
     super("okhttp", "okhttp-3.0");
   }
 
-  @Override
-  public boolean isIndyModule() {
-    // java.lang.LinkageError: bad method type alias: (Builder,Map)void not visible from class
-    // io.opentelemetry.javaagent.instrumentation.okhttp.v3_0.OkHttp3Instrumentation$ConstructorAdvice
-    return false;
-  }
-
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(new OkHttp3Instrumentation(), new OkHttp3DispatcherInstrumentation());

diff --git a/...lemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientInstrumentationModule.java b/...lemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientInstrumentationModule.java
@@ -6,14 +6,17 @@
 package io.opentelemetry.javaagent.instrumentation.vertx.v4_0.sql;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
-public class VertxSqlClientInstrumentationModule extends InstrumentationModule {
+public class VertxSqlClientInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
 
   public VertxSqlClientInstrumentationModule() {
     super("vertx-sql-client", "vertx-sql-client-4.0", "vertx");
@@ -25,9 +28,8 @@ public boolean isHelperClass(String className) {
   }
 
   @Override
-  public boolean isIndyModule() {
-    // QueryExecutorUtil accesses package private class io.vertx.sqlclient.impl.QueryExecutor
-    return false;
+  public List<String> injectedClassNames() {
+    return singletonList("io.vertx.sqlclient.impl.QueryExecutorUtil");
   }
 
   @Override

diff --git a/...metry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java b/...metry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java
@@ -5,8 +5,11 @@
 
 package io.opentelemetry.javaagent.extension.instrumentation.internal;
 
+import static java.util.Collections.emptyList;
+
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector;
+import java.util.List;
 
 /**
  * This class is internal and is hence not for public use. Its APIs are unstable and can change at
@@ -25,4 +28,12 @@ public interface ExperimentalInstrumentationModule {
    * @param injector the builder for injecting classes
    */
   default void injectClasses(ClassInjector injector) {}
+
+  /**
+   * Returns a list of helper classes that will be defined in the class loader of the instrumented
+   * library.
+   */
+  default List<String> injectedClassNames() {
+    return emptyList();
+  }
 }
diff --git a/...va/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/...va/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java
@@ -31,6 +31,7 @@
 import io.opentelemetry.javaagent.tooling.util.NamedMatcher;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
 import java.lang.instrument.Instrumentation;
+import java.util.Collections;
 import java.util.List;
 import net.bytebuddy.agent.builder.AgentBuilder;
 import net.bytebuddy.description.annotation.AnnotationSource;
@@ -69,46 +70,75 @@ AgentBuilder install(
     }
 
     if (instrumentationModule.isIndyModule()) {
-      return installIndyModule(instrumentationModule, parentAgentBuilder);
+      return installIndyModule(instrumentationModule, parentAgentBuilder, config);
     } else {
       return installInjectingModule(instrumentationModule, parentAgentBuilder, config);
     }
   }
 
   private AgentBuilder installIndyModule(
-      InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder) {
-
-    IndyModuleRegistry.registerIndyModule(instrumentationModule);
-
+      InstrumentationModule instrumentationModule,
+      AgentBuilder parentAgentBuilder,
+      ConfigProperties config) {
+    List<String> helperClassNames =
+        InstrumentationModuleMuzzle.getHelperClassNames(instrumentationModule);
     HelperResourceBuilderImpl helperResourceBuilder = new HelperResourceBuilderImpl();
     instrumentationModule.registerHelperResources(helperResourceBuilder);
+    List<TypeInstrumentation> typeInstrumentations = instrumentationModule.typeInstrumentations();
+    if (typeInstrumentations.isEmpty()) {
+      if (!helperClassNames.isEmpty() || !helperResourceBuilder.getResources().isEmpty()) {
+        logger.log(
+            WARNING,
+            "Helper classes and resources won't be injected if no types are instrumented: {0}",
+            instrumentationModule.instrumentationName());
+      }
+
+      return parentAgentBuilder;
+    }
+
+    List<String> injectedHelperClassNames = Collections.emptyList();
+    if (instrumentationModule instanceof ExperimentalInstrumentationModule) {
+      ExperimentalInstrumentationModule experimentalInstrumentationModule =
+          (ExperimentalInstrumentationModule) instrumentationModule;
+      injectedHelperClassNames = experimentalInstrumentationModule.injectedClassNames();
+    }
+
+    IndyModuleRegistry.registerIndyModule(instrumentationModule);
 
     ClassInjectorImpl injectedClassesCollector = new ClassInjectorImpl(instrumentationModule);
     if (instrumentationModule instanceof ExperimentalInstrumentationModule) {
       ((ExperimentalInstrumentationModule) instrumentationModule)
           .injectClasses(injectedClassesCollector);
     }
 
+    MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);
+
     AgentBuilder.Transformer helperInjector =
         new HelperInjector(
             instrumentationModule.instrumentationName(),
-            injectedClassesCollector.getClassesToInject(),
+            injectedHelperClassNames,
             helperResourceBuilder.getResources(),
             instrumentationModule.getClass().getClassLoader(),
             instrumentation);
+    AgentBuilder.Transformer indyHelperInjector =
+        new HelperInjector(
+            instrumentationModule.instrumentationName(),
+            injectedClassesCollector.getClassesToInject(),
+            Collections.emptyList(),
+            instrumentationModule.getClass().getClassLoader(),
+            instrumentation);
 
-    // TODO (Jonas): Adapt MuzzleMatcher to use the same type lookup strategy as the
-    // InstrumentationModuleClassLoader (see IndyModuleTypePool)
-    // MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);
     VirtualFieldImplementationInstaller contextProvider =
         virtualFieldInstallerFactory.create(instrumentationModule);
 
     AgentBuilder agentBuilder = parentAgentBuilder;
     for (TypeInstrumentation typeInstrumentation : instrumentationModule.typeInstrumentations()) {
       AgentBuilder.Identified.Extendable extendableAgentBuilder =
           setTypeMatcher(agentBuilder, instrumentationModule, typeInstrumentation)
+              .and(muzzleMatcher)
               .transform(new PatchByteCodeVersionTransformer())
-              .transform(helperInjector);
+              .transform(helperInjector)
+              .transform(indyHelperInjector);
 
       // TODO (Jonas): we are not calling
       // contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder) anymore

diff --git a/...oling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/MuzzleMatcher.java b/...oling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/MuzzleMatcher.java
@@ -14,6 +14,7 @@
 import io.opentelemetry.javaagent.tooling.TransformSafeLogger;
 import io.opentelemetry.javaagent.tooling.Utils;
 import io.opentelemetry.javaagent.tooling.config.AgentConfig;
+import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry;
 import io.opentelemetry.javaagent.tooling.muzzle.Mismatch;
 import io.opentelemetry.javaagent.tooling.muzzle.ReferenceMatcher;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
@@ -60,6 +61,10 @@ public boolean matches(
       ProtectionDomain protectionDomain) {
     if (classLoader == BOOTSTRAP_LOADER) {
       classLoader = Utils.getBootstrapProxy();
+    } else if (instrumentationModule.isIndyModule()) {
+      classLoader =
+          IndyModuleRegistry.getInstrumentationClassloader(
+              instrumentationModule.getClass().getName(), classLoader);
     }
     return matchCache.computeIfAbsent(classLoader, this::doesMatch);
   }

diff --git a/...main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java b/...main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java
@@ -9,6 +9,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle;
 import java.lang.ref.WeakReference;
 import java.util.HashSet;
@@ -84,6 +85,9 @@ static InstrumentationModuleClassLoader createInstrumentationModuleClassloader(
     // TODO (Jonas): Make muzzle include advice classes as helper classes
     // so that we don't have to include them here
     toInject.addAll(getModuleAdviceNames(module));
+    if (module instanceof ExperimentalInstrumentationModule) {
+      toInject.removeAll(((ExperimentalInstrumentationModule) module).injectedClassNames());
+    }
 
     ClassLoader agentOrExtensionCl = module.getClass().getClassLoader();
     Map<String, ClassCopySource> injectedClasses =

diff --git a/...pentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java b/...pentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java
@@ -54,16 +54,26 @@ public class InstrumentationModuleClassLoader extends ClassLoader {
   private volatile MethodHandles.Lookup cachedLookup;
 
   private final ClassLoader instrumentedCl;
+  private final boolean delegateAllToAgent;
 
   public InstrumentationModuleClassLoader(
       ClassLoader instrumentedCl,
       ClassLoader agentOrExtensionCl,
       Map<String, ClassCopySource> injectedClasses) {
+    this(instrumentedCl, agentOrExtensionCl, injectedClasses, false);
+  }
+
+  InstrumentationModuleClassLoader(
+      ClassLoader instrumentedCl,
+      ClassLoader agentOrExtensionCl,
+      Map<String, ClassCopySource> injectedClasses,
+      boolean delegateAllToAgent) {
     // agent/extension-classloader is "main"-parent, but class lookup is overridden
     super(agentOrExtensionCl);
     additionalInjectedClasses = injectedClasses;
     this.agentOrExtensionCl = agentOrExtensionCl;
     this.instrumentedCl = instrumentedCl;
+    this.delegateAllToAgent = delegateAllToAgent;
   }
 
   /**
@@ -110,7 +120,7 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
           }
         }
       }
-      if (result == null) {
+      if (result == null && shouldLoadFromAgent(name)) {
         result = tryLoad(agentOrExtensionCl, name);
       }
       if (result == null) {
@@ -128,6 +138,10 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
     }
   }
 
+  private boolean shouldLoadFromAgent(String dotClassName) {
+    return delegateAllToAgent || dotClassName.startsWith("io.opentelemetry.javaagent");
+  }
+
   private static Class<?> tryLoad(ClassLoader cl, String name) {
     try {
       return cl.loadClass(name);

diff --git a/...elemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoaderTest.java b/...elemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoaderTest.java
@@ -43,9 +43,9 @@ void checkLookup() throws Throwable {
     ClassLoader dummyParent = new URLClassLoader(new URL[] {}, null);
 
     InstrumentationModuleClassLoader m1 =
-        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
+        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);
     InstrumentationModuleClassLoader m2 =
-        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
+        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);
 
     // MethodHandles.publicLookup() always succeeds on the first invocation
     lookupAndInvokeFoo(m1);
@@ -79,7 +79,7 @@ void checkInjectedClassesHavePackage() throws Throwable {
 
     ClassLoader dummyParent = new URLClassLoader(new URL[] {}, null);
     InstrumentationModuleClassLoader m1 =
-        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
+        new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);
 
     Class<?> injected = Class.forName(A.class.getName(), true, m1);
     // inject two classes from the same package to trigger errors if we try to redefine the package
@@ -120,7 +120,7 @@ void checkClassLookupPrecedence(@TempDir Path tempDir) throws Exception {
       toInject.put(C.class.getName(), ClassCopySource.create(C.class.getName(), moduleSourceCl));
 
       InstrumentationModuleClassLoader moduleCl =
-          new InstrumentationModuleClassLoader(appCl, agentCl, toInject);
+          new InstrumentationModuleClassLoader(appCl, agentCl, toInject, true);
 
       // Verify precedence for classloading
       Class<?> clA = moduleCl.loadClass(A.class.getName());

diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java
@@ -160,7 +160,6 @@ public static void setHelperInjectorListener(HelperInjectorListener listener) {
   private Map<String, Supplier<byte[]>> getHelperMap(ClassLoader targetClassloader) {
     Map<String, Supplier<byte[]>> result = new LinkedHashMap<>();
     if (dynamicTypeMap.isEmpty()) {
-
       for (String helperClassName : helperClassNames) {
         result.put(
             helperClassName,

diff --git a/...a/indy/IndyInstrumentationTestModule.java → ...aagent/IndyInstrumentationTestModule.java b/...a/indy/IndyInstrumentationTestModule.java → ...aagent/IndyInstrumentationTestModule.java
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package indy;
+package io.opentelemetry.javaagent;
 
 import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
 import static net.bytebuddy.matcher.ElementMatchers.named;