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 all commits
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,6 +31,7 @@
import io.opentelemetry.javaagent.tooling.util.NamedMatcher;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.lang.instrument.Instrumentation;
import java.util.Collections;
import java.util.List;
import net.bytebuddy.agent.builder.AgentBuilder;
import net.bytebuddy.description.annotation.AnnotationSource;
Expand Down Expand Up @@ -69,46 +70,75 @@ 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();
}

IndyModuleRegistry.registerIndyModule(instrumentationModule);

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

MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);

AgentBuilder.Transformer helperInjector =
new HelperInjector(
instrumentationModule.instrumentationName(),
injectedClassesCollector.getClassesToInject(),
injectedHelperClassNames,
helperResourceBuilder.getResources(),
instrumentationModule.getClass().getClassLoader(),
instrumentation);
AgentBuilder.Transformer indyHelperInjector =
new HelperInjector(
instrumentationModule.instrumentationName(),
injectedClassesCollector.getClassesToInject(),
Collections.emptyList(),
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);
.transform(helperInjector)
.transform(indyHelperInjector);

// TODO (Jonas): we are not calling
// contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder) anymore
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.opentelemetry.javaagent.tooling.TransformSafeLogger;
import io.opentelemetry.javaagent.tooling.Utils;
import io.opentelemetry.javaagent.tooling.config.AgentConfig;
import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry;
import io.opentelemetry.javaagent.tooling.muzzle.Mismatch;
import io.opentelemetry.javaagent.tooling.muzzle.ReferenceMatcher;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
Expand Down Expand Up @@ -60,6 +61,10 @@ public boolean matches(
ProtectionDomain protectionDomain) {
if (classLoader == BOOTSTRAP_LOADER) {
classLoader = Utils.getBootstrapProxy();
} else if (instrumentationModule.isIndyModule()) {
classLoader =
IndyModuleRegistry.getInstrumentationClassloader(
instrumentationModule.getClass().getName(), classLoader);
}
return matchCache.computeIfAbsent(classLoader, this::doesMatch);
}
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 @@ -160,7 +160,6 @@ public static void setHelperInjectorListener(HelperInjectorListener listener) {
private Map<String, Supplier<byte[]>> getHelperMap(ClassLoader targetClassloader) {
Map<String, Supplier<byte[]>> result = new LinkedHashMap<>();
if (dynamicTypeMap.isEmpty()) {

for (String helperClassName : helperClassNames) {
result.put(
helperClassName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package indy;
package io.opentelemetry.javaagent;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
import static net.bytebuddy.matcher.ElementMatchers.named;
Expand Down
Loading