From 4b03e98b4784975b8d51be02c3380522cf8f99bd Mon Sep 17 00:00:00 2001 From: SylvainJuge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 5 Sep 2024 01:06:13 +0200 Subject: [PATCH] make internal-reflection indy compatible (#12136) Co-authored-by: Jonas Kunz --- .../reflection/ReflectionInstrumentation.java | 22 ++++++++++++------- .../ReflectionInstrumentationModule.java | 20 ++++++++++++----- .../InstrumentationModuleClassLoader.java | 11 ++++++++-- .../javaagent/tooling/HelperInjector.java | 7 +++++- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentation.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentation.java index 472b61d5244d..d2610b20481d 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentation.java +++ b/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); } } } diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java index 7cc217d8715a..b22464b81215 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java +++ b/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 typeInstrumentations() { + return asList(new ClassInstrumentation(), new ReflectionInstrumentation()); } @Override - public List 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); } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java index fada8911eb71..3c346823acf8 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/InstrumentationModuleClassLoader.java +++ b/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); } } diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index b3ae20e43e10..b5464273b60b 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -172,7 +172,8 @@ public DynamicType.Builder transform( injectedClassLoaders.computeIfAbsent( maskNullClassLoader(classLoader), cl -> { - List helpers = helperClassesGenerator.apply(cl); + List helpers = + helperClassesGenerator.apply(unmaskNullClassLoader(cl)); LinkedHashMap> 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; }