From 25a8763ef297922c3e6e0408384edff9c41823e7 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Tue, 26 Sep 2023 10:45:11 +0200 Subject: [PATCH 01/16] Implemented invokedynamic proxy injection for InstrumentationModules --- .../InstrumentationModule.java | 12 ++ .../injection/ClassInjector.java | 7 ++ .../injection/InjectionMode.java | 9 ++ .../injection/ProxyInjectionBuilder.java | 6 + .../InstrumentationModuleInstaller.java | 20 +++- .../instrumentation/indy/IndyBootstrap.java | 109 +++++++++++++++++- .../indy/IndyModuleRegistry.java | 2 +- .../indy/IndyModuleTypePool.java | 41 +++++++ .../InstrumentationModuleClassLoader.java | 23 +++- .../indy/injection/IndyClassInjector.java | 65 +++++++++++ .../javaagent/tooling/HelperInjector.java | 35 +++--- .../indy/IndyInstrumentationTestModule.java | 15 +++ .../src/main/java/indy/ProxyMe.java | 17 +++ .../java/indy/IndyInstrumentationTest.java | 29 +++++ .../main/java/library/MyProxySuperclass.java | 6 + 15 files changed, 371 insertions(+), 25 deletions(-) create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java create mode 100644 testing-common/integration-tests/src/main/java/indy/ProxyMe.java create mode 100644 testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java index 9d8bb78b8200..1562fee31759 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java @@ -10,6 +10,7 @@ import static net.bytebuddy.matcher.ElementMatchers.any; import io.opentelemetry.javaagent.bootstrap.internal.ExperimentalConfig; +import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; import io.opentelemetry.sdk.autoconfigure.spi.Ordered; import java.util.Collections; @@ -159,4 +160,15 @@ public ElementMatcher.Junction classLoaderMatcher() { public List getAdditionalHelperClassNames() { return Collections.emptyList(); } + + /** + * Only functional for Modules where {@link #isIndyModule()} returns {@code true}. + * + * Normally, helper and advice classes are loaded in a child classloader of the instrumented classloader. + * This method allows to inject classes directly into the instrumented classloader instead. + * + * @param injector the builder for injecting classes + */ + public void injectClasses(ClassInjector injector) { + } } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java new file mode 100644 index 000000000000..a5b60f3ee318 --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java @@ -0,0 +1,7 @@ +package io.opentelemetry.javaagent.extension.instrumentation.injection; + +public interface ClassInjector { + + ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyName); + +} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java new file mode 100644 index 000000000000..1848c6bf498c --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java @@ -0,0 +1,9 @@ +package io.opentelemetry.javaagent.extension.instrumentation.injection; + +public enum InjectionMode { + + CLASS_ONLY + //TODO: implement the modes RESOURCE_ONLY and CLASS_AND_RESOURCE + //This will require a custom URL implementation for byte arrays, similar to how bytebuddy's ByteArrayClassLoader does it + +} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java new file mode 100644 index 000000000000..9bd8836b2dee --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java @@ -0,0 +1,6 @@ +package io.opentelemetry.javaagent.extension.instrumentation.injection; + +public interface ProxyInjectionBuilder { + + void inject(InjectionMode mode); +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index 8a866a43b771..f6bb3b2e6007 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -23,6 +23,7 @@ import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyTypeTransformerImpl; import io.opentelemetry.javaagent.tooling.instrumentation.indy.PatchByteCodeVersionTransformer; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.injection.IndyClassInjector; import io.opentelemetry.javaagent.tooling.muzzle.HelperResourceBuilderImpl; import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle; import io.opentelemetry.javaagent.tooling.util.IgnoreFailedTypeMatcher; @@ -78,8 +79,22 @@ private AgentBuilder installIndyModule( IndyModuleRegistry.registerIndyModule(instrumentationModule); + HelperResourceBuilderImpl helperResourceBuilder = new HelperResourceBuilderImpl(); + instrumentationModule.registerHelperResources(helperResourceBuilder); + + IndyClassInjector injectedClassesCollector = new IndyClassInjector(instrumentationModule); + instrumentationModule.injectClasses(injectedClassesCollector); + + AgentBuilder.Transformer helperInjector = + new HelperInjector( + instrumentationModule.instrumentationName(), + injectedClassesCollector.getClassesToInject(), + helperResourceBuilder.getResources(), + instrumentationModule.getClass().getClassLoader(), + instrumentation); + // TODO (Jonas): Adapt MuzzleMatcher to use the same type lookup strategy as the - // InstrumentationModuleClassLoader + // InstrumentationModuleClassLoader (see IndyModuleTypePool) // MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config); VirtualFieldImplementationInstaller contextProvider = virtualFieldInstallerFactory.create(instrumentationModule); @@ -88,7 +103,8 @@ private AgentBuilder installIndyModule( for (TypeInstrumentation typeInstrumentation : instrumentationModule.typeInstrumentations()) { AgentBuilder.Identified.Extendable extendableAgentBuilder = setTypeMatcher(agentBuilder, instrumentationModule, typeInstrumentation) - .transform(new PatchByteCodeVersionTransformer()); + .transform(new PatchByteCodeVersionTransformer()) + .transform(helperInjector); // TODO (Jonas): we are not calling // contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder) anymore diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java index bea30381559b..a1b651f2af67 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java @@ -63,6 +63,13 @@ public class IndyBootstrap { private static final Method indyBootstrapMethod; + private static final String BOOTSTRAP_KIND_ADVICE = "advice"; + private static final String BOOTSTRAP_KIND_PROXY = "proxy"; + + private static final String PROXY_KIND_STATIC = "static"; + private static final String PROXY_KIND_CONSTRUCTOR = "constructor"; + private static final String PROXY_KIND_VIRTUAL = "virtual"; + static { try { indyBootstrapMethod = @@ -118,6 +125,31 @@ private static ConstantCallSite internalBootstrap( String adviceMethodName, MethodType adviceMethodType, Object[] args) { + try { + String kind = (String) args[0]; + switch (kind) { + case BOOTSTRAP_KIND_ADVICE: + // See the getAdviceBootstrapArguments method for the argument definitions + return bootstrapAdvice(lookup, adviceMethodName, adviceMethodType, (String) args[1], (String) args[2]); + case BOOTSTRAP_KIND_PROXY: + //See getProxyFactory for the argument definitions + return bootstrapProxyMethod(lookup, adviceMethodName, adviceMethodType, (String) args[1], (String) args[2], (String) args[3]); + default: + throw new IllegalArgumentException("Unknown bootstrapping kind: "+kind); + } + } catch (Exception e) { + logger.log(Level.SEVERE, e.getMessage(), e); + return null; + } + } + + private static ConstantCallSite bootstrapAdvice( + MethodHandles.Lookup lookup, + String adviceMethodName, + MethodType adviceMethodType, + String moduleClassName, + String adviceClassName) + throws NoSuchMethodException, IllegalAccessException, ClassNotFoundException { CallDepth callDepth = CallDepth.forClass(IndyBootstrap.class); try { if (callDepth.getAndIncrement() > 0) { @@ -130,9 +162,6 @@ private static ConstantCallSite internalBootstrap( new Throwable()); return null; } - // See the getAdviceBootstrapArguments method for where these arguments come from - String moduleClassName = (String) args[0]; - String adviceClassName = (String) args[1]; InstrumentationModuleClassLoader instrumentationClassloader = IndyModuleRegistry.getInstrumentationClassloader( @@ -146,9 +175,6 @@ private static ConstantCallSite internalBootstrap( .getLookup() .findStatic(adviceClass, adviceMethodName, adviceMethodType); return new ConstantCallSite(methodHandle); - } catch (Exception e) { - logger.log(Level.SEVERE, e.getMessage(), e); - return null; } finally { callDepth.decrementAndGet(); } @@ -160,7 +186,78 @@ static Advice.BootstrapArgumentResolver.Factory getAdviceBootstrapArguments( return (adviceMethod, exit) -> (instrumentedType, instrumentedMethod) -> Arrays.asList( + JavaConstant.Simple.ofLoaded(BOOTSTRAP_KIND_ADVICE), JavaConstant.Simple.ofLoaded(moduleName), JavaConstant.Simple.ofLoaded(adviceMethod.getDeclaringType().getName())); } + + private static ConstantCallSite bootstrapProxyMethod( + MethodHandles.Lookup lookup, + String proxyMethodName, + MethodType expectedMethodType, + String moduleClassName, + String proxyClassName, + String methodKind) + throws NoSuchMethodException, IllegalAccessException, ClassNotFoundException { + InstrumentationModuleClassLoader instrumentationClassloader = + IndyModuleRegistry.getInstrumentationClassloader( + moduleClassName, lookup.lookupClass().getClassLoader()); + + Class proxiedClass = instrumentationClassloader.loadClass(proxyClassName); + + MethodHandle target; + switch (methodKind) { + case PROXY_KIND_STATIC: + target = MethodHandles.publicLookup().findStatic(proxiedClass, proxyMethodName, expectedMethodType); + break; + case PROXY_KIND_CONSTRUCTOR: + target = + MethodHandles.publicLookup() + .findConstructor(proxiedClass, expectedMethodType.changeReturnType(void.class)) + .asType(expectedMethodType); //return type is the proxied class, but proxies expect Object + break; + case PROXY_KIND_VIRTUAL: + target = + MethodHandles.publicLookup() + .findVirtual(proxiedClass, proxyMethodName, expectedMethodType.dropParameterTypes(0, 1)) + .asType(expectedMethodType); //first argument type is the proxied class, but proxies expect Object + break; + default: + throw new IllegalStateException("unknown proxy method kind: "+methodKind); + } + return new ConstantCallSite(target); + } + + /** + * Creates a proxy factory for generating proxies for classes which are loaded by + * an {@link InstrumentationModuleClassLoader} for the provided {@link InstrumentationModule}. + * + * @param instrumentationModule the isntrumentation module used to load the proxied target classes + * @return the factory + */ + public static IndyProxyFactory getProxyFactory(InstrumentationModule instrumentationModule) { + String moduleName = instrumentationModule.getClass().getName(); + return new IndyProxyFactory(getIndyBootstrapMethod(), (proxiedType, proxiedMethod) -> { + + String methodKind; + if(proxiedMethod.isConstructor()) { + methodKind = PROXY_KIND_CONSTRUCTOR; + } else if(proxiedMethod.isMethod()) { + if(proxiedMethod.isStatic()) { + methodKind = PROXY_KIND_STATIC; + } else { + methodKind = PROXY_KIND_VIRTUAL; + } + } else { + throw new IllegalArgumentException("Unknown type of method: "+proxiedMethod.getName()); + } + + return Arrays.asList( + JavaConstant.Simple.ofLoaded(BOOTSTRAP_KIND_PROXY), + JavaConstant.Simple.ofLoaded(moduleName), + JavaConstant.Simple.ofLoaded(proxiedType.getName()), + JavaConstant.Simple.ofLoaded(methodKind) + ); + }); + } } 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 b6099ee12050..dfe421224c23 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 @@ -77,7 +77,7 @@ private static ClassLoader maskNullClassLoader(ClassLoader classLoader) { return classLoader == null ? BOOT_LOADER : classLoader; } - private static InstrumentationModuleClassLoader createInstrumentationModuleClassloader( + static InstrumentationModuleClassLoader createInstrumentationModuleClassloader( InstrumentationModule module, ClassLoader instrumentedClassloader) { Set toInject = new HashSet<>(InstrumentationModuleMuzzle.getHelperClassNames(module)); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java new file mode 100644 index 000000000000..29705572d71a --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java @@ -0,0 +1,41 @@ +package io.opentelemetry.javaagent.tooling.instrumentation.indy; + +import io.opentelemetry.instrumentation.api.internal.cache.Cache; +import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; +import io.opentelemetry.javaagent.tooling.muzzle.AgentTooling; +import net.bytebuddy.pool.TypePool; +import java.util.concurrent.ConcurrentHashMap; + +public class IndyModuleTypePool { + + private IndyModuleTypePool() {} + + private static final ConcurrentHashMap< + InstrumentationModule, + Cache> + classloadersForTypePools = new ConcurrentHashMap<>(); + + + public static TypePool get(ClassLoader instrumentedCl, InstrumentationModule module) { + + Cache cacheForModule = + classloadersForTypePools.computeIfAbsent(module, (k) -> Cache.weak()); + + ResourceOnlyClassLoader resourceOnlyCl = cacheForModule.computeIfAbsent( + instrumentedCl, cl -> new ResourceOnlyClassLoader(IndyModuleRegistry.createInstrumentationModuleClassloader(module, cl))); + + return AgentTooling.poolStrategy().typePool(AgentTooling.locationStrategy().classFileLocator(resourceOnlyCl), resourceOnlyCl); + } + + private static class ResourceOnlyClassLoader extends ClassLoader { + + public ResourceOnlyClassLoader(ClassLoader delegate) { + super(delegate); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + throw new UnsupportedOperationException("This classloader should only be used for resource loading!"); + } + } +} 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 c790f34b1252..a6b5c4481b14 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 @@ -9,6 +9,7 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; +import java.lang.ref.WeakReference; import java.net.URL; import java.security.PrivilegedAction; import java.security.ProtectionDomain; @@ -51,9 +52,17 @@ public class InstrumentationModuleClassLoader extends ClassLoader { private final Map additionalInjectedClasses; private final ClassLoader agentOrExtensionCl; - private final ClassLoader instrumentedCl; private volatile MethodHandles.Lookup cachedLookup; + /** + * We only weakly reference the instrumented classloader to make sure that we don't prevent it from being GCed. + * This only works as long as this {@link InstrumentationModuleClassLoader} has not loaded any classes referencing types from the instrumentedCl yet: + * As soon as such a class is loaded, it will strongly reference the instrumentedCl. + * + * For this reason, users of {@link InstrumentationModuleClassLoader} must make sure that they only weakly reference it if it is actually used for classloading and not for resource lookups (e.g. via {@link net.bytebuddy.pool.TypePool}s. + */ + private final WeakReference instrumentedClassloader; + public InstrumentationModuleClassLoader( ClassLoader instrumentedCl, ClassLoader agentOrExtensionCl, @@ -62,7 +71,15 @@ public InstrumentationModuleClassLoader( super(agentOrExtensionCl); additionalInjectedClasses = injectedClasses; this.agentOrExtensionCl = agentOrExtensionCl; - this.instrumentedCl = instrumentedCl; + this.instrumentedClassloader = new WeakReference<>(instrumentedCl); + } + + private ClassLoader getInstrumentedClassloader() { + ClassLoader classLoader = instrumentedClassloader.get(); + if(classLoader == null) { + throw new IllegalStateException("The referenced instrumentated ClassLoader has already been GCed!"); + } + return classLoader; } /** @@ -89,6 +106,7 @@ public MethodHandles.Lookup getLookup() { @Override @SuppressWarnings("removal") // AccessController is deprecated for removal protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + ClassLoader instrumentedCl = getInstrumentedClassloader(); synchronized (getClassLoadingLock(name)) { Class result = findLoadedClass(name); @@ -137,6 +155,7 @@ private static Class tryLoad(ClassLoader cl, String name) { @Override public URL getResource(String resourceName) { + ClassLoader instrumentedCl = getInstrumentedClassloader(); String className = resourceToClassName(resourceName); if (className == null) { // delegate to just the default parent (the agent classloader) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java new file mode 100644 index 000000000000..c565c6328ef1 --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java @@ -0,0 +1,65 @@ +package io.opentelemetry.javaagent.tooling.instrumentation.indy.injection; + +import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; +import io.opentelemetry.javaagent.extension.instrumentation.injection.ProxyInjectionBuilder; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyBootstrap; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleTypePool; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyProxyFactory; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.pool.TypePool; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; + +public class IndyClassInjector implements ClassInjector { + + /** + * The CL which loads the {@link io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule}. + */ + private final InstrumentationModule instrumentationModule; + + private final Map> classesToInject; + + private final IndyProxyFactory proxyFactory; + + public IndyClassInjector(InstrumentationModule module) { + instrumentationModule = module; + classesToInject = new HashMap<>(); + proxyFactory = IndyBootstrap.getProxyFactory(module); + } + + public Map> getClassesToInject() { + return classesToInject; + } + + @Override + public ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyName) { + return new ProxyBuilder(classToProxy, newProxyName); + } + + + private class ProxyBuilder implements ProxyInjectionBuilder { + + private final String classToProxy; + private final String proxyClassName; + + ProxyBuilder(String classToProxy, String proxyClassName) { + this.classToProxy = classToProxy; + this.proxyClassName = proxyClassName; + } + + @Override + public void inject(InjectionMode mode) { + if(mode != InjectionMode.CLASS_ONLY) { + throw new UnsupportedOperationException("Not yet implemented"); + } + classesToInject.put(proxyClassName, cl -> { + TypePool typePool = IndyModuleTypePool.get(cl, instrumentationModule); + TypeDescription proxiedType = typePool.describe(classToProxy).resolve(); + return proxyFactory.generateProxy(proxiedType, proxyClassName).getBytes(); + }); + } + } +} 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 4b4e50b4c90e..eb0fe050b7d6 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -30,6 +30,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nullable; import net.bytebuddy.agent.builder.AgentBuilder.Transformer; @@ -94,7 +95,7 @@ Class inject(ClassLoader classLoader, String className) { private final List helperResources; @Nullable private final ClassLoader helpersSource; @Nullable private final Instrumentation instrumentation; - private final Map> dynamicTypeMap = new LinkedHashMap<>(); + private final Map> dynamicTypeMap = new LinkedHashMap<>(); private final Cache injectedClassLoaders = Cache.weak(); private final Cache resourcesInjectedClassLoaders = Cache.weak(); @@ -124,17 +125,19 @@ public HelperInjector( this.instrumentation = instrumentation; } - private HelperInjector( + public HelperInjector( String requestingName, - Map> helperMap, + Map> helperMap, + List helperResources, + ClassLoader helpersSource, Instrumentation instrumentation) { this.requestingName = requestingName; this.helperClassNames = helperMap.keySet(); this.dynamicTypeMap.putAll(helperMap); - this.helperResources = Collections.emptyList(); - this.helpersSource = null; + this.helperResources = helperResources; + this.helpersSource = helpersSource; this.instrumentation = instrumentation; } @@ -142,20 +145,20 @@ public static HelperInjector forDynamicTypes( String requestingName, Collection> helpers, Instrumentation instrumentation) { - Map> bytes = new HashMap<>(helpers.size()); + Map> bytes = new HashMap<>(helpers.size()); for (DynamicType.Unloaded helper : helpers) { - bytes.put(helper.getTypeDescription().getName(), helper::getBytes); + bytes.put(helper.getTypeDescription().getName(), cl -> helper.getBytes()); } - return new HelperInjector(requestingName, bytes, instrumentation); + return new HelperInjector(requestingName, bytes, Collections.emptyList(), null, instrumentation); } public static void setHelperInjectorListener(HelperInjectorListener listener) { helperInjectorListener = listener; } - private Map> getHelperMap() { + private Map> getHelperMap(ClassLoader targetClassloader) { + Map> result = new LinkedHashMap<>(); if (dynamicTypeMap.isEmpty()) { - Map> result = new LinkedHashMap<>(); for (String helperClassName : helperClassNames) { result.put( @@ -172,11 +175,15 @@ private Map> getHelperMap() { } }); } - - return result; } else { - return dynamicTypeMap; + dynamicTypeMap.forEach((name, bytecodeGenerator) -> { + //Eagerly compute bytecode to not risk accidentally holding onto targetClassloader for too long + byte[] bytecode = bytecodeGenerator.apply(targetClassloader); + result.put(name, () -> bytecode); + }); } + + return result; } @Override @@ -265,7 +272,7 @@ private void injectHelperClasses(TypeDescription typeDescription, ClassLoader cl new Object[] {cl, helperClassNames}); } - Map> classnameToBytes = getHelperMap(); + Map> classnameToBytes = getHelperMap(cl); Map map = helperInjectors.computeIfAbsent(cl, (unused) -> new ConcurrentHashMap<>()); for (Map.Entry> entry : classnameToBytes.entrySet()) { diff --git a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java index 3b1a831d931c..86a01cefd58b 100644 --- a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java @@ -14,6 +14,8 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.Collections; import java.util.List; +import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; import net.bytebuddy.asm.Advice; import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument; import net.bytebuddy.asm.Advice.AssignReturned.ToFields.ToField; @@ -42,6 +44,19 @@ public boolean isHelperClass(String className) { return className.equals(LocalHelper.class.getName()); } + @Override + public List getAdditionalHelperClassNames() { + //TODO: should not be needed as soon as we automatically add proxied classes to muzzle root set + return Collections.singletonList("indy.ProxyMe"); + } + + @Override + public void injectClasses(ClassInjector injector) { + injector + .proxyBuilder("indy.ProxyMe", "foo.bar.Proxy") + .inject(InjectionMode.CLASS_ONLY); + } + public static class Instrumentation implements TypeInstrumentation { @Override diff --git a/testing-common/integration-tests/src/main/java/indy/ProxyMe.java b/testing-common/integration-tests/src/main/java/indy/ProxyMe.java new file mode 100644 index 000000000000..b1444358f144 --- /dev/null +++ b/testing-common/integration-tests/src/main/java/indy/ProxyMe.java @@ -0,0 +1,17 @@ +package indy; + +import library.MyProxySuperclass; +import java.util.concurrent.Callable; + +public class ProxyMe extends MyProxySuperclass implements Callable { + + @Override + public String call() { + return "Hi from ProxyMe"; + } + + + public static String staticHello() { + return "Hi from static"; + } +} diff --git a/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java b/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java index 6d87267c50ac..c6c14f857f3f 100644 --- a/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java +++ b/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java @@ -10,6 +10,8 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import java.lang.reflect.Field; +import java.util.concurrent.Callable; @SuppressWarnings({"unused", "MethodCanBeStatic"}) public class IndyInstrumentationTest { @@ -110,4 +112,31 @@ void testHelperClassLoading() { assertThat(globalHelper.getName()).endsWith("GlobalHelper"); assertThat(globalHelper.getClassLoader().getClass().getName()).endsWith("AgentClassLoader"); } + + + @Test + @SuppressWarnings("unchecked") + void testProxyInjection() throws Exception{ + Class proxyClass = Class.forName("foo.bar.Proxy"); + + //create an instance and invoke static & non-static methods + //this verifies that our invokedynamic bootstrapping works for constructors, static and non-static methods + + Object proxyInstance = proxyClass.getConstructor().newInstance(); + assertThat(proxyInstance).isInstanceOf(Callable.class); + + String invocResult = ((Callable) proxyInstance).call(); + assertThat(invocResult).isEqualTo("Hi from ProxyMe"); + + String staticResult = (String) proxyClass.getMethod("staticHello").invoke(null); + assertThat(staticResult).isEqualTo("Hi from static"); + + Field delegateField = proxyClass.getDeclaredField("delegate"); + delegateField.setAccessible(true); + Object delegate = delegateField.get(proxyInstance); + + ClassLoader delegateCl = delegate.getClass().getClassLoader(); + assertThat(delegate.getClass().getName()).isEqualTo("indy.ProxyMe"); + assertThat(delegateCl.getClass().getName()).endsWith("InstrumentationModuleClassLoader"); + } } diff --git a/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java b/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java new file mode 100644 index 000000000000..3c8003c099e2 --- /dev/null +++ b/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java @@ -0,0 +1,6 @@ +package library; + +/** + * Custom superclass to be used for testing the proxying mechanism. + */ +public class MyProxySuperclass {} From 7a03962626d714c423a822398a39678e45b2c3bf Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 27 Sep 2023 12:39:33 +0200 Subject: [PATCH 02/16] Comments and spotless --- .../InstrumentationModule.java | 8 +- .../injection/ClassInjector.java | 6 +- .../injection/InjectionMode.java | 11 ++- .../injection/ProxyInjectionBuilder.java | 5 ++ .../InstrumentationModuleInstaller.java | 4 +- .../instrumentation/indy/IndyBootstrap.java | 82 +++++++++++-------- .../indy/IndyModuleTypePool.java | 55 +++++++++++-- .../InstrumentationModuleClassLoader.java | 16 ++-- ...ssInjector.java => ClassInjectorImpl.java} | 31 +++---- .../javaagent/tooling/HelperInjector.java | 15 ++-- .../indy/IndyInstrumentationTestModule.java | 11 ++- .../src/main/java/indy/ProxyMe.java | 8 +- .../java/indy/IndyInstrumentationTest.java | 14 ++-- .../main/java/library/MyProxySuperclass.java | 8 +- 14 files changed, 181 insertions(+), 93 deletions(-) rename javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/{IndyClassInjector.java => ClassInjectorImpl.java} (76%) diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java index 1562fee31759..6e9cbaccb884 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java @@ -164,11 +164,11 @@ public List getAdditionalHelperClassNames() { /** * Only functional for Modules where {@link #isIndyModule()} returns {@code true}. * - * Normally, helper and advice classes are loaded in a child classloader of the instrumented classloader. - * This method allows to inject classes directly into the instrumented classloader instead. + *

Normally, helper and advice classes are loaded in a child classloader of the instrumented + * classloader. This method allows to inject classes directly into the instrumented classloader + * instead. * * @param injector the builder for injecting classes */ - public void injectClasses(ClassInjector injector) { - } + public void injectClasses(ClassInjector injector) {} } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java index a5b60f3ee318..06029d725170 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java @@ -1,7 +1,11 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.extension.instrumentation.injection; public interface ClassInjector { ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyName); - } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java index 1848c6bf498c..39e38ad27864 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java @@ -1,9 +1,14 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.extension.instrumentation.injection; public enum InjectionMode { - CLASS_ONLY - //TODO: implement the modes RESOURCE_ONLY and CLASS_AND_RESOURCE - //This will require a custom URL implementation for byte arrays, similar to how bytebuddy's ByteArrayClassLoader does it + // TODO: implement the modes RESOURCE_ONLY and CLASS_AND_RESOURCE + // This will require a custom URL implementation for byte arrays, similar to how bytebuddy's + // ByteArrayClassLoader does it } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java index 9bd8836b2dee..38f8e69686ad 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.extension.instrumentation.injection; public interface ProxyInjectionBuilder { diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index f6bb3b2e6007..6223a804cdd5 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -23,7 +23,7 @@ import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyTypeTransformerImpl; import io.opentelemetry.javaagent.tooling.instrumentation.indy.PatchByteCodeVersionTransformer; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.injection.IndyClassInjector; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.injection.ClassInjectorImpl; import io.opentelemetry.javaagent.tooling.muzzle.HelperResourceBuilderImpl; import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle; import io.opentelemetry.javaagent.tooling.util.IgnoreFailedTypeMatcher; @@ -82,7 +82,7 @@ private AgentBuilder installIndyModule( HelperResourceBuilderImpl helperResourceBuilder = new HelperResourceBuilderImpl(); instrumentationModule.registerHelperResources(helperResourceBuilder); - IndyClassInjector injectedClassesCollector = new IndyClassInjector(instrumentationModule); + ClassInjectorImpl injectedClassesCollector = new ClassInjectorImpl(instrumentationModule); instrumentationModule.injectClasses(injectedClassesCollector); AgentBuilder.Transformer helperInjector = diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java index a1b651f2af67..4574caa6ed8e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyBootstrap.java @@ -130,12 +130,19 @@ private static ConstantCallSite internalBootstrap( switch (kind) { case BOOTSTRAP_KIND_ADVICE: // See the getAdviceBootstrapArguments method for the argument definitions - return bootstrapAdvice(lookup, adviceMethodName, adviceMethodType, (String) args[1], (String) args[2]); + return bootstrapAdvice( + lookup, adviceMethodName, adviceMethodType, (String) args[1], (String) args[2]); case BOOTSTRAP_KIND_PROXY: - //See getProxyFactory for the argument definitions - return bootstrapProxyMethod(lookup, adviceMethodName, adviceMethodType, (String) args[1], (String) args[2], (String) args[3]); + // See getProxyFactory for the argument definitions + return bootstrapProxyMethod( + lookup, + adviceMethodName, + adviceMethodType, + (String) args[1], + (String) args[2], + (String) args[3]); default: - throw new IllegalArgumentException("Unknown bootstrapping kind: "+kind); + throw new IllegalArgumentException("Unknown bootstrapping kind: " + kind); } } catch (Exception e) { logger.log(Level.SEVERE, e.getMessage(), e); @@ -208,56 +215,63 @@ private static ConstantCallSite bootstrapProxyMethod( MethodHandle target; switch (methodKind) { case PROXY_KIND_STATIC: - target = MethodHandles.publicLookup().findStatic(proxiedClass, proxyMethodName, expectedMethodType); + target = + MethodHandles.publicLookup() + .findStatic(proxiedClass, proxyMethodName, expectedMethodType); break; case PROXY_KIND_CONSTRUCTOR: target = MethodHandles.publicLookup() .findConstructor(proxiedClass, expectedMethodType.changeReturnType(void.class)) - .asType(expectedMethodType); //return type is the proxied class, but proxies expect Object + .asType(expectedMethodType); // return type is the proxied class, but proxies expect + // Object break; case PROXY_KIND_VIRTUAL: target = MethodHandles.publicLookup() - .findVirtual(proxiedClass, proxyMethodName, expectedMethodType.dropParameterTypes(0, 1)) - .asType(expectedMethodType); //first argument type is the proxied class, but proxies expect Object + .findVirtual( + proxiedClass, proxyMethodName, expectedMethodType.dropParameterTypes(0, 1)) + .asType( + expectedMethodType); // first argument type is the proxied class, but proxies + // expect Object break; default: - throw new IllegalStateException("unknown proxy method kind: "+methodKind); + throw new IllegalStateException("unknown proxy method kind: " + methodKind); } return new ConstantCallSite(target); } /** - * Creates a proxy factory for generating proxies for classes which are loaded by - * an {@link InstrumentationModuleClassLoader} for the provided {@link InstrumentationModule}. + * Creates a proxy factory for generating proxies for classes which are loaded by an {@link + * InstrumentationModuleClassLoader} for the provided {@link InstrumentationModule}. * - * @param instrumentationModule the isntrumentation module used to load the proxied target classes - * @return the factory + * @param instrumentationModule the instrumentation module used to load the proxied target classes + * @return a factory for generating proxy classes */ public static IndyProxyFactory getProxyFactory(InstrumentationModule instrumentationModule) { String moduleName = instrumentationModule.getClass().getName(); - return new IndyProxyFactory(getIndyBootstrapMethod(), (proxiedType, proxiedMethod) -> { - - String methodKind; - if(proxiedMethod.isConstructor()) { - methodKind = PROXY_KIND_CONSTRUCTOR; - } else if(proxiedMethod.isMethod()) { - if(proxiedMethod.isStatic()) { - methodKind = PROXY_KIND_STATIC; - } else { - methodKind = PROXY_KIND_VIRTUAL; - } - } else { - throw new IllegalArgumentException("Unknown type of method: "+proxiedMethod.getName()); - } + return new IndyProxyFactory( + getIndyBootstrapMethod(), + (proxiedType, proxiedMethod) -> { + String methodKind; + if (proxiedMethod.isConstructor()) { + methodKind = PROXY_KIND_CONSTRUCTOR; + } else if (proxiedMethod.isMethod()) { + if (proxiedMethod.isStatic()) { + methodKind = PROXY_KIND_STATIC; + } else { + methodKind = PROXY_KIND_VIRTUAL; + } + } else { + throw new IllegalArgumentException( + "Unknown type of method: " + proxiedMethod.getName()); + } - return Arrays.asList( - JavaConstant.Simple.ofLoaded(BOOTSTRAP_KIND_PROXY), - JavaConstant.Simple.ofLoaded(moduleName), - JavaConstant.Simple.ofLoaded(proxiedType.getName()), - JavaConstant.Simple.ofLoaded(methodKind) - ); - }); + return Arrays.asList( + JavaConstant.Simple.ofLoaded(BOOTSTRAP_KIND_PROXY), + JavaConstant.Simple.ofLoaded(moduleName), + JavaConstant.Simple.ofLoaded(proxiedType.getName()), + JavaConstant.Simple.ofLoaded(methodKind)); + }); } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java index 29705572d71a..297303a77b64 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java @@ -1,30 +1,68 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.tooling.instrumentation.indy; import io.opentelemetry.instrumentation.api.internal.cache.Cache; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.tooling.muzzle.AgentTooling; -import net.bytebuddy.pool.TypePool; import java.util.concurrent.ConcurrentHashMap; +import net.bytebuddy.pool.TypePool; public class IndyModuleTypePool { private IndyModuleTypePool() {} + /** + * We implement the TypePool by delegating the .class resource lookup to actual dummy {@link + * InstrumentationModuleClassLoader} instances. We cache those dummy instances so that repeated + * calls to {@link #get(ClassLoader, InstrumentationModule)} share the underlying cache for {@link + * net.bytebuddy.description.type.TypeDescription}s. Note that it is important that the {@link + * InstrumentationModuleClassLoader} only weakly references the {@link ClassLoader} being + * instrumented, which is also used as the key of the cache. Otherwise we would accidentally + * create a strong reference, preventing the instrumented {@link ClassLoader} from being ever + * GCed. To ensure that the cached {@link InstrumentationModuleClassLoader} is only used for + * resource lookups and not for classloading, it is wrapped in a {@link ResourceOnlyClassLoader}. + * Loading classes could otherwise cause the {@link InstrumentationModuleClassLoader} to strongly + * reference the instrumented {@link ClassLoader}. + */ private static final ConcurrentHashMap< - InstrumentationModule, - Cache> + InstrumentationModule, Cache> classloadersForTypePools = new ConcurrentHashMap<>(); - + /** + * Provides a {@link TypePool} which has the same lookup rules for {@link + * net.bytebuddy.description.type.TypeDescription}s as {@link InstrumentationModuleClassLoader} + * have for classes. + * + * @param instrumentedCl the classloader being instrumented (e.g. for which the {@link + * InstrumentationModuleClassLoader} is being created). + * @param module the {@link InstrumentationModule} performing the instrumentation + * @return the type pool + */ public static TypePool get(ClassLoader instrumentedCl, InstrumentationModule module) { Cache cacheForModule = classloadersForTypePools.computeIfAbsent(module, (k) -> Cache.weak()); - ResourceOnlyClassLoader resourceOnlyCl = cacheForModule.computeIfAbsent( - instrumentedCl, cl -> new ResourceOnlyClassLoader(IndyModuleRegistry.createInstrumentationModuleClassloader(module, cl))); + ResourceOnlyClassLoader resourceOnlyCl = + cacheForModule.computeIfAbsent( + instrumentedCl, + cl -> + new ResourceOnlyClassLoader( + IndyModuleRegistry.createInstrumentationModuleClassloader(module, cl))); - return AgentTooling.poolStrategy().typePool(AgentTooling.locationStrategy().classFileLocator(resourceOnlyCl), resourceOnlyCl); + // TODO: this implementation makes the cache for TypeDescriptions somewhat inefficient + // The returned pool will cache all returned TypeDescriptions as if they were loaded by the + // dummy InstrumentationModuleClassLoader, + // even the classes which are actually loaded by the instrumented Classloader or the agent + // Classloader + // This could be improved by implementing a custom TypePool instead, which delegates to parent + // TypePools and mirrors the delegation model of the InstrumentationModuleClassLoader + return AgentTooling.poolStrategy() + .typePool(AgentTooling.locationStrategy().classFileLocator(resourceOnlyCl), resourceOnlyCl); } private static class ResourceOnlyClassLoader extends ClassLoader { @@ -35,7 +73,8 @@ public ResourceOnlyClassLoader(ClassLoader delegate) { @Override protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - throw new UnsupportedOperationException("This classloader should only be used for resource loading!"); + throw new UnsupportedOperationException( + "This classloader should only be used for resource loading!"); } } } 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 a6b5c4481b14..9e99fc903d50 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 @@ -55,11 +55,14 @@ public class InstrumentationModuleClassLoader extends ClassLoader { private volatile MethodHandles.Lookup cachedLookup; /** - * We only weakly reference the instrumented classloader to make sure that we don't prevent it from being GCed. - * This only works as long as this {@link InstrumentationModuleClassLoader} has not loaded any classes referencing types from the instrumentedCl yet: - * As soon as such a class is loaded, it will strongly reference the instrumentedCl. + * We only weakly reference the instrumented classloader to make sure that we don't prevent it + * from being GCed. This only works as long as this {@link InstrumentationModuleClassLoader} has + * not loaded any classes referencing types from the instrumentedCl yet: As soon as such a class + * is loaded, it will strongly reference the instrumentedCl. * - * For this reason, users of {@link InstrumentationModuleClassLoader} must make sure that they only weakly reference it if it is actually used for classloading and not for resource lookups (e.g. via {@link net.bytebuddy.pool.TypePool}s. + *

For this reason, users of {@link InstrumentationModuleClassLoader} must make sure that they + * only weakly reference it if it is actually used for classloading and not for resource lookups + * (e.g. via {@link net.bytebuddy.pool.TypePool}s. */ private final WeakReference instrumentedClassloader; @@ -76,8 +79,9 @@ public InstrumentationModuleClassLoader( private ClassLoader getInstrumentedClassloader() { ClassLoader classLoader = instrumentedClassloader.get(); - if(classLoader == null) { - throw new IllegalStateException("The referenced instrumentated ClassLoader has already been GCed!"); + if (classLoader == null) { + throw new IllegalStateException( + "The referenced instrumented ClassLoader has already been GCed!"); } return classLoader; } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java similarity index 76% rename from javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java rename to javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java index c565c6328ef1..3088f956293f 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/IndyClassInjector.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.tooling.instrumentation.indy.injection; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; @@ -7,24 +12,21 @@ import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyBootstrap; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleTypePool; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyProxyFactory; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.pool.TypePool; import java.util.HashMap; import java.util.Map; import java.util.function.Function; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.pool.TypePool; -public class IndyClassInjector implements ClassInjector { +public class ClassInjectorImpl implements ClassInjector { - /** - * The CL which loads the {@link io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule}. - */ private final InstrumentationModule instrumentationModule; private final Map> classesToInject; private final IndyProxyFactory proxyFactory; - public IndyClassInjector(InstrumentationModule module) { + public ClassInjectorImpl(InstrumentationModule module) { instrumentationModule = module; classesToInject = new HashMap<>(); proxyFactory = IndyBootstrap.getProxyFactory(module); @@ -39,7 +41,6 @@ public ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyNa return new ProxyBuilder(classToProxy, newProxyName); } - private class ProxyBuilder implements ProxyInjectionBuilder { private final String classToProxy; @@ -52,14 +53,16 @@ private class ProxyBuilder implements ProxyInjectionBuilder { @Override public void inject(InjectionMode mode) { - if(mode != InjectionMode.CLASS_ONLY) { + if (mode != InjectionMode.CLASS_ONLY) { throw new UnsupportedOperationException("Not yet implemented"); } - classesToInject.put(proxyClassName, cl -> { - TypePool typePool = IndyModuleTypePool.get(cl, instrumentationModule); - TypeDescription proxiedType = typePool.describe(classToProxy).resolve(); - return proxyFactory.generateProxy(proxiedType, proxyClassName).getBytes(); - }); + classesToInject.put( + proxyClassName, + cl -> { + TypePool typePool = IndyModuleTypePool.get(cl, instrumentationModule); + TypeDescription proxiedType = typePool.describe(classToProxy).resolve(); + return proxyFactory.generateProxy(proxiedType, proxyClassName).getBytes(); + }); } } } 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 eb0fe050b7d6..2b2d0e4b9475 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -149,7 +149,8 @@ public static HelperInjector forDynamicTypes( for (DynamicType.Unloaded helper : helpers) { bytes.put(helper.getTypeDescription().getName(), cl -> helper.getBytes()); } - return new HelperInjector(requestingName, bytes, Collections.emptyList(), null, instrumentation); + return new HelperInjector( + requestingName, bytes, Collections.emptyList(), null, instrumentation); } public static void setHelperInjectorListener(HelperInjectorListener listener) { @@ -176,11 +177,13 @@ private Map> getHelperMap(ClassLoader targetClassloader }); } } else { - dynamicTypeMap.forEach((name, bytecodeGenerator) -> { - //Eagerly compute bytecode to not risk accidentally holding onto targetClassloader for too long - byte[] bytecode = bytecodeGenerator.apply(targetClassloader); - result.put(name, () -> bytecode); - }); + dynamicTypeMap.forEach( + (name, bytecodeGenerator) -> { + // Eagerly compute bytecode to not risk accidentally holding onto targetClassloader for + // too long + byte[] bytecode = bytecodeGenerator.apply(targetClassloader); + result.put(name, () -> bytecode); + }); } return result; diff --git a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java index 86a01cefd58b..b816605b04c8 100644 --- a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java @@ -12,10 +12,10 @@ import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.util.Collections; -import java.util.List; import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; +import java.util.Collections; +import java.util.List; import net.bytebuddy.asm.Advice; import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument; import net.bytebuddy.asm.Advice.AssignReturned.ToFields.ToField; @@ -46,15 +46,14 @@ public boolean isHelperClass(String className) { @Override public List getAdditionalHelperClassNames() { - //TODO: should not be needed as soon as we automatically add proxied classes to muzzle root set + // TODO: should not be needed as soon as we automatically add proxied classes to the muzzle root + // set return Collections.singletonList("indy.ProxyMe"); } @Override public void injectClasses(ClassInjector injector) { - injector - .proxyBuilder("indy.ProxyMe", "foo.bar.Proxy") - .inject(InjectionMode.CLASS_ONLY); + injector.proxyBuilder("indy.ProxyMe", "foo.bar.Proxy").inject(InjectionMode.CLASS_ONLY); } public static class Instrumentation implements TypeInstrumentation { diff --git a/testing-common/integration-tests/src/main/java/indy/ProxyMe.java b/testing-common/integration-tests/src/main/java/indy/ProxyMe.java index b1444358f144..8ab7387ad30e 100644 --- a/testing-common/integration-tests/src/main/java/indy/ProxyMe.java +++ b/testing-common/integration-tests/src/main/java/indy/ProxyMe.java @@ -1,7 +1,12 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package indy; -import library.MyProxySuperclass; import java.util.concurrent.Callable; +import library.MyProxySuperclass; public class ProxyMe extends MyProxySuperclass implements Callable { @@ -10,7 +15,6 @@ public String call() { return "Hi from ProxyMe"; } - public static String staticHello() { return "Hi from static"; } diff --git a/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java b/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java index c6c14f857f3f..9905c1cc8a0f 100644 --- a/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java +++ b/testing-common/integration-tests/src/test/java/indy/IndyInstrumentationTest.java @@ -8,10 +8,11 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Test; import java.lang.reflect.Field; import java.util.concurrent.Callable; +import library.MyProxySuperclass; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; @SuppressWarnings({"unused", "MethodCanBeStatic"}) public class IndyInstrumentationTest { @@ -113,17 +114,18 @@ void testHelperClassLoading() { assertThat(globalHelper.getClassLoader().getClass().getName()).endsWith("AgentClassLoader"); } - @Test @SuppressWarnings("unchecked") - void testProxyInjection() throws Exception{ + void testProxyInjection() throws Exception { Class proxyClass = Class.forName("foo.bar.Proxy"); - //create an instance and invoke static & non-static methods - //this verifies that our invokedynamic bootstrapping works for constructors, static and non-static methods + // create an instance and invoke static & non-static methods + // this verifies that our invokedynamic bootstrapping works for constructors, static and + // non-static methods Object proxyInstance = proxyClass.getConstructor().newInstance(); assertThat(proxyInstance).isInstanceOf(Callable.class); + assertThat(proxyInstance).isInstanceOf(MyProxySuperclass.class); String invocResult = ((Callable) proxyInstance).call(); assertThat(invocResult).isEqualTo("Hi from ProxyMe"); diff --git a/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java b/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java index 3c8003c099e2..6c17e3d38590 100644 --- a/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java +++ b/testing-common/library-for-integration-tests/src/main/java/library/MyProxySuperclass.java @@ -1,6 +1,12 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package library; /** - * Custom superclass to be used for testing the proxying mechanism. + * Custom superclass only living in the instrumented {@link ClassLoader} to be used for testing the + * proxying mechanism. */ public class MyProxySuperclass {} From 9209cc67b9aceefc9215258413f331aabeee79ed Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 27 Sep 2023 13:47:32 +0200 Subject: [PATCH 03/16] Remove unnecessary subpackage --- .../tooling/instrumentation/InstrumentationModuleInstaller.java | 2 +- .../instrumentation/indy/{injection => }/ClassInjectorImpl.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/{injection => }/ClassInjectorImpl.java (96%) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index 6223a804cdd5..0c64b8a50cf2 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -23,7 +23,7 @@ import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyTypeTransformerImpl; import io.opentelemetry.javaagent.tooling.instrumentation.indy.PatchByteCodeVersionTransformer; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.injection.ClassInjectorImpl; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.ClassInjectorImpl; import io.opentelemetry.javaagent.tooling.muzzle.HelperResourceBuilderImpl; import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle; import io.opentelemetry.javaagent.tooling.util.IgnoreFailedTypeMatcher; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java similarity index 96% rename from javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java rename to javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java index 3088f956293f..dfaac5413403 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/injection/ClassInjectorImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.tooling.instrumentation.indy.injection; +package io.opentelemetry.javaagent.tooling.instrumentation.indy; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; From a54e730fc620c74c3563611d9e048dff4b83fd40 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 27 Sep 2023 14:10:08 +0200 Subject: [PATCH 04/16] Added missing javadoc --- .../instrumentation/injection/ClassInjector.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java index 06029d725170..07cbbda90b66 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java @@ -7,5 +7,18 @@ public interface ClassInjector { + /** + * Create a builder for a proxy class which will be injected into the instrumented {@link + * ClassLoader}. The generated proxy will contain {@code INVOKEDYNAMIC} instructions to invoke the + * proxied class. + * + *

This removes the need for the proxied class and its dependencies to be visible (just like + * Advices) to the instrumented ClassLoader. + * + * @param classToProxy the fully qualified name of the class for which a proxy will be generated + * @param newProxyName the fully qualified name to use for the generated proxy + * @return a builder for further customizing the proxy. {@link + * ProxyInjectionBuilder#inject(InjectionMode)} must be called to actually inject the proxy. + */ ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyName); } From 57da000792b97bfe5aeabd2da9bb13d86ade1011 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 27 Sep 2023 14:57:59 +0200 Subject: [PATCH 05/16] spotless fixes --- .../instrumentation/InstrumentationModuleInstaller.java | 2 +- .../tooling/instrumentation/indy/ClassInjectorImpl.java | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index 0c64b8a50cf2..77cdc9f6fe7c 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -20,10 +20,10 @@ import io.opentelemetry.javaagent.tooling.config.AgentConfig; import io.opentelemetry.javaagent.tooling.field.VirtualFieldImplementationInstaller; import io.opentelemetry.javaagent.tooling.field.VirtualFieldImplementationInstallerFactory; +import io.opentelemetry.javaagent.tooling.instrumentation.indy.ClassInjectorImpl; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry; import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyTypeTransformerImpl; import io.opentelemetry.javaagent.tooling.instrumentation.indy.PatchByteCodeVersionTransformer; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.ClassInjectorImpl; import io.opentelemetry.javaagent.tooling.muzzle.HelperResourceBuilderImpl; import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle; import io.opentelemetry.javaagent.tooling.util.IgnoreFailedTypeMatcher; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java index dfaac5413403..d6668932340f 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java @@ -9,9 +9,6 @@ import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; import io.opentelemetry.javaagent.extension.instrumentation.injection.ProxyInjectionBuilder; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyBootstrap; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleTypePool; -import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyProxyFactory; import java.util.HashMap; import java.util.Map; import java.util.function.Function; From 1aa563e530a29bbadf8174bede5bec0d5b2e2d2c Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 27 Sep 2023 12:14:30 +0200 Subject: [PATCH 06/16] Enable AWS SDK instrumentation for indy via proxy mechanism --- .../awssdk/v2_2/AwsSdkInstrumentationModule.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index a6bb33a49b77..9dacb22cfa89 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -10,6 +10,8 @@ import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; +import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; @AutoService(InstrumentationModule.class) public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule { @@ -26,6 +28,15 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) helperResourceBuilder.register("software/amazon/awssdk/global/handlers/execution.interceptors"); } + @Override + public void injectClasses(ClassInjector injector) { + injector + .proxyBuilder( + "io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor", + "io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor") + .inject(InjectionMode.CLASS_ONLY); + } + @Override void doTransform(TypeTransformer transformer) { // Nothing to transform, this type instrumentation is only used for injecting resources. From d48563cc621a24949a015eb83a6a84f83278ab18 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 2 Oct 2023 11:33:53 +0200 Subject: [PATCH 07/16] Moved injection to new ExtendedInstrumentationModule --- .../v2_2/AwsSdkInstrumentationModule.java | 8 +++--- .../InstrumentationModule.java | 12 --------- .../injection/ProxyInjectionBuilder.java | 11 -------- .../injection/ClassInjector.java | 6 ++++- .../ExtendedInstrumentationModule.java | 27 +++++++++++++++++++ .../injection/InjectionMode.java | 6 ++++- .../injection/ProxyInjectionBuilder.java | 15 +++++++++++ .../InstrumentationModuleInstaller.java | 6 ++++- .../indy/ClassInjectorImpl.java | 6 ++--- .../indy/IndyInstrumentationTestModule.java | 8 +++--- 10 files changed, 70 insertions(+), 35 deletions(-) delete mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java rename javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/{ => internal}/injection/ClassInjector.java (82%) create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java rename javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/{ => internal}/injection/InjectionMode.java (62%) create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ProxyInjectionBuilder.java diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 9dacb22cfa89..31d15490ffd7 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -10,11 +10,13 @@ import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; -import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; @AutoService(InstrumentationModule.class) -public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule { +public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule + implements ExtendedInstrumentationModule { public AwsSdkInstrumentationModule() { super("aws-sdk-2.2-core"); } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java index 6e9cbaccb884..9d8bb78b8200 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java @@ -10,7 +10,6 @@ import static net.bytebuddy.matcher.ElementMatchers.any; import io.opentelemetry.javaagent.bootstrap.internal.ExperimentalConfig; -import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; import io.opentelemetry.sdk.autoconfigure.spi.Ordered; import java.util.Collections; @@ -160,15 +159,4 @@ public ElementMatcher.Junction classLoaderMatcher() { public List getAdditionalHelperClassNames() { return Collections.emptyList(); } - - /** - * Only functional for Modules where {@link #isIndyModule()} returns {@code true}. - * - *

Normally, helper and advice classes are loaded in a child classloader of the instrumented - * classloader. This method allows to inject classes directly into the instrumented classloader - * instead. - * - * @param injector the builder for injecting classes - */ - public void injectClasses(ClassInjector injector) {} } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java deleted file mode 100644 index 38f8e69686ad..000000000000 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ProxyInjectionBuilder.java +++ /dev/null @@ -1,11 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.extension.instrumentation.injection; - -public interface ProxyInjectionBuilder { - - void inject(InjectionMode mode); -} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java similarity index 82% rename from javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java rename to javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java index 07cbbda90b66..ebc10d9c87a6 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/ClassInjector.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java @@ -3,8 +3,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.extension.instrumentation.injection; +package io.opentelemetry.javaagent.extension.instrumentation.internal.injection; +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ public interface ClassInjector { /** diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java new file mode 100644 index 000000000000..6c98ff4a208e --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java @@ -0,0 +1,27 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.extension.instrumentation.internal.injection; + +import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; + +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ +public interface ExtendedInstrumentationModule { + + /** + * Only functional for Modules where {@link InstrumentationModule#isIndyModule()} returns {@code + * true}. + * + *

Normally, helper and advice classes are loaded in a child classloader of the instrumented + * classloader. This method allows to inject classes directly into the instrumented classloader + * instead. + * + * @param injector the builder for injecting classes + */ + default void injectClasses(ClassInjector injector) {} +} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/InjectionMode.java similarity index 62% rename from javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java rename to javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/InjectionMode.java index 39e38ad27864..b78759a9f65f 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/injection/InjectionMode.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/InjectionMode.java @@ -3,8 +3,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.extension.instrumentation.injection; +package io.opentelemetry.javaagent.extension.instrumentation.internal.injection; +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ public enum InjectionMode { CLASS_ONLY // TODO: implement the modes RESOURCE_ONLY and CLASS_AND_RESOURCE diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ProxyInjectionBuilder.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ProxyInjectionBuilder.java new file mode 100644 index 000000000000..23d5237aabda --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ProxyInjectionBuilder.java @@ -0,0 +1,15 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.extension.instrumentation.internal.injection; + +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ +public interface ProxyInjectionBuilder { + + void inject(InjectionMode mode); +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index 77cdc9f6fe7c..0e649d7cdc24 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -13,6 +13,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; import io.opentelemetry.javaagent.tooling.HelperInjector; import io.opentelemetry.javaagent.tooling.TransformSafeLogger; import io.opentelemetry.javaagent.tooling.Utils; @@ -83,7 +84,10 @@ private AgentBuilder installIndyModule( instrumentationModule.registerHelperResources(helperResourceBuilder); ClassInjectorImpl injectedClassesCollector = new ClassInjectorImpl(instrumentationModule); - instrumentationModule.injectClasses(injectedClassesCollector); + if (instrumentationModule instanceof ExtendedInstrumentationModule) { + ((ExtendedInstrumentationModule) instrumentationModule) + .injectClasses(injectedClassesCollector); + } AgentBuilder.Transformer helperInjector = new HelperInjector( diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java index d6668932340f..b3d5e9f047d3 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ClassInjectorImpl.java @@ -6,9 +6,9 @@ package io.opentelemetry.javaagent.tooling.instrumentation.indy; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; -import io.opentelemetry.javaagent.extension.instrumentation.injection.ClassInjector; -import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; -import io.opentelemetry.javaagent.extension.instrumentation.injection.ProxyInjectionBuilder; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ProxyInjectionBuilder; import java.util.HashMap; import java.util.Map; import java.util.function.Function; diff --git a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java index b816605b04c8..51700e7dc0ca 100644 --- a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java @@ -12,8 +12,9 @@ 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.injection.ClassInjector; -import io.opentelemetry.javaagent.extension.instrumentation.injection.InjectionMode; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; import java.util.Collections; import java.util.List; import net.bytebuddy.asm.Advice; @@ -23,7 +24,8 @@ import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) -public class IndyInstrumentationTestModule extends InstrumentationModule { +public class IndyInstrumentationTestModule extends InstrumentationModule + implements ExtendedInstrumentationModule { public IndyInstrumentationTestModule() { super("indy-test"); From 43fc61869b7bcfbb0ab760d30ab80c04af62e093 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 2 Oct 2023 11:45:30 +0200 Subject: [PATCH 08/16] Javadoc review fixes --- .../instrumentation/internal/injection/ClassInjector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java index ebc10d9c87a6..30b415e1df13 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java @@ -13,8 +13,8 @@ public interface ClassInjector { /** * Create a builder for a proxy class which will be injected into the instrumented {@link - * ClassLoader}. The generated proxy will contain {@code INVOKEDYNAMIC} instructions to invoke the - * proxied class. + * ClassLoader}. The generated proxy will delegate to the original class, which is loaded in a + * separate classloader. * *

This removes the need for the proxied class and its dependencies to be visible (just like * Advices) to the instrumented ClassLoader. From 5adf59e94e0b308c66c57053cf739dc3eb18956a Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 2 Oct 2023 13:08:42 +0200 Subject: [PATCH 09/16] Fix AWS tests for indy, enable for testing with indy flag --- .../AbstractAwsSdkInstrumentationModule.java | 5 --- .../v2_2/AwsSdkInstrumentationModule.java | 32 +++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java index 5dc5f2ab9d4f..25b7b5c06c38 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java @@ -27,11 +27,6 @@ public boolean isHelperClass(String className) { return className.startsWith("io.opentelemetry.contrib.awsxray."); } - @Override - public boolean isIndyModule() { - return false; - } - @Override public ElementMatcher.Junction classLoaderMatcher() { // We don't actually transform it but want to make sure we only apply the instrumentation when diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 31d15490ffd7..1e5fc65b8114 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -13,6 +13,9 @@ import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; @AutoService(InstrumentationModule.class) public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule @@ -21,6 +24,35 @@ public AwsSdkInstrumentationModule() { super("aws-sdk-2.2-core"); } + @Override + @SuppressWarnings("unchecked") + public List getAdditionalHelperClassNames() { + if (isIndyModule()) { + // With the invokedynamic approach, the SnsInstrumentationModule and SqsInstrumentationModule + // are not required anymore, because we don't inject the helpers which reference potentially + // missing classes + // Instead, those are loaded by the InstrumentationModuleClassloader and LinkageErrors are + // caught just like when using those classes as library instrumentation + List helpers = new ArrayList<>(); + InstrumentationModule[] modules = { + new SnsInstrumentationModule(), new SqsInstrumentationModule() + }; + for (InstrumentationModule include : modules) { + try { + List moduleRefs = + (List) + include.getClass().getDeclaredMethod("getMuzzleHelperClassNames").invoke(include); + helpers.addAll(moduleRefs); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } + return helpers; + } else { + return Collections.emptyList(); + } + } + /** * Injects resource file with reference to our {@link TracingExecutionInterceptor} to allow SDK's * service loading mechanism to pick it up. From ac777802de6f3acacd29c9421bea60f455649a0e Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 4 Oct 2023 09:58:23 +0200 Subject: [PATCH 10/16] Moved and renamed ExtendedInstrumentationModule --- .../awssdk/v2_2/AwsSdkInstrumentationModule.java | 4 ++-- ...onModule.java => ExperimentalInstrumentationModule.java} | 5 +++-- .../instrumentation/InstrumentationModuleInstaller.java | 6 +++--- .../src/main/java/indy/IndyInstrumentationTestModule.java | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) rename javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/{injection/ExtendedInstrumentationModule.java => ExperimentalInstrumentationModule.java} (84%) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 1e5fc65b8114..3570bfb9a201 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -11,7 +11,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; -import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; import java.util.ArrayList; import java.util.Collections; @@ -19,7 +19,7 @@ @AutoService(InstrumentationModule.class) public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule - implements ExtendedInstrumentationModule { + implements ExperimentalInstrumentationModule { public AwsSdkInstrumentationModule() { super("aws-sdk-2.2-core"); } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java similarity index 84% rename from javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java rename to javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java index 6c98ff4a208e..bfd0a620166e 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ExtendedInstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java @@ -3,15 +3,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.extension.instrumentation.internal.injection; +package io.opentelemetry.javaagent.extension.instrumentation.internal; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; /** * This class is internal and is hence not for public use. Its APIs are unstable and can change at * any time. */ -public interface ExtendedInstrumentationModule { +public interface ExperimentalInstrumentationModule { /** * Only functional for Modules where {@link InstrumentationModule#isIndyModule()} returns {@code diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java index 0e649d7cdc24..23c293d1c142 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java @@ -13,7 +13,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; -import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import io.opentelemetry.javaagent.tooling.HelperInjector; import io.opentelemetry.javaagent.tooling.TransformSafeLogger; import io.opentelemetry.javaagent.tooling.Utils; @@ -84,8 +84,8 @@ private AgentBuilder installIndyModule( instrumentationModule.registerHelperResources(helperResourceBuilder); ClassInjectorImpl injectedClassesCollector = new ClassInjectorImpl(instrumentationModule); - if (instrumentationModule instanceof ExtendedInstrumentationModule) { - ((ExtendedInstrumentationModule) instrumentationModule) + if (instrumentationModule instanceof ExperimentalInstrumentationModule) { + ((ExperimentalInstrumentationModule) instrumentationModule) .injectClasses(injectedClassesCollector); } diff --git a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java index 51700e7dc0ca..1871cbb188ef 100644 --- a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java @@ -13,7 +13,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; -import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ExtendedInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; import java.util.Collections; import java.util.List; @@ -25,7 +25,7 @@ @AutoService(InstrumentationModule.class) public class IndyInstrumentationTestModule extends InstrumentationModule - implements ExtendedInstrumentationModule { + implements ExperimentalInstrumentationModule { public IndyInstrumentationTestModule() { super("indy-test"); From 6b9f958d85c9ca72b798c9bf939237c6358b5f45 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 4 Oct 2023 10:02:58 +0200 Subject: [PATCH 11/16] Added default method for generating proxy with same name --- .../awssdk/v2_2/AwsSdkInstrumentationModule.java | 2 +- .../internal/injection/ClassInjector.java | 11 +++++++++++ .../main/java/indy/IndyInstrumentationTestModule.java | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 3570bfb9a201..246a38b69130 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -10,8 +10,8 @@ import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; 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 java.util.ArrayList; import java.util.Collections; diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java index 30b415e1df13..9a11cbd6c689 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/injection/ClassInjector.java @@ -25,4 +25,15 @@ public interface ClassInjector { * ProxyInjectionBuilder#inject(InjectionMode)} must be called to actually inject the proxy. */ ProxyInjectionBuilder proxyBuilder(String classToProxy, String newProxyName); + + /** + * Same as invoking {@link #proxyBuilder(String, String)}, but the resulting proxy will have the + * same name as the proxied class. + * + * @param classToProxy the fully qualified name of the class for which a proxy will be generated + * @return a builder for further customizing and injecting the proxy + */ + default ProxyInjectionBuilder proxyBuilder(String classToProxy) { + return proxyBuilder(classToProxy, classToProxy); + } } diff --git a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java index 1871cbb188ef..12089bcde530 100644 --- a/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/indy/IndyInstrumentationTestModule.java @@ -12,8 +12,8 @@ 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.injection.ClassInjector; 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 java.util.Collections; import java.util.List; From 466ca1be517bf2d0f498a77a8bc62bc0b07ccff2 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 4 Oct 2023 10:17:12 +0200 Subject: [PATCH 12/16] Revert AWS changes --- .../AbstractAwsSdkInstrumentationModule.java | 5 ++ .../v2_2/AwsSdkInstrumentationModule.java | 47 +------------------ 2 files changed, 6 insertions(+), 46 deletions(-) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java index 25b7b5c06c38..5dc5f2ab9d4f 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java @@ -27,6 +27,11 @@ public boolean isHelperClass(String className) { return className.startsWith("io.opentelemetry.contrib.awsxray."); } + @Override + public boolean isIndyModule() { + return false; + } + @Override public ElementMatcher.Junction classLoaderMatcher() { // We don't actually transform it but want to make sure we only apply the instrumentation when diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 246a38b69130..a6bb33a49b77 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -10,49 +10,13 @@ import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -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 java.util.ArrayList; -import java.util.Collections; -import java.util.List; @AutoService(InstrumentationModule.class) -public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule - implements ExperimentalInstrumentationModule { +public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule { public AwsSdkInstrumentationModule() { super("aws-sdk-2.2-core"); } - @Override - @SuppressWarnings("unchecked") - public List getAdditionalHelperClassNames() { - if (isIndyModule()) { - // With the invokedynamic approach, the SnsInstrumentationModule and SqsInstrumentationModule - // are not required anymore, because we don't inject the helpers which reference potentially - // missing classes - // Instead, those are loaded by the InstrumentationModuleClassloader and LinkageErrors are - // caught just like when using those classes as library instrumentation - List helpers = new ArrayList<>(); - InstrumentationModule[] modules = { - new SnsInstrumentationModule(), new SqsInstrumentationModule() - }; - for (InstrumentationModule include : modules) { - try { - List moduleRefs = - (List) - include.getClass().getDeclaredMethod("getMuzzleHelperClassNames").invoke(include); - helpers.addAll(moduleRefs); - } catch (Exception e) { - throw new IllegalStateException(e); - } - } - return helpers; - } else { - return Collections.emptyList(); - } - } - /** * Injects resource file with reference to our {@link TracingExecutionInterceptor} to allow SDK's * service loading mechanism to pick it up. @@ -62,15 +26,6 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) helperResourceBuilder.register("software/amazon/awssdk/global/handlers/execution.interceptors"); } - @Override - public void injectClasses(ClassInjector injector) { - injector - .proxyBuilder( - "io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor", - "io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor") - .inject(InjectionMode.CLASS_ONLY); - } - @Override void doTransform(TypeTransformer transformer) { // Nothing to transform, this type instrumentation is only used for injecting resources. From 1b3352b1ef3eb85cd6b4af448b96fc5aab02823b Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 4 Oct 2023 10:56:30 +0200 Subject: [PATCH 13/16] Added proxy in log4j 2.17 instrumentation --- .../v2_17/Log4j2InstrumentationModule.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java b/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java index f68ded00e4f8..efc761cf1d7e 100644 --- a/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java +++ b/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java @@ -15,11 +15,15 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.List; +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 net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) -public class Log4j2InstrumentationModule extends InstrumentationModule { +public class Log4j2InstrumentationModule extends InstrumentationModule implements + ExperimentalInstrumentationModule { public Log4j2InstrumentationModule() { super("log4j-context-data", "log4j-context-data-2.17"); } @@ -31,8 +35,9 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) } @Override - public boolean isIndyModule() { - return false; + public void injectClasses(ClassInjector injector) { + injector.proxyBuilder("io.opentelemetry.instrumentation.log4j.contextdata.v2_17.OpenTelemetryContextDataProvider") + .inject(InjectionMode.CLASS_ONLY); } @Override From 85829bbd1059ea9876d3d1727eec1a2ce756a792 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 4 Oct 2023 10:59:56 +0200 Subject: [PATCH 14/16] Spotless fixes --- .../contextdata/v2_17/Log4j2InstrumentationModule.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java b/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java index efc761cf1d7e..238122e7fd7c 100644 --- a/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java +++ b/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/contextdata/v2_17/Log4j2InstrumentationModule.java @@ -14,16 +14,16 @@ import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.util.List; 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 java.util.List; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) -public class Log4j2InstrumentationModule extends InstrumentationModule implements - ExperimentalInstrumentationModule { +public class Log4j2InstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public Log4j2InstrumentationModule() { super("log4j-context-data", "log4j-context-data-2.17"); } @@ -36,7 +36,9 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) @Override public void injectClasses(ClassInjector injector) { - injector.proxyBuilder("io.opentelemetry.instrumentation.log4j.contextdata.v2_17.OpenTelemetryContextDataProvider") + injector + .proxyBuilder( + "io.opentelemetry.instrumentation.log4j.contextdata.v2_17.OpenTelemetryContextDataProvider") .inject(InjectionMode.CLASS_ONLY); } From cbe27b3344bcc52f3dc0f0ef3aafef912d40315d Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 18 Oct 2023 11:25:16 +0200 Subject: [PATCH 15/16] Remove type pool caching --- .../indy/IndyModuleTypePool.java | 56 ++----------------- .../InstrumentationModuleClassLoader.java | 26 +-------- 2 files changed, 7 insertions(+), 75 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java index 297303a77b64..056f88536f3a 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java @@ -5,33 +5,14 @@ package io.opentelemetry.javaagent.tooling.instrumentation.indy; -import io.opentelemetry.instrumentation.api.internal.cache.Cache; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.tooling.muzzle.AgentTooling; -import java.util.concurrent.ConcurrentHashMap; import net.bytebuddy.pool.TypePool; public class IndyModuleTypePool { private IndyModuleTypePool() {} - /** - * We implement the TypePool by delegating the .class resource lookup to actual dummy {@link - * InstrumentationModuleClassLoader} instances. We cache those dummy instances so that repeated - * calls to {@link #get(ClassLoader, InstrumentationModule)} share the underlying cache for {@link - * net.bytebuddy.description.type.TypeDescription}s. Note that it is important that the {@link - * InstrumentationModuleClassLoader} only weakly references the {@link ClassLoader} being - * instrumented, which is also used as the key of the cache. Otherwise we would accidentally - * create a strong reference, preventing the instrumented {@link ClassLoader} from being ever - * GCed. To ensure that the cached {@link InstrumentationModuleClassLoader} is only used for - * resource lookups and not for classloading, it is wrapped in a {@link ResourceOnlyClassLoader}. - * Loading classes could otherwise cause the {@link InstrumentationModuleClassLoader} to strongly - * reference the instrumented {@link ClassLoader}. - */ - private static final ConcurrentHashMap< - InstrumentationModule, Cache> - classloadersForTypePools = new ConcurrentHashMap<>(); - /** * Provides a {@link TypePool} which has the same lookup rules for {@link * net.bytebuddy.description.type.TypeDescription}s as {@link InstrumentationModuleClassLoader} @@ -40,41 +21,14 @@ private IndyModuleTypePool() {} * @param instrumentedCl the classloader being instrumented (e.g. for which the {@link * InstrumentationModuleClassLoader} is being created). * @param module the {@link InstrumentationModule} performing the instrumentation - * @return the type pool + * @return the type pool, must not be cached! */ public static TypePool get(ClassLoader instrumentedCl, InstrumentationModule module) { - - Cache cacheForModule = - classloadersForTypePools.computeIfAbsent(module, (k) -> Cache.weak()); - - ResourceOnlyClassLoader resourceOnlyCl = - cacheForModule.computeIfAbsent( - instrumentedCl, - cl -> - new ResourceOnlyClassLoader( - IndyModuleRegistry.createInstrumentationModuleClassloader(module, cl))); - - // TODO: this implementation makes the cache for TypeDescriptions somewhat inefficient - // The returned pool will cache all returned TypeDescriptions as if they were loaded by the - // dummy InstrumentationModuleClassLoader, - // even the classes which are actually loaded by the instrumented Classloader or the agent - // Classloader + // TODO: this implementation doesn't allow caching the returned bool and its types // This could be improved by implementing a custom TypePool instead, which delegates to parent // TypePools and mirrors the delegation model of the InstrumentationModuleClassLoader - return AgentTooling.poolStrategy() - .typePool(AgentTooling.locationStrategy().classFileLocator(resourceOnlyCl), resourceOnlyCl); - } - - private static class ResourceOnlyClassLoader extends ClassLoader { - - public ResourceOnlyClassLoader(ClassLoader delegate) { - super(delegate); - } - - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - throw new UnsupportedOperationException( - "This classloader should only be used for resource loading!"); - } + InstrumentationModuleClassLoader dummyCl = + IndyModuleRegistry.createInstrumentationModuleClassloader(module, instrumentedCl); + return TypePool.Default.of(AgentTooling.locationStrategy().classFileLocator(dummyCl)); } } 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 9e99fc903d50..30641430464f 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 @@ -9,7 +9,6 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; -import java.lang.ref.WeakReference; import java.net.URL; import java.security.PrivilegedAction; import java.security.ProtectionDomain; @@ -54,17 +53,7 @@ public class InstrumentationModuleClassLoader extends ClassLoader { private final ClassLoader agentOrExtensionCl; private volatile MethodHandles.Lookup cachedLookup; - /** - * We only weakly reference the instrumented classloader to make sure that we don't prevent it - * from being GCed. This only works as long as this {@link InstrumentationModuleClassLoader} has - * not loaded any classes referencing types from the instrumentedCl yet: As soon as such a class - * is loaded, it will strongly reference the instrumentedCl. - * - *

For this reason, users of {@link InstrumentationModuleClassLoader} must make sure that they - * only weakly reference it if it is actually used for classloading and not for resource lookups - * (e.g. via {@link net.bytebuddy.pool.TypePool}s. - */ - private final WeakReference instrumentedClassloader; + private final ClassLoader instrumentedCl; public InstrumentationModuleClassLoader( ClassLoader instrumentedCl, @@ -74,16 +63,7 @@ public InstrumentationModuleClassLoader( super(agentOrExtensionCl); additionalInjectedClasses = injectedClasses; this.agentOrExtensionCl = agentOrExtensionCl; - this.instrumentedClassloader = new WeakReference<>(instrumentedCl); - } - - private ClassLoader getInstrumentedClassloader() { - ClassLoader classLoader = instrumentedClassloader.get(); - if (classLoader == null) { - throw new IllegalStateException( - "The referenced instrumented ClassLoader has already been GCed!"); - } - return classLoader; + this.instrumentedCl = instrumentedCl; } /** @@ -110,7 +90,6 @@ public MethodHandles.Lookup getLookup() { @Override @SuppressWarnings("removal") // AccessController is deprecated for removal protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - ClassLoader instrumentedCl = getInstrumentedClassloader(); synchronized (getClassLoadingLock(name)) { Class result = findLoadedClass(name); @@ -159,7 +138,6 @@ private static Class tryLoad(ClassLoader cl, String name) { @Override public URL getResource(String resourceName) { - ClassLoader instrumentedCl = getInstrumentedClassloader(); String className = resourceToClassName(resourceName); if (className == null) { // delegate to just the default parent (the agent classloader) From 0a3b73604f772164c140470782d423ad354d5185 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 18 Oct 2023 15:54:55 +0200 Subject: [PATCH 16/16] typo fix --- .../tooling/instrumentation/indy/IndyModuleTypePool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java index 056f88536f3a..7cf63528a870 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleTypePool.java @@ -24,7 +24,7 @@ private IndyModuleTypePool() {} * @return the type pool, must not be cached! */ public static TypePool get(ClassLoader instrumentedCl, InstrumentationModule module) { - // TODO: this implementation doesn't allow caching the returned bool and its types + // TODO: this implementation doesn't allow caching the returned pool and its types // This could be improved by implementing a custom TypePool instead, which delegates to parent // TypePools and mirrors the delegation model of the InstrumentationModuleClassLoader InstrumentationModuleClassLoader dummyCl =