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.

Sign up for GitHub

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

indy instrumentation - leftovers migration #13074

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
+198 −231
Open

indy instrumentation - leftovers migration #13074

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c911faa
lambda
SylvainJuge Jan 20, 2025
26e79f9
inject proxies for indy
SylvainJuge Jan 20, 2025
7f3ae3b
kotlinx + integration tests
SylvainJuge Jan 21, 2025
9183992
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Jan 21, 2025
29c99dd
inject missing classes for indy
SylvainJuge Jan 21, 2025
45ead47
do not inject proxies for lambda
SylvainJuge Jan 21, 2025
ba30ba8
move back comment in the expected location
SylvainJuge Jan 21, 2025
eb945b8
remove duplication
SylvainJuge Jan 22, 2025
eba7e20
add jpms compatible class file transformer
SylvainJuge Jan 22, 2025
7a1cbc7
remove lambda java9 module
SylvainJuge Jan 22, 2025
590f86e
fix pebkc
SylvainJuge Jan 22, 2025
a8cbb24
fix lambda instrumentation, must not redefine them
SylvainJuge Jan 22, 2025
403caaa
fix javadoc
SylvainJuge Jan 22, 2025
506cc62
fix things again
SylvainJuge Jan 22, 2025
da1c13d
fix it again
SylvainJuge Jan 22, 2025
c4350d0
make the comments a bit better
SylvainJuge Jan 23, 2025
1d54a14
fix typo
SylvainJuge Jan 23, 2025
03aa810
avoid injecting classes for kotlinxcoroutines
SylvainJuge Jan 24, 2025
2ed4e96
remove commented code
SylvainJuge Jan 24, 2025
77a4f00
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Jan 27, 2025
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
20 changes: 0 additions & 20 deletions instrumentation/internal/internal-lambda-java9/javaagent/build.gradle.kts
View file
Open in desktop

This file was deleted.

30 changes: 0 additions & 30 deletions ...va/io/opentelemetry/javaagent/instrumentation/internal/lambda/Java9LambdaTransformer.java
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion instrumentation/internal/internal-lambda/javaagent/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ plugins {

dependencies {
compileOnly(project(":javaagent-bootstrap"))
implementation(project(":instrumentation:internal:internal-lambda-java9:javaagent"))

testImplementation(project(":javaagent-bootstrap"))
}
20 changes: 8 additions & 12 deletions .../opentelemetry/javaagent/instrumentation/internal/lambda/LambdaInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import net.bytebuddy.utility.JavaModule;

@AutoService(InstrumentationModule.class)
public class LambdaInstrumentationModule extends InstrumentationModule {
public class LambdaInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {
public LambdaInstrumentationModule() {
super("internal-lambda");
}
Expand All @@ -28,21 +29,16 @@ public boolean defaultEnabled(ConfigProperties config) {
}

@Override
public boolean isIndyModule() {
return false;
public List<String> injectedClassNames() {
return Collections.singletonList(
"io.opentelemetry.javaagent.instrumentation.internal.lambda.LambdaTransformer");
}

@Override
public List<String> getAdditionalHelperClassNames() {
// this instrumentation uses ASM not ByteBuddy so muzzle doesn't automatically add helper
// classes
List<String> classNames = new ArrayList<>();
classNames.add("io.opentelemetry.javaagent.instrumentation.internal.lambda.LambdaTransformer");
if (JavaModule.isSupported()) {
classNames.add(
"io.opentelemetry.javaagent.instrumentation.internal.lambda.Java9LambdaTransformer");
}
return classNames;
return injectedClassNames();
}

@Override
Expand Down
44 changes: 13 additions & 31 deletions ...in/java/io/opentelemetry/javaagent/instrumentation/internal/lambda/LambdaTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,11 @@

/** Helper class for transforming lambda class bytes. */
public final class LambdaTransformer {
private static final boolean IS_JAVA_9 = isJava9();

private LambdaTransformer() {}

private static boolean isJava9() {
try {
Class.forName("java.lang.Module", false, null);
return true;
} catch (ClassNotFoundException exception) {
return false;
}
}

/**
* Called from {@code java.lang.invoke.InnerClassLambdaMetafactory} to transform lambda class
* Called from {@code java.lang.invoke.InnerClassLambdaMetaFactory} to transform lambda class
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
* bytes.
*/
@SuppressWarnings("unused")
Expand All @@ -34,29 +24,21 @@ public static byte[] transform(byte[] classBytes, String slashClassName, Class<?
if (InjectedClassHelper.isHelperClass(targetClass)) {
return classBytes;
}

ClassFileTransformer transformer = ClassFileTransformerHolder.getClassFileTransformer();
if (transformer != null) {
try {
byte[] result;
if (IS_JAVA_9) {
result =
Java9LambdaTransformer.transform(
transformer, classBytes, slashClassName, targetClass);
} else {
result =
transformer.transform(
targetClass.getClassLoader(), slashClassName, null, null, classBytes);
}
if (result != null) {
return result;
}
} catch (Throwable throwable) {
// sun.instrument.TransformerManager catches Throwable from ClassFileTransformer and ignores
// it, we do the same.
if (transformer == null) {
return classBytes;
}
try {
byte[] result =
transformer.transform(
targetClass.getClassLoader(), slashClassName, targetClass, null, classBytes);
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
if (result != null) {
classBytes = result;
}
} catch (Throwable throwable) {
// sun.instrument.TransformerManager catches Throwable from ClassFileTransformer and ignores
// it, we do the same.
}

return classBytes;
}
}
20 changes: 13 additions & 7 deletions ...ntation/kotlinxcoroutines/instrumentationannotations/AnnotationInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@
import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
import java.util.Arrays;
import java.util.List;
import net.bytebuddy.matcher.ElementMatcher;

/** Instrumentation for methods annotated with {@code WithSpan} annotation. */
@AutoService(InstrumentationModule.class)
public class AnnotationInstrumentationModule extends InstrumentationModule {
public class AnnotationInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {

public AnnotationInstrumentationModule() {
super(
Expand All @@ -26,12 +29,6 @@ public AnnotationInstrumentationModule() {
"opentelemetry-instrumentation-annotations");
}

@Override
public boolean isIndyModule() {
// needs helper classes in the same class loader
return false;
}

@Override
public int order() {
// Run first to ensure other automatic instrumentation is added after and therefore is executed
Expand All @@ -50,4 +47,13 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
public List<TypeInstrumentation> typeInstrumentations() {
return singletonList(new WithSpanInstrumentation());
}

@Override
public List<String> injectedClassNames() {
return Arrays.asList(
"io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.AnnotationSingletons",
"io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.AnnotationInstrumentationHelper",
"io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.MethodRequest",
"io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.MethodRequestCodeAttributesGetter");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of injecting all the helper did you consider adding a proxy for AnnotationInstrumentationHelper similarly to

opentelemetry-java-instrumentation/instrumentation/graphql-java/graphql-java-20.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/v20_0/GraphqlInstrumentationModule.java

Lines 41 to 48 in edbd9aa

public void injectClasses(ClassInjector injector) {
// we do not use ByteBuddy Advice dispatching in this instrumentation
// Instead, we manually call GraphqlSingletons via ASM
// Easiest solution to work with indy is to inject an indy-proxy to be invoked
injector
.proxyBuilder("io.opentelemetry.javaagent.instrumentation.graphql.v20_0.GraphqlSingletons")
.inject(InjectionMode.CLASS_ONLY);
}

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 am not sure we have the choice here, as MethodRequest is being used directly used in WithSpanInstrumentation which uses ASM, also if MethodRequest is being injected then it's also pulling AnnotationSingletons which is using it to be injected as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could replace MethodRequest with Object in method signatures. We had similar issue in indy instrumentation where advice methods could not have helper classes in the signature, see #11873 What do you think?

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 that could work, I will try to see if we can easily do it.

If I understand what you aim to achieve here is only exposing the AnnotationInstrumentationHelper class to the instrumented code through an injected proxy, and then leave other classes called by the real AnnotationInstrumentationHelper class implementation as implementation details.

One of the downsides of this approach is that we loose the type-safety of the method signatures and need to introduce explicit casts to make it work.

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've implemented it in 03aa810 and there aren't that many compromises on the code, so I think it was again a great suggestion @laurit !

}
34 changes: 34 additions & 0 deletions ...ava9/src/main/java/io/opentelemetry/javaagent/tooling/JavaModuleClassFileTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.tooling;

import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.IllegalClassFormatException;
import java.security.ProtectionDomain;

/** {@link ClassFileTransformer} implementation that provides java9 jpms module compatibility */
public class JavaModuleClassFileTransformer implements ClassFileTransformer {

private ClassFileTransformer delegate;

public JavaModuleClassFileTransformer(ClassFileTransformer delegate) {
this.delegate = delegate;
}

@Override
public byte[] transform(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] here we override the pre-java9 API and then resolve the class module from the class, this allows to transparently call it from code compiled with Java 8 API level.

In practice, there is only a single usage for this in the internal lambda instrumentation, which makes it equivalent to the (now removed) Java9LambdaTransformer.

ClassLoader loader,
String className,
Class<?> targetClass,
ProtectionDomain protectionDomain,
byte[] classfileBuffer)
throws IllegalClassFormatException {

Module module = targetClass != null ? targetClass.getModule() : null;
return delegate.transform(
module, loader, className, targetClass, protectionDomain, classfileBuffer);
}
}
10 changes: 7 additions & 3 deletions javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.SdkAutoconfigureAccess;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.Instrumentation;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -63,7 +64,6 @@
import net.bytebuddy.ByteBuddy;
import net.bytebuddy.agent.builder.AgentBuilder;
import net.bytebuddy.agent.builder.AgentBuilderUtil;
import net.bytebuddy.agent.builder.ResettableClassFileTransformer;
import net.bytebuddy.description.type.TypeDefinition;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.dynamic.DynamicType;
Expand Down Expand Up @@ -199,9 +199,13 @@ private static void installBytebuddyAgent(
logger.log(FINE, "Installed {0} extension(s)", numberOfLoadedExtensions);

agentBuilder = AgentBuilderUtil.optimize(agentBuilder);
ResettableClassFileTransformer resettableClassFileTransformer = agentBuilder.installOn(inst);
ClassFileTransformer transformer = agentBuilder.installOn(inst);
instrumentationInstalled = true;
ClassFileTransformerHolder.setClassFileTransformer(resettableClassFileTransformer);
if (JavaModule.isSupported()) {
// wrapping in a JPMS compliant implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] wrapping at registration time helps to make the consumer transparently use it with java 8 API.

transformer = new JavaModuleClassFileTransformer(transformer);
}
ClassFileTransformerHolder.setClassFileTransformer(transformer);

addHttpServerResponseCustomizers(extensionClassLoader);

Expand Down
1 change: 0 additions & 1 deletion settings.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ include(":instrumentation:internal:internal-class-loader:javaagent")
include(":instrumentation:internal:internal-class-loader:javaagent-integration-tests")
include(":instrumentation:internal:internal-eclipse-osgi-3.6:javaagent")
include(":instrumentation:internal:internal-lambda:javaagent")
include(":instrumentation:internal:internal-lambda-java9:javaagent")
include(":instrumentation:internal:internal-reflection:javaagent")
include(":instrumentation:internal:internal-reflection:javaagent-integration-tests")
include(":instrumentation:internal:internal-url-class-loader:javaagent")
Expand Down
2 changes: 0 additions & 2 deletions testing-common/integration-tests/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,13 @@ tasks {
includeTestsMatching("InstrumentOldBytecode")
}
include("**/InstrumentOldBytecode.*")
jvmArgs("-Dotel.instrumentation.inline-ibm-resource-level.enabled=false")
}

val testInlineModuleOldBytecodeInstrumentation by registering(Test::class) {
filter {
includeTestsMatching("InstrumentOldBytecode")
}
include("**/InstrumentOldBytecode.*")
jvmArgs("-Dotel.instrumentation.indy-ibm-resource-level.enabled=false")
}

test {
Expand Down
8 changes: 4 additions & 4 deletions ...bmResourceLevelInstrumentationModule.java → ...bmResourceLevelInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
import java.util.List;

@AutoService(InstrumentationModule.class)
public class InlineIbmResourceLevelInstrumentationModule extends InstrumentationModule {
public InlineIbmResourceLevelInstrumentationModule() {
super("inline-ibm-resource-level");
public class IbmResourceLevelInstrumentationModule extends InstrumentationModule {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] there is no need to maintain an inline and indy variants of this instrumentation, so we only need one that is compatible with both.

public IbmResourceLevelInstrumentationModule() {
super("ibm-resource-level");
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return singletonList(new InlineResourceLevelInstrumentation());
return singletonList(new ResourceLevelInstrumentation());
}
}
28 changes: 0 additions & 28 deletions ...ing-common/integration-tests/src/main/java/IndyIbmResourceLevelInstrumentationModule.java
View file
Open in desktop

This file was deleted.

33 changes: 0 additions & 33 deletions testing-common/integration-tests/src/main/java/InlineResourceLevelInstrumentation.java
View file
Open in desktop

This file was deleted.

Loading
Loading