make internal-reflection indy compatible (#12136)

Co-authored-by: Jonas Kunz <[email protected]>

Author: SylvainJuge

instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentation.java

@@ -53,21 +53,27 @@ public void transform(TypeTransformer transformer) {
 
   @SuppressWarnings("unused")
   public static class FilterFieldsAdvice {
+
+    // using AsScalar is needed to return the array itself instead of "advice Object[] return"
+    @Advice.AssignReturned.ToReturned
+    @Advice.AssignReturned.AsScalar
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void filter(
-        @Advice.Argument(0) Class<?> containingClass,
-        @Advice.Return(readOnly = false) Field[] fields) {
-      fields = ReflectionHelper.filterFields(containingClass, fields);
+    public static Field[] filter(
+        @Advice.Argument(0) Class<?> containingClass, @Advice.Return Field[] fields) {
+      return ReflectionHelper.filterFields(containingClass, fields);
     }
   }
 
   @SuppressWarnings("unused")
   public static class FilterMethodsAdvice {
+
+    // using AsScalar is needed to return the array itself instead of "advice Object[] return"
+    @Advice.AssignReturned.ToReturned
+    @Advice.AssignReturned.AsScalar
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void filter(
-        @Advice.Argument(0) Class<?> containingClass,
-        @Advice.Return(readOnly = false) Method[] methods) {
-      methods = ReflectionHelper.filterMethods(containingClass, methods);
+    public static Method[] filter(
+        @Advice.Argument(0) Class<?> containingClass, @Advice.Return Method[] methods) {
+      return ReflectionHelper.filterMethods(containingClass, methods);
     }
   }
 }

instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java

@@ -10,11 +10,15 @@
 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 io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
-public class ReflectionInstrumentationModule extends InstrumentationModule {
+public class ReflectionInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   public ReflectionInstrumentationModule() {
     super("internal-reflection");
   }
@@ -26,12 +30,18 @@ public boolean defaultEnabled(ConfigProperties config) {
   }
 
   @Override
-  public boolean isIndyModule() {
-    return false;
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return asList(new ClassInstrumentation(), new ReflectionInstrumentation());
   }
 
   @Override
-  public List<TypeInstrumentation> typeInstrumentations() {
-    return asList(new ClassInstrumentation(), new ReflectionInstrumentation());
+  public void injectClasses(ClassInjector injector) {
+    // we do not use ByteBuddy Advice dispatching in this instrumentation
+    // Instead, we manually call ReflectionHelper via ASM
+    // Easiest solution to work with indy is to inject an indy-proxy to be invoked
+    injector
+        .proxyBuilder(
+            "io.opentelemetry.javaagent.instrumentation.internal.reflection.ReflectionHelper")
+        .inject(InjectionMode.CLASS_ONLY);
   }
 }

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java

@@ -109,10 +109,17 @@ public MethodHandles.Lookup getLookup() {
     if (cachedLookup == null) {
       // Load the injected copy of LookupExposer and invoke it
       try {
+        MethodType getLookupType = MethodType.methodType(MethodHandles.Lookup.class);
         // we don't mind the race condition causing the initialization to run multiple times here
         Class<?> lookupExposer = loadClass(LookupExposer.class.getName());
-        cachedLookup = (MethodHandles.Lookup) lookupExposer.getMethod("getLookup").invoke(null);
-      } catch (Exception e) {
+        // Note: we must use MethodHandles instead of reflection here to avoid a recursion
+        // for our internal ReflectionInstrumentationModule which instruments reflection methods
+        cachedLookup =
+            (MethodHandles.Lookup)
+                MethodHandles.publicLookup()
+                    .findStatic(lookupExposer, "getLookup", getLookupType)
+                    .invoke();
+      } catch (Throwable e) {
         throw new IllegalStateException(e);
       }
     }

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java

@@ -172,7 +172,8 @@ public DynamicType.Builder<?> transform(
     injectedClassLoaders.computeIfAbsent(
         maskNullClassLoader(classLoader),
         cl -> {
-          List<HelperClassDefinition> helpers = helperClassesGenerator.apply(cl);
+          List<HelperClassDefinition> helpers =
+              helperClassesGenerator.apply(unmaskNullClassLoader(cl));
 
           LinkedHashMap<String, Supplier<byte[]>> classesToInject =
               helpers.stream()
@@ -355,6 +356,10 @@ private static ClassLoader maskNullClassLoader(ClassLoader classLoader) {
     return classLoader != null ? classLoader : BOOTSTRAP_CLASSLOADER_PLACEHOLDER;
   }
 
+  private static ClassLoader unmaskNullClassLoader(ClassLoader classLoader) {
+    return isBootClassLoader(classLoader) ? null : classLoader;
+  }
+
   private static boolean isBootClassLoader(ClassLoader classLoader) {
     return classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER;
   }