From 278edf88e9554ce124f923c3e9e15e3d45106776 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 29 Aug 2024 11:39:42 +0200 Subject: [PATCH 1/8] internal-reflection --- .../reflection/ReflectionInstrumentation.java | 22 ++++++++++++------- .../ReflectionInstrumentationModule.java | 5 ----- 2 files changed, 14 insertions(+), 13 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..9f6fcd4dec9e 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 @@ -25,11 +25,6 @@ public boolean defaultEnabled(ConfigProperties config) { return true; } - @Override - public boolean isIndyModule() { - return false; - } - @Override public List typeInstrumentations() { return asList(new ClassInstrumentation(), new ReflectionInstrumentation()); From ad7440fa922945cc049bb054c11be813457780b3 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 29 Aug 2024 15:53:32 +0200 Subject: [PATCH 2/8] commit wip for investigation --- .../ReflectionInstrumentationModule.java | 19 ++++++++++++++++++- .../indy/IndyModuleRegistry.java | 7 +++++++ 2 files changed, 25 insertions(+), 1 deletion(-) 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 9f6fcd4dec9e..ff24338a11b5 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"); } @@ -29,4 +33,17 @@ public boolean defaultEnabled(ConfigProperties config) { public List typeInstrumentations() { return asList(new ClassInstrumentation(), new ReflectionInstrumentation()); } + + @Override + public String getModuleGroup() { + return "internal-reflection"; + } + + @Override + public void injectClasses(ClassInjector injector) { + 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/IndyModuleRegistry.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java index 7eaf7f3511b6..93d1a4f5b1f9 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java @@ -45,9 +45,16 @@ public static InstrumentationModuleClassLoader getInstrumentationClassLoader( String groupName = getModuleGroup(module); + // this is a hack attempt to make it work + if (instrumentedClassLoader != null && instrumentedClassLoader.getClass().getName() + .startsWith("io.opentelemetry.javaagent.tooling.HelperInjector$1")) { + instrumentedClassLoader = null; + } + Map loadersByGroupName = instrumentationClassLoaders.get(instrumentedClassLoader); + if (loadersByGroupName == null) { throw new IllegalArgumentException( module From 96f035661c5b39d09293dd74d89911a339a27786 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Fri, 30 Aug 2024 10:06:40 +0200 Subject: [PATCH 3/8] Replace reflection with MethodHandles usage in indy bootstraping code to avoid recursion --- .../indy/InstrumentationModuleClassLoader.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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..4412e63b8b9a 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,15 @@ 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); } } From e7d4a1f739d8f2075637f3df4a5b30a7a7eec9ba Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Fri, 30 Aug 2024 15:49:56 +0200 Subject: [PATCH 4/8] Fix bootstrap proxy injection --- .../tooling/instrumentation/indy/IndyModuleRegistry.java | 6 ------ .../io/opentelemetry/javaagent/tooling/HelperInjector.java | 5 ++++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java index 93d1a4f5b1f9..cbb330ed7bde 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java @@ -45,12 +45,6 @@ public static InstrumentationModuleClassLoader getInstrumentationClassLoader( String groupName = getModuleGroup(module); - // this is a hack attempt to make it work - if (instrumentedClassLoader != null && instrumentedClassLoader.getClass().getName() - .startsWith("io.opentelemetry.javaagent.tooling.HelperInjector$1")) { - instrumentedClassLoader = null; - } - Map loadersByGroupName = instrumentationClassLoaders.get(instrumentedClassLoader); 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..e98d5681351b 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,10 @@ public DynamicType.Builder transform( injectedClassLoaders.computeIfAbsent( maskNullClassLoader(classLoader), cl -> { - List helpers = helperClassesGenerator.apply(cl); + + List helpers = helperClassesGenerator.apply( + isBootClassLoader(cl) ? null : cl + ); LinkedHashMap> classesToInject = helpers.stream() From c086e33adb62738a254e7845433d35f7fed56fd3 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 30 Aug 2024 16:47:38 +0200 Subject: [PATCH 5/8] lint --- .../tooling/instrumentation/indy/IndyModuleRegistry.java | 1 - .../indy/InstrumentationModuleClassLoader.java | 8 +++++--- .../opentelemetry/javaagent/tooling/HelperInjector.java | 6 ++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java index cbb330ed7bde..7eaf7f3511b6 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java @@ -48,7 +48,6 @@ public static InstrumentationModuleClassLoader getInstrumentationClassLoader( Map loadersByGroupName = instrumentationClassLoaders.get(instrumentedClassLoader); - if (loadersByGroupName == null) { throw new IllegalArgumentException( module 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 4412e63b8b9a..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 @@ -114,9 +114,11 @@ public MethodHandles.Lookup getLookup() { Class lookupExposer = loadClass(LookupExposer.class.getName()); // 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(); + 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 e98d5681351b..29d92fc4de4a 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -172,10 +172,8 @@ public DynamicType.Builder transform( injectedClassLoaders.computeIfAbsent( maskNullClassLoader(classLoader), cl -> { - - List helpers = helperClassesGenerator.apply( - isBootClassLoader(cl) ? null : cl - ); + List helpers = + helperClassesGenerator.apply(isBootClassLoader(cl) ? null : cl); LinkedHashMap> classesToInject = helpers.stream() From 7c7f5129da2ea91e6dfd7e4be208343179c232fc Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 30 Aug 2024 17:07:31 +0200 Subject: [PATCH 6/8] remove useless moduleGroup --- .../internal/reflection/ReflectionInstrumentationModule.java | 5 ----- 1 file changed, 5 deletions(-) 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 ff24338a11b5..d32d00cdb86d 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 @@ -34,11 +34,6 @@ public List typeInstrumentations() { return asList(new ClassInstrumentation(), new ReflectionInstrumentation()); } - @Override - public String getModuleGroup() { - return "internal-reflection"; - } - @Override public void injectClasses(ClassInjector injector) { injector From e5bfd05b8e63b29603853e4ce73e1dde6b1bcb0f Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Mon, 2 Sep 2024 11:22:15 +0200 Subject: [PATCH 7/8] add unmaskNullClassLoader method for clarity --- .../io/opentelemetry/javaagent/tooling/HelperInjector.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 29d92fc4de4a..b5464273b60b 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -173,7 +173,7 @@ public DynamicType.Builder transform( maskNullClassLoader(classLoader), cl -> { List helpers = - helperClassesGenerator.apply(isBootClassLoader(cl) ? null : cl); + helperClassesGenerator.apply(unmaskNullClassLoader(cl)); LinkedHashMap> classesToInject = helpers.stream() @@ -356,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; } From 9bcb526bb1e4ba767b853b750fd788ca5e979afa Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Wed, 4 Sep 2024 09:05:46 +0200 Subject: [PATCH 8/8] add comment --- .../internal/reflection/ReflectionInstrumentationModule.java | 3 +++ 1 file changed, 3 insertions(+) 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 d32d00cdb86d..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 @@ -36,6 +36,9 @@ public List typeInstrumentations() { @Override 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")