Skip to content

Commit 10480ad

Browse files
JonasKunzJonasKunz
authored
Implement invokedynamic advice bootstrapping (#9382)
1 parent 3b77cc4 commit 10480ad

File tree

18 files changed

+997
-37
lines changed

18 files changed

+997
-37
lines changed

‎conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,9 @@ tasks {
105105
// some moving.
106106
disable("DefaultPackage")
107107

108-
// we use modified OtelPrivateConstructorForUtilityClass which ignores *Advice classes
108+
// we use modified Otel* checks which ignore *Advice classes
109109
disable("PrivateConstructorForUtilityClass")
110+
disable("CanIgnoreReturnValueSuggester")
110111

111112
// TODO(anuraaga): Remove this, probably after instrumenter API migration instead of dealing
112113
// with older APIs.
@@ -125,9 +126,9 @@ tasks {
125126
// Allow underscore in test-type method names
126127
disable("MemberName")
127128
}
128-
if (project.path.endsWith(":testing") || name.contains("Test")) {
129+
if ((project.path.endsWith(":testing") || name.contains("Test")) && !project.name.equals("custom-checks")) {
129130
// This check causes too many failures, ignore the ones in tests
130-
disable("CanIgnoreReturnValueSuggester")
131+
disable("OtelCanIgnoreReturnValueSuggester")
131132
}
132133
}
133134
}

‎custom-checks/src/main/java/io/opentelemetry/javaagent/customchecks/OtelCanIgnoreReturnValueSuggester.java

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.customchecks;
7+
8+
import static com.google.errorprone.matchers.Description.NO_MATCH;
9+
10+
import com.google.auto.service.AutoService;
11+
import com.google.errorprone.BugPattern;
12+
import com.google.errorprone.VisitorState;
13+
import com.google.errorprone.bugpatterns.BugChecker;
14+
import com.google.errorprone.bugpatterns.checkreturnvalue.CanIgnoreReturnValueSuggester;
15+
import com.google.errorprone.matchers.Description;
16+
import com.sun.source.tree.ClassTree;
17+
import com.sun.source.tree.MethodTree;
18+
import com.sun.source.util.TreePath;
19+
20+
@AutoService(BugChecker.class)
21+
@BugPattern(
22+
summary =
23+
"Methods with ignorable return values (including methods that always 'return this') should be annotated with @com.google.errorprone.annotations.CanIgnoreReturnValue",
24+
severity = BugPattern.SeverityLevel.WARNING)
25+
public class OtelCanIgnoreReturnValueSuggester extends BugChecker
26+
implements BugChecker.MethodTreeMatcher {
27+
28+
private static final long serialVersionUID = 1L;
29+
30+
private final CanIgnoreReturnValueSuggester delegate = new CanIgnoreReturnValueSuggester();
31+
32+
@Override
33+
public Description matchMethod(MethodTree methodTree, VisitorState visitorState) {
34+
ClassTree containerClass = findContainingClass(visitorState.getPath());
35+
if (containerClass.getSimpleName().toString().endsWith("Advice")) {
36+
return NO_MATCH;
37+
}
38+
Description description = delegate.matchMethod(methodTree, visitorState);
39+
if (description == NO_MATCH) {
40+
return description;
41+
}
42+
return describeMatch(methodTree);
43+
}
44+
45+
private static ClassTree findContainingClass(TreePath path) {
46+
TreePath parent = path.getParentPath();
47+
while (parent != null && !(parent.getLeaf() instanceof ClassTree)) {
48+
parent = parent.getParentPath();
49+
}
50+
if (parent == null) {
51+
throw new IllegalStateException(
52+
"Method is expected to be contained in a class, something must be wrong");
53+
}
54+
ClassTree containerClass = (ClassTree) parent.getLeaf();
55+
return containerClass;
56+
}
57+
}

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ void report() {
8282
}
8383

8484
// this private method is designed for assignment of the return value
85-
@SuppressWarnings("CanIgnoreReturnValueSuggester")
85+
@SuppressWarnings("OtelCanIgnoreReturnValueSuggester")
8686
private SupportabilityMetrics start() {
8787
if (agentDebugEnabled) {
8888
ScheduledExecutorService executor =

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/cache/weaklockfree/WeakConcurrentMap.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ static final class LookupKey<K> {
145145
private K key;
146146
private int hashCode;
147147

148-
@SuppressWarnings("CanIgnoreReturnValueSuggester")
148+
@SuppressWarnings("OtelCanIgnoreReturnValueSuggester")
149149
LookupKey<K> withValue(K key) {
150150
this.key = key;
151151
hashCode = System.identityHashCode(key);

‎instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/DistributionStatisticConfigModifier.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
99

10-
@SuppressWarnings("CanIgnoreReturnValueSuggester")
10+
@SuppressWarnings("OtelCanIgnoreReturnValueSuggester")
1111
enum DistributionStatisticConfigModifier {
1212
DISABLE_HISTOGRAM_GAUGES {
1313
@Override

‎javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/IndyBootstrapDispatcher.java

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.bootstrap;
7+
8+
import java.lang.invoke.CallSite;
9+
import java.lang.invoke.ConstantCallSite;
10+
import java.lang.invoke.MethodHandle;
11+
import java.lang.invoke.MethodHandles;
12+
import java.lang.invoke.MethodType;
13+
import java.lang.reflect.Array;
14+
15+
/**
16+
* Contains the bootstrap method for initializing invokedynamic callsites which are added via agent
17+
* instrumentation.
18+
*/
19+
public class IndyBootstrapDispatcher {
20+
21+
private static volatile MethodHandle bootstrap;
22+
23+
private IndyBootstrapDispatcher() {}
24+
25+
/**
26+
* Initialized the invokedynamic bootstrapping method to which this class will delegate.
27+
*
28+
* @param bootstrapMethod the method to delegate to. Must have the same type as {@link
29+
* #bootstrap}.
30+
*/
31+
public static void init(MethodHandle bootstrapMethod) {
32+
bootstrap = bootstrapMethod;
33+
}
34+
35+
public static CallSite bootstrap(
36+
MethodHandles.Lookup lookup,
37+
String adviceMethodName,
38+
MethodType adviceMethodType,
39+
Object... args) {
40+
CallSite callSite = null;
41+
if (bootstrap != null) {
42+
try {
43+
callSite = (CallSite) bootstrap.invoke(lookup, adviceMethodName, adviceMethodType, args);
44+
} catch (Throwable e) {
45+
ExceptionLogger.logSuppressedError("Error bootstrapping indy instruction", e);
46+
}
47+
}
48+
if (callSite == null) {
49+
// The MethodHandle pointing to the Advice could not be created for some reason,
50+
// fallback to a Noop MethodHandle to not crash the application
51+
MethodHandle noop = generateNoopMethodHandle(adviceMethodType);
52+
callSite = new ConstantCallSite(noop);
53+
}
54+
return callSite;
55+
}
56+
57+
// package visibility for testing
58+
static MethodHandle generateNoopMethodHandle(MethodType methodType) {
59+
Class<?> returnType = methodType.returnType();
60+
MethodHandle noopNoArg;
61+
if (returnType == void.class) {
62+
noopNoArg =
63+
MethodHandles.constant(Void.class, null).asType(MethodType.methodType(void.class));
64+
} else {
65+
noopNoArg = MethodHandles.constant(returnType, getDefaultValue(returnType));
66+
}
67+
return MethodHandles.dropArguments(noopNoArg, 0, methodType.parameterList());
68+
}
69+
70+
private static Object getDefaultValue(Class<?> classOrPrimitive) {
71+
if (classOrPrimitive.isPrimitive()) {
72+
// arrays of primitives are initialized with the correct primitive default value (e.g. 0 for
73+
// int.class)
74+
// we use this fact to generate the correct default value reflectively
75+
return Array.get(Array.newInstance(classOrPrimitive, 1), 0);
76+
} else {
77+
return null; // null is the default value for reference types
78+
}
79+
}
80+
}

‎javaagent-bootstrap/src/test/java/io/opentelemetry/javaagent/bootstrap/IndyBootstrapDispatcherTest.java

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.bootstrap;
7+
8+
import static org.assertj.core.api.Assertions.assertThat;
9+
10+
import java.lang.invoke.MethodHandle;
11+
import java.lang.invoke.MethodType;
12+
import org.junit.jupiter.api.Test;
13+
14+
public class IndyBootstrapDispatcherTest {
15+
16+
@Test
17+
void testVoidNoopMethodHandle() throws Throwable {
18+
MethodHandle noArg = generateAndCheck(MethodType.methodType(void.class));
19+
noArg.invokeExact();
20+
21+
MethodHandle intArg = generateAndCheck(MethodType.methodType(void.class, int.class));
22+
intArg.invokeExact(42);
23+
}
24+
25+
@Test
26+
void testIntNoopMethodHandle() throws Throwable {
27+
MethodHandle noArg = generateAndCheck(MethodType.methodType(int.class));
28+
assertThat((int) noArg.invokeExact()).isEqualTo(0);
29+
30+
MethodHandle intArg = generateAndCheck(MethodType.methodType(int.class, int.class));
31+
assertThat((int) intArg.invokeExact(42)).isEqualTo(0);
32+
}
33+
34+
@Test
35+
void testBooleanNoopMethodHandle() throws Throwable {
36+
MethodHandle noArg = generateAndCheck(MethodType.methodType(boolean.class));
37+
assertThat((boolean) noArg.invokeExact()).isEqualTo(false);
38+
39+
MethodHandle intArg = generateAndCheck(MethodType.methodType(boolean.class, int.class));
40+
assertThat((boolean) intArg.invokeExact(42)).isEqualTo(false);
41+
}
42+
43+
@Test
44+
void testReferenceNoopMethodHandle() throws Throwable {
45+
MethodHandle noArg = generateAndCheck(MethodType.methodType(Runnable.class));
46+
assertThat((Runnable) noArg.invokeExact()).isEqualTo(null);
47+
48+
MethodHandle intArg = generateAndCheck(MethodType.methodType(Runnable.class, int.class));
49+
assertThat((Runnable) intArg.invokeExact(42)).isEqualTo(null);
50+
}
51+
52+
private static MethodHandle generateAndCheck(MethodType type) {
53+
MethodHandle mh = IndyBootstrapDispatcher.generateNoopMethodHandle(type);
54+
assertThat(mh.type()).isEqualTo(type);
55+
return mh;
56+
}
57+
}

‎javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java

Lines changed: 83 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@
2020
import io.opentelemetry.javaagent.tooling.config.AgentConfig;
2121
import io.opentelemetry.javaagent.tooling.field.VirtualFieldImplementationInstaller;
2222
import io.opentelemetry.javaagent.tooling.field.VirtualFieldImplementationInstallerFactory;
23+
import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyModuleRegistry;
24+
import io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyTypeTransformerImpl;
25+
import io.opentelemetry.javaagent.tooling.instrumentation.indy.PatchByteCodeVersionTransformer;
2326
import io.opentelemetry.javaagent.tooling.muzzle.HelperResourceBuilderImpl;
2427
import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle;
2528
import io.opentelemetry.javaagent.tooling.util.IgnoreFailedTypeMatcher;
@@ -62,6 +65,52 @@ AgentBuilder install(
6265
FINE, "Instrumentation {0} is disabled", instrumentationModule.instrumentationName());
6366
return parentAgentBuilder;
6467
}
68+
69+
if (instrumentationModule.isIndyModule()) {
70+
return installIndyModule(instrumentationModule, parentAgentBuilder);
71+
} else {
72+
return installInjectingModule(instrumentationModule, parentAgentBuilder, config);
73+
}
74+
}
75+
76+
private AgentBuilder installIndyModule(
77+
InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder) {
78+
79+
IndyModuleRegistry.registerIndyModule(instrumentationModule);
80+
81+
// TODO (Jonas): Adapt MuzzleMatcher to use the same type lookup strategy as the
82+
// InstrumentationModuleClassLoader
83+
// MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);
84+
VirtualFieldImplementationInstaller contextProvider =
85+
virtualFieldInstallerFactory.create(instrumentationModule);
86+
87+
AgentBuilder agentBuilder = parentAgentBuilder;
88+
for (TypeInstrumentation typeInstrumentation : instrumentationModule.typeInstrumentations()) {
89+
AgentBuilder.Identified.Extendable extendableAgentBuilder =
90+
setTypeMatcher(agentBuilder, instrumentationModule, typeInstrumentation)
91+
.transform(new PatchByteCodeVersionTransformer());
92+
93+
IndyTypeTransformerImpl typeTransformer =
94+
new IndyTypeTransformerImpl(extendableAgentBuilder, instrumentationModule);
95+
typeInstrumentation.transform(typeTransformer);
96+
extendableAgentBuilder = typeTransformer.getAgentBuilder();
97+
// TODO (Jonas): make instrumentation of bytecode older than 1.4 opt-in via a config option
98+
// TODO (Jonas): we are not calling
99+
// contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder) anymore
100+
// As a result the advices should store `VirtualFields` as static variables instead of having
101+
// the lookup inline
102+
// We need to update our documentation on that
103+
extendableAgentBuilder = contextProvider.injectFields(extendableAgentBuilder);
104+
105+
agentBuilder = extendableAgentBuilder;
106+
}
107+
return agentBuilder;
108+
}
109+
110+
private AgentBuilder installInjectingModule(
111+
InstrumentationModule instrumentationModule,
112+
AgentBuilder parentAgentBuilder,
113+
ConfigProperties config) {
65114
List<String> helperClassNames =
66115
InstrumentationModuleMuzzle.getHelperClassNames(instrumentationModule);
67116
HelperResourceBuilderImpl helperResourceBuilder = new HelperResourceBuilderImpl();
@@ -78,8 +127,6 @@ AgentBuilder install(
78127
return parentAgentBuilder;
79128
}
80129

81-
ElementMatcher.Junction<ClassLoader> moduleClassLoaderMatcher =
82-
instrumentationModule.classLoaderMatcher();
83130
MuzzleMatcher muzzleMatcher = new MuzzleMatcher(logger, instrumentationModule, config);
84131
AgentBuilder.Transformer helperInjector =
85132
new HelperInjector(
@@ -93,32 +140,9 @@ AgentBuilder install(
93140

94141
AgentBuilder agentBuilder = parentAgentBuilder;
95142
for (TypeInstrumentation typeInstrumentation : typeInstrumentations) {
96-
ElementMatcher<TypeDescription> typeMatcher =
97-
new NamedMatcher<>(
98-
instrumentationModule.getClass().getSimpleName()
99-
+ "#"
100-
+ typeInstrumentation.getClass().getSimpleName(),
101-
new IgnoreFailedTypeMatcher(typeInstrumentation.typeMatcher()));
102-
ElementMatcher<ClassLoader> classLoaderMatcher =
103-
new NamedMatcher<>(
104-
instrumentationModule.getClass().getSimpleName()
105-
+ "#"
106-
+ typeInstrumentation.getClass().getSimpleName(),
107-
moduleClassLoaderMatcher.and(typeInstrumentation.classLoaderOptimization()));
108143

109144
AgentBuilder.Identified.Extendable extendableAgentBuilder =
110-
agentBuilder
111-
.type(
112-
new LoggingFailSafeMatcher<>(
113-
typeMatcher,
114-
"Instrumentation type matcher unexpected exception: " + typeMatcher),
115-
new LoggingFailSafeMatcher<>(
116-
classLoaderMatcher,
117-
"Instrumentation class loader matcher unexpected exception: "
118-
+ classLoaderMatcher))
119-
.and(
120-
(typeDescription, classLoader, module, classBeingRedefined, protectionDomain) ->
121-
classLoader == null || NOT_DECORATOR_MATCHER.matches(typeDescription))
145+
setTypeMatcher(agentBuilder, instrumentationModule, typeInstrumentation)
122146
.and(muzzleMatcher)
123147
.transform(ConstantAdjuster.instance())
124148
.transform(helperInjector);
@@ -133,4 +157,37 @@ AgentBuilder install(
133157

134158
return agentBuilder;
135159
}
160+
161+
private static AgentBuilder.Identified.Narrowable setTypeMatcher(
162+
AgentBuilder agentBuilder,
163+
InstrumentationModule instrumentationModule,
164+
TypeInstrumentation typeInstrumentation) {
165+
166+
ElementMatcher.Junction<ClassLoader> moduleClassLoaderMatcher =
167+
instrumentationModule.classLoaderMatcher();
168+
169+
ElementMatcher<TypeDescription> typeMatcher =
170+
new NamedMatcher<>(
171+
instrumentationModule.getClass().getSimpleName()
172+
+ "#"
173+
+ typeInstrumentation.getClass().getSimpleName(),
174+
new IgnoreFailedTypeMatcher(typeInstrumentation.typeMatcher()));
175+
ElementMatcher<ClassLoader> classLoaderMatcher =
176+
new NamedMatcher<>(
177+
instrumentationModule.getClass().getSimpleName()
178+
+ "#"
179+
+ typeInstrumentation.getClass().getSimpleName(),
180+
moduleClassLoaderMatcher.and(typeInstrumentation.classLoaderOptimization()));
181+
182+
return agentBuilder
183+
.type(
184+
new LoggingFailSafeMatcher<>(
185+
typeMatcher, "Instrumentation type matcher unexpected exception: " + typeMatcher),
186+
new LoggingFailSafeMatcher<>(
187+
classLoaderMatcher,
188+
"Instrumentation class loader matcher unexpected exception: " + classLoaderMatcher))
189+
.and(
190+
(typeDescription, classLoader, module, classBeingRedefined, protectionDomain) ->
191+
classLoader == null || NOT_DECORATOR_MATCHER.matches(typeDescription));
192+
}
136193
}

0 commit comments

Comments
 (0)