Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make more tests run with indy #9729

Merged
merged 2 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@
package io.opentelemetry.javaagent.instrumentation.couchbase.v2_0;

import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;

import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import java.util.List;

@AutoService(InstrumentationModule.class)
public class CouchbaseInstrumentationModule extends InstrumentationModule {
public class CouchbaseInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {

public CouchbaseInstrumentationModule() {
super("couchbase", "couchbase-2.0");
}
Expand All @@ -24,13 +28,12 @@ public boolean isHelperClass(String className) {
}

@Override
public boolean isIndyModule() {
// rx.__OpenTelemetryTracingUtil is used for accessing a package private field
return false;
public List<TypeInstrumentation> typeInstrumentations() {
return asList(new CouchbaseBucketInstrumentation(), new CouchbaseClusterInstrumentation());
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return asList(new CouchbaseBucketInstrumentation(), new CouchbaseClusterInstrumentation());
public List<String> injectedClassNames() {
return singletonList("rx.__OpenTelemetryTracingUtil");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import java.util.List;

@AutoService(InstrumentationModule.class)
public class HystrixInstrumentationModule extends InstrumentationModule {
public class HystrixInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {

public HystrixInstrumentationModule() {
super("hystrix", "hystrix-1.4");
Expand All @@ -25,13 +27,12 @@ public boolean isHelperClass(String className) {
}

@Override
public boolean isIndyModule() {
// rx.__OpenTelemetryTracingUtil is used for accessing a package private field
return false;
public List<TypeInstrumentation> typeInstrumentations() {
return singletonList(new HystrixCommandInstrumentation());
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return singletonList(new HystrixCommandInstrumentation());
public List<String> injectedClassNames() {
return singletonList("rx.__OpenTelemetryTracingUtil");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@ public boolean isHelperClass(String className) {
return className.startsWith("io.opentelemetry.extension.kotlin.");
}

@Override
public boolean isIndyModule() {
// java.lang.LinkageError: loader constraint violation in interface itable initialization for
// class
// io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion: when selecting method 'java.lang.Object io.ktor.client.plugins.HttpClientPlugin.prepare(kotlin.jvm.functions.Function1)' the class loader 'app' for super interface io.ktor.client.plugins.HttpClientPlugin, and the class loader io.opentelemetry.javaagent.tooling.instrumentation.indy.InstrumentationModuleClassLoader @2565a7d0 of the selected method's class, io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion have different Class objects for the type kotlin.jvm.functions.Function1 used in the signature (io.ktor.client.plugins.HttpClientPlugin is in unnamed module of loader 'app'; io.opentelemetry.javaagent.shaded.instrumentation.ktor.v2_0.client.KtorClientTracing$Companion is in unnamed module of loader io.opentelemetry.javaagent.tooling.instrumentation.indy.InstrumentationModuleClassLoader @2565a7d0, parent loader io.opentelemetry.javaagent.bootstrap.AgentClassLoader @ea30797)
return false;
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return asList(new ServerInstrumentation(), new HttpClientInstrumentation());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@ public OkHttp3InstrumentationModule() {
super("okhttp", "okhttp-3.0");
}

@Override
public boolean isIndyModule() {
// java.lang.LinkageError: bad method type alias: (Builder,Map)void not visible from class
// io.opentelemetry.javaagent.instrumentation.okhttp.v3_0.OkHttp3Instrumentation$ConstructorAdvice
return false;
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return asList(new OkHttp3Instrumentation(), new OkHttp3DispatcherInstrumentation());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,17 @@
package io.opentelemetry.javaagent.instrumentation.vertx.v4_0.sql;

import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;

import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import java.util.List;

@AutoService(InstrumentationModule.class)
public class VertxSqlClientInstrumentationModule extends InstrumentationModule {
public class VertxSqlClientInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {

public VertxSqlClientInstrumentationModule() {
super("vertx-sql-client", "vertx-sql-client-4.0", "vertx");
Expand All @@ -25,9 +28,8 @@ public boolean isHelperClass(String className) {
}

@Override
public boolean isIndyModule() {
// QueryExecutorUtil accesses package private class io.vertx.sqlclient.impl.QueryExecutor
return false;
public List<String> injectedClassNames() {
return singletonList("io.vertx.sqlclient.impl.QueryExecutorUtil");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@

package io.opentelemetry.javaagent.extension.instrumentation.internal;

import static java.util.Collections.emptyList;

import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector;
import java.util.List;

/**
* This class is internal and is hence not for public use. Its APIs are unstable and can change at
Expand All @@ -25,4 +28,12 @@ public interface ExperimentalInstrumentationModule {
* @param injector the builder for injecting classes
*/
default void injectClasses(ClassInjector injector) {}

/**
* Returns a list of helper classes that will be defined in the class loader of the instrumented
* library.
*/
default List<String> injectedClassNames() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would have preferred to add a injector.copy(String classname).inject(InjectionMode.CLASS_ONLY) method to ClassInjector than a separate method.

This would allow to see all injected classes (proxies and copies) with a glance at the injectClasses method, allow the injection of the bytecode as a resource if needed and easier future extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can change this later. I implemented most of this before the pr with ClassInjector was merged. My hope is that if we create enough of a mess with the apis then @mateuszrzeszutek will come and clean all of this up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me, hopefully for @mateuszrzeszutek too 😆

mateuszrzeszutek marked this conversation as resolved.
Show resolved Hide resolved
return emptyList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@
import io.opentelemetry.javaagent.tooling.util.NamedMatcher;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.lang.instrument.Instrumentation;
import java.security.ProtectionDomain;
import java.util.Collections;
import java.util.List;
import net.bytebuddy.agent.builder.AgentBuilder;
import net.bytebuddy.description.annotation.AnnotationSource;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
import net.bytebuddy.utility.JavaModule;

public final class InstrumentationModuleInstaller {

Expand Down Expand Up @@ -69,44 +72,87 @@ AgentBuilder install(
}

if (instrumentationModule.isIndyModule()) {
return installIndyModule(instrumentationModule, parentAgentBuilder);
return installIndyModule(instrumentationModule, parentAgentBuilder, config);
} else {
return installInjectingModule(instrumentationModule, parentAgentBuilder, config);
}
}

private AgentBuilder installIndyModule(
InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder) {

IndyModuleRegistry.registerIndyModule(instrumentationModule);

InstrumentationModule instrumentationModule,
AgentBuilder parentAgentBuilder,
ConfigProperties config) {
List<String> helperClassNames =
InstrumentationModuleMuzzle.getHelperClassNames(instrumentationModule);
HelperResourceBuilderImpl helperResourceBuilder = new HelperResourceBuilderImpl();
instrumentationModule.registerHelperResources(helperResourceBuilder);
List<TypeInstrumentation> typeInstrumentations = instrumentationModule.typeInstrumentations();
if (typeInstrumentations.isEmpty()) {
if (!helperClassNames.isEmpty() || !helperResourceBuilder.getResources().isEmpty()) {
logger.log(
WARNING,
"Helper classes and resources won't be injected if no types are instrumented: {0}",
instrumentationModule.instrumentationName());
}

return parentAgentBuilder;
}

List<String> injectedHelperClassNames = Collections.emptyList();
if (instrumentationModule instanceof ExperimentalInstrumentationModule) {
ExperimentalInstrumentationModule experimentalInstrumentationModule =
(ExperimentalInstrumentationModule) instrumentationModule;
injectedHelperClassNames = experimentalInstrumentationModule.injectedClassNames();
helperClassNames.removeAll(injectedHelperClassNames);
laurit marked this conversation as resolved.
Show resolved Hide resolved
}

IndyModuleRegistry.registerIndyModule(instrumentationModule);

ClassInjectorImpl injectedClassesCollector = new ClassInjectorImpl(instrumentationModule);
if (instrumentationModule instanceof ExperimentalInstrumentationModule) {
((ExperimentalInstrumentationModule) instrumentationModule)
.injectClasses(injectedClassesCollector);
}

// TODO (Jonas): Adapt MuzzleMatcher to use the same type lookup strategy as the
laurit marked this conversation as resolved.
Show resolved Hide resolved
// InstrumentationModuleClassLoader (see IndyModuleTypePool)
MuzzleMatcher muzzleMatcher =
new MuzzleMatcher(logger, instrumentationModule, config) {
@Override
public boolean matches(
TypeDescription typeDescription,
ClassLoader classLoader,
JavaModule module,
Class<?> classBeingRedefined,
ProtectionDomain protectionDomain) {
ClassLoader instrumentationClassLoader =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my understanding of AgentCachingPoolStrategy is correct, our TypeDescription-Cache ensures that we never hold a strong reference to classloaders, correct?

As a result, the caching won't really work here and it will also cause good entries to be evicted from the cache if it is fixed-size due to "flooding" the cache with temporary classloader.

I think we could avoid these problems and keep the muzzle-matching efficient by instead altering ReferenceMatcher.createTypePool:

// loader cannot be null, must pass "bootstrap proxy" instead of bootstrap class loader
  private TypePool createTypePool(ClassLoader loader) {
    // ok to use locationStrategy() without fallback bootstrap proxy here since loader is non-null
    TypePool instrumentedClPool = AgentTooling.poolStrategy()
        .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
    if(instrumentationModule.isIndy()) {
      TypePool instrumentedClPool = AgentTooling.poolStrategy()
          .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
      ClassLoader moduleOrAgentCl = instrumentationModule.getClass().getClassLoader();
      TypePool agentOrExtensionPool = AgentTooling.poolStrategy()
          .typePool(AgentTooling.locationStrategy().classFileLocator(moduleOrAgentCl), moduleOrAgentCl);
      return new TypePool() {
        @Override
        public Resolution describe(String name) {
          if(name.startsWith("io.opentelemetry.javaagent")) {
            return agentOrExtensionPool.describe(name);
          }
          return instrumentedClPool.describe(name);
        }

        @Override
        public void clear() {
          instrumentedClPool.clear();
          agentOrExtensionPool.clear();
        }
      };
    }
    return instrumentedClPool;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require further investigation. As far as I understand we don't pass the temporary class loader here but the real instrumentation loader. Keep in mind that having to transform a class is really a special case, relatively few classes get transformed. It should be fine as long as loading classes that don't get transformed does minimal work.

Copy link
Contributor

@JonasKunz JonasKunz Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require further investigation. As far as I understand we don't pass the temporary class loader here but the real instrumentation loader.

Correct, but we make sure that we never hold a strong reference to that InstrumentationModuleClassloader, because that would result in a strong reference to the instrumented application classloader.

The InstrumentationModuleClassloader will only be strongly-referenced when the first invokedynamic-instruction is bootstrapped, therefore it may be GCed until this happens.

Keep in mind that having to transform a class is really a special case, relatively few classes get transformed. It should be fine as long as loading classes that don't get transformed does minimal work.

Makes sense, so we can keep it as is for now. I just thought that my proposed alternative with the custom TypePool might avoid these issues altogether, but we don't need to do this now if it is too much effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to insert a strong reference from application class loader to InstrumentationModuleClassloader so that InstrumentationModuleClassloader couldn't be gcd before the application class loader? Could do this be defining a class in application class loader that could keep the reference in a static field or map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is worth the effort and complexity now, because IINM the custom TypePool I suggested would still be better and imo less complex: When caching TypeDescriptions the corresponding classloader is used as part of the cache-key. If we use the InstrumentationModuleClassloader here, this will only allow the cache to be effective for classes from the same classloader when checking for the same InstrumentationModule.

If we instead use something like the custom type pool I suggested, the TypeDescriptions will instead be associated with either the application classloader or the agent/extension classloader, depending where they come from, allowing a better "reuse" in theory.

I think for now the "risk" of the InstrumentationModuleCL being GCed here is okay. If you want to, I can do a follow-up PR trying to implement that custom type pool.

IndyModuleRegistry.getInstrumentationClassloader(
instrumentationModule.getClass().getName(), classLoader);
laurit marked this conversation as resolved.
Show resolved Hide resolved
return super.matches(
typeDescription,
instrumentationClassLoader,
module,
classBeingRedefined,
protectionDomain);
}
};
AgentBuilder.Transformer helperInjector =
new HelperInjector(
instrumentationModule.instrumentationName(),
injectedHelperClassNames,
injectedClassesCollector.getClassesToInject(),
helperResourceBuilder.getResources(),
instrumentationModule.getClass().getClassLoader(),
instrumentation);

// TODO (Jonas): Adapt MuzzleMatcher to use the same type lookup strategy as the
// InstrumentationModuleClassLoader (see IndyModuleTypePool)
// MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);
VirtualFieldImplementationInstaller contextProvider =
virtualFieldInstallerFactory.create(instrumentationModule);

AgentBuilder agentBuilder = parentAgentBuilder;
for (TypeInstrumentation typeInstrumentation : instrumentationModule.typeInstrumentations()) {
AgentBuilder.Identified.Extendable extendableAgentBuilder =
setTypeMatcher(agentBuilder, instrumentationModule, typeInstrumentation)
.and(muzzleMatcher)
.transform(new PatchByteCodeVersionTransformer())
.transform(helperInjector);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle;
import java.lang.ref.WeakReference;
import java.util.HashSet;
Expand Down Expand Up @@ -84,6 +85,9 @@ static InstrumentationModuleClassLoader createInstrumentationModuleClassloader(
// TODO (Jonas): Make muzzle include advice classes as helper classes
// so that we don't have to include them here
toInject.addAll(getModuleAdviceNames(module));
if (module instanceof ExperimentalInstrumentationModule) {
toInject.removeAll(((ExperimentalInstrumentationModule) module).injectedClassNames());
}

ClassLoader agentOrExtensionCl = module.getClass().getClassLoader();
Map<String, ClassCopySource> injectedClasses =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,26 @@ public class InstrumentationModuleClassLoader extends ClassLoader {
private volatile MethodHandles.Lookup cachedLookup;

private final ClassLoader instrumentedCl;
private final boolean delegateAllToAgent;

public InstrumentationModuleClassLoader(
ClassLoader instrumentedCl,
ClassLoader agentOrExtensionCl,
Map<String, ClassCopySource> injectedClasses) {
this(instrumentedCl, agentOrExtensionCl, injectedClasses, false);
}

InstrumentationModuleClassLoader(
ClassLoader instrumentedCl,
ClassLoader agentOrExtensionCl,
Map<String, ClassCopySource> injectedClasses,
boolean delegateAllToAgent) {
// agent/extension-classloader is "main"-parent, but class lookup is overridden
super(agentOrExtensionCl);
additionalInjectedClasses = injectedClasses;
this.agentOrExtensionCl = agentOrExtensionCl;
this.instrumentedCl = instrumentedCl;
this.delegateAllToAgent = delegateAllToAgent;
}

/**
Expand Down Expand Up @@ -110,7 +120,7 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
}
}
}
if (result == null) {
if (result == null && shouldLoadFromAgent(name)) {
result = tryLoad(agentOrExtensionCl, name);
}
if (result == null) {
Expand All @@ -128,6 +138,10 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
}
}

private boolean shouldLoadFromAgent(String dotClassName) {
return delegateAllToAgent || dotClassName.startsWith("io.opentelemetry.javaagent");
}

private static Class<?> tryLoad(ClassLoader cl, String name) {
try {
return cl.loadClass(name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ void checkLookup() throws Throwable {
ClassLoader dummyParent = new URLClassLoader(new URL[] {}, null);

InstrumentationModuleClassLoader m1 =
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);
InstrumentationModuleClassLoader m2 =
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);

// MethodHandles.publicLookup() always succeeds on the first invocation
lookupAndInvokeFoo(m1);
Expand Down Expand Up @@ -79,7 +79,7 @@ void checkInjectedClassesHavePackage() throws Throwable {

ClassLoader dummyParent = new URLClassLoader(new URL[] {}, null);
InstrumentationModuleClassLoader m1 =
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject);
new InstrumentationModuleClassLoader(dummyParent, dummyParent, toInject, true);

Class<?> injected = Class.forName(A.class.getName(), true, m1);
// inject two classes from the same package to trigger errors if we try to redefine the package
Expand Down Expand Up @@ -120,7 +120,7 @@ void checkClassLookupPrecedence(@TempDir Path tempDir) throws Exception {
toInject.put(C.class.getName(), ClassCopySource.create(C.class.getName(), moduleSourceCl));

InstrumentationModuleClassLoader moduleCl =
new InstrumentationModuleClassLoader(appCl, agentCl, toInject);
new InstrumentationModuleClassLoader(appCl, agentCl, toInject, true);

// Verify precedence for classloading
Class<?> clA = moduleCl.loadClass(A.class.getName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,15 @@ public HelperInjector(

public HelperInjector(
String requestingName,
List<String> helperClassNames,
Map<String, Function<ClassLoader, byte[]>> helperMap,
List<HelperResource> helperResources,
ClassLoader helpersSource,
Instrumentation instrumentation) {
this.requestingName = requestingName;

this.helperClassNames = helperMap.keySet();
this.helperClassNames = new LinkedHashSet<>(helperClassNames);
this.helperClassNames.addAll(helperMap.keySet());
this.dynamicTypeMap.putAll(helperMap);
laurit marked this conversation as resolved.
Show resolved Hide resolved

this.helperResources = helperResources;
Expand All @@ -150,7 +152,12 @@ public static HelperInjector forDynamicTypes(
bytes.put(helper.getTypeDescription().getName(), cl -> helper.getBytes());
}
return new HelperInjector(
requestingName, bytes, Collections.emptyList(), null, instrumentation);
requestingName,
Collections.emptyList(),
bytes,
Collections.emptyList(),
null,
instrumentation);
}

public static void setHelperInjectorListener(HelperInjectorListener listener) {
Expand Down
Loading
Loading