Skip to content

Commit a0f980d

Browse files
zeitlingerzeitlinger
committed
pr feedback
1 parent 02fb7b6 commit a0f980d

File tree

4 files changed

+51
-131
lines changed

4 files changed

+51
-131
lines changed

‎instrumentation/methods/javaagent/build.gradle.kts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ muzzle {
1111
dependencies {
1212
compileOnly(project(":javaagent-tooling"))
1313
compileOnly(project(":instrumentation-annotations-support"))
14+
implementation("net.bytebuddy:byte-buddy-dep")
1415
}
1516

1617
tasks.withType<Test>().configureEach {

‎instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java‎

Lines changed: 28 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -21,27 +21,21 @@
2121
import io.opentelemetry.instrumentation.api.incubator.semconv.util.ClassAndMethod;
2222
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
2323
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
24-
import java.util.Set;
24+
import java.util.Map;
2525
import net.bytebuddy.asm.Advice;
2626
import net.bytebuddy.description.type.TypeDescription;
2727
import net.bytebuddy.implementation.bytecode.assign.Assigner;
2828
import net.bytebuddy.matcher.ElementMatcher;
29+
import net.bytebuddy.utility.JavaConstant;
2930

31+
@SuppressWarnings("EnumOrdinal")
3032
public class MethodInstrumentation implements TypeInstrumentation {
3133
private final String className;
32-
private final Set<String> internalMethodNames;
33-
private final Set<String> serverMethodNames;
34-
private final Set<String> clientMethodNames;
35-
36-
public MethodInstrumentation(
37-
String className,
38-
Set<String> internalMethodNames,
39-
Set<String> serverMethodNames,
40-
Set<String> clientMethodNames) {
34+
private final Map<String, SpanKind> methodNames;
35+
36+
public MethodInstrumentation(String className, Map<String, SpanKind> methodNames) {
4137
this.className = className;
42-
this.internalMethodNames = internalMethodNames;
43-
this.serverMethodNames = serverMethodNames;
44-
this.clientMethodNames = clientMethodNames;
38+
this.methodNames = methodNames;
4539
}
4640

4741
@Override
@@ -64,123 +58,46 @@ public ElementMatcher<TypeDescription> typeMatcher() {
6458

6559
@Override
6660
public void transform(TypeTransformer transformer) {
67-
applyMethodTransformation(transformer, internalMethodNames, "$InternalMethodAdvice");
68-
applyMethodTransformation(transformer, clientMethodNames, "$ClientMethodAdvice");
69-
applyMethodTransformation(transformer, serverMethodNames, "$ServerMethodAdvice");
70-
}
71-
72-
private static void applyMethodTransformation(
73-
TypeTransformer transformer, Set<String> methodNames, String methodAdvice) {
7461
transformer.applyAdviceToMethod(
75-
namedOneOf(methodNames.toArray(new String[0])).and(isMethod()),
62+
namedOneOf(methodNames.keySet().toArray(new String[0])).and(isMethod()),
7663
mapping ->
77-
mapping.bind(
78-
MethodReturnType.class,
79-
(instrumentedType, instrumentedMethod, assigner, argumentHandler, sort) ->
80-
Advice.OffsetMapping.Target.ForStackManipulation.of(
81-
instrumentedMethod.getReturnType().asErasure())),
82-
MethodInstrumentation.class.getName() + methodAdvice);
64+
mapping
65+
.bind(
66+
MethodReturnType.class,
67+
(instrumentedType, instrumentedMethod, assigner, argumentHandler, sort) ->
68+
Advice.OffsetMapping.Target.ForStackManipulation.of(
69+
instrumentedMethod.getReturnType().asErasure()))
70+
.bind(
71+
SpanKindOrdinal.class,
72+
(instrumentedType, instrumentedMethod, assigner, argumentHandler, sort) ->
73+
Advice.OffsetMapping.Target.ForStackManipulation.of(
74+
JavaConstant.Simple.ofLoaded(
75+
methodNames.get(instrumentedMethod.getName()).ordinal()))),
76+
MethodInstrumentation.class.getName() + "$MethodAdvice");
8377
}
8478

8579
// custom annotation that represents the return type of the method
8680
@interface MethodReturnType {}
8781

88-
@SuppressWarnings("unused")
89-
public static class InternalMethodAdvice {
90-
91-
@Advice.OnMethodEnter(suppress = Throwable.class)
92-
public static void onEnter(
93-
@Advice.Origin("#t") Class<?> declaringClass,
94-
@Advice.Origin("#m") String methodName,
95-
@Advice.Local("otelMethod") MethodAndType classAndMethod,
96-
@Advice.Local("otelContext") Context context,
97-
@Advice.Local("otelScope") Scope scope) {
98-
Context parentContext = currentContext();
99-
classAndMethod =
100-
MethodAndType.create(
101-
ClassAndMethod.create(declaringClass, methodName), SpanKind.INTERNAL);
102-
103-
if (!instrumenter().shouldStart(parentContext, classAndMethod)) {
104-
return;
105-
}
106-
107-
context = instrumenter().start(parentContext, classAndMethod);
108-
scope = context.makeCurrent();
109-
}
110-
111-
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
112-
public static void stopSpan(
113-
@MethodReturnType Class<?> methodReturnType,
114-
@Advice.Local("otelMethod") MethodAndType classAndMethod,
115-
@Advice.Local("otelContext") Context context,
116-
@Advice.Local("otelScope") Scope scope,
117-
@Advice.Return(typing = Assigner.Typing.DYNAMIC, readOnly = false) Object returnValue,
118-
@Advice.Thrown Throwable throwable) {
119-
if (scope == null) {
120-
return;
121-
}
122-
scope.close();
123-
124-
returnValue =
125-
AsyncOperationEndSupport.create(instrumenter(), Void.class, methodReturnType)
126-
.asyncEnd(context, classAndMethod, returnValue, throwable);
127-
}
128-
}
129-
130-
@SuppressWarnings("unused")
131-
public static class ClientMethodAdvice {
132-
133-
@Advice.OnMethodEnter(suppress = Throwable.class)
134-
public static void onEnter(
135-
@Advice.Origin("#t") Class<?> declaringClass,
136-
@Advice.Origin("#m") String methodName,
137-
@Advice.Local("otelMethod") MethodAndType classAndMethod,
138-
@Advice.Local("otelContext") Context context,
139-
@Advice.Local("otelScope") Scope scope) {
140-
Context parentContext = currentContext();
141-
classAndMethod =
142-
MethodAndType.create(ClassAndMethod.create(declaringClass, methodName), SpanKind.CLIENT);
143-
144-
if (!instrumenter().shouldStart(parentContext, classAndMethod)) {
145-
return;
146-
}
147-
148-
context = instrumenter().start(parentContext, classAndMethod);
149-
scope = context.makeCurrent();
150-
}
151-
152-
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
153-
public static void stopSpan(
154-
@MethodReturnType Class<?> methodReturnType,
155-
@Advice.Local("otelMethod") MethodAndType classAndMethod,
156-
@Advice.Local("otelContext") Context context,
157-
@Advice.Local("otelScope") Scope scope,
158-
@Advice.Return(typing = Assigner.Typing.DYNAMIC, readOnly = false) Object returnValue,
159-
@Advice.Thrown Throwable throwable) {
160-
if (scope == null) {
161-
return;
162-
}
163-
scope.close();
164-
165-
returnValue =
166-
AsyncOperationEndSupport.create(instrumenter(), Void.class, methodReturnType)
167-
.asyncEnd(context, classAndMethod, returnValue, throwable);
168-
}
169-
}
82+
// custom annotation that represents the SpanKind of the method
83+
@interface SpanKindOrdinal {}
17084

17185
@SuppressWarnings("unused")
172-
public static class ServerMethodAdvice {
86+
public static class MethodAdvice {
17387

17488
@Advice.OnMethodEnter(suppress = Throwable.class)
17589
public static void onEnter(
90+
@SpanKindOrdinal int spanKindOrdinal,
17691
@Advice.Origin("#t") Class<?> declaringClass,
17792
@Advice.Origin("#m") String methodName,
17893
@Advice.Local("otelMethod") MethodAndType classAndMethod,
17994
@Advice.Local("otelContext") Context context,
18095
@Advice.Local("otelScope") Scope scope) {
18196
Context parentContext = currentContext();
18297
classAndMethod =
183-
MethodAndType.create(ClassAndMethod.create(declaringClass, methodName), SpanKind.SERVER);
98+
MethodAndType.create(
99+
ClassAndMethod.create(declaringClass, methodName),
100+
SpanKind.values()[spanKindOrdinal]);
184101

185102
if (!instrumenter().shouldStart(parentContext, classAndMethod)) {
186103
return;

‎instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@
99

1010
import com.google.auto.service.AutoService;
1111
import io.opentelemetry.api.incubator.config.DeclarativeConfigProperties;
12+
import io.opentelemetry.api.trace.SpanKind;
1213
import io.opentelemetry.javaagent.bootstrap.internal.AgentInstrumentationConfig;
1314
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
1415
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1516
import io.opentelemetry.javaagent.tooling.config.MethodsConfigurationParser;
1617
import java.util.Arrays;
17-
import java.util.Collections;
1818
import java.util.List;
1919
import java.util.Map;
2020
import java.util.Set;
@@ -46,7 +46,11 @@ private static List<TypeInstrumentation> parseConfigProperties() {
4646
.map(
4747
e ->
4848
new MethodInstrumentation(
49-
e.getKey(), e.getValue(), Collections.emptySet(), Collections.emptySet()))
49+
e.getKey(),
50+
e.getValue().stream()
51+
.collect(
52+
Collectors.toMap(
53+
methodName -> methodName, ignore -> SpanKind.INTERNAL))))
5054
.collect(Collectors.toList());
5155
}
5256

‎instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodsConfig.java‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,12 @@
88
import static java.util.Collections.emptyList;
99

1010
import io.opentelemetry.api.incubator.config.DeclarativeConfigProperties;
11+
import io.opentelemetry.api.trace.SpanKind;
1112
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
12-
import java.util.HashSet;
13+
import java.util.LinkedHashMap;
1314
import java.util.List;
14-
import java.util.Set;
15+
import java.util.Locale;
16+
import java.util.Map;
1517
import java.util.logging.Level;
1618
import java.util.logging.Logger;
1719
import java.util.stream.Collectors;
@@ -36,36 +38,32 @@ private static Stream<MethodInstrumentation> parseMethodInstrumentation(
3638
logger.log(Level.WARNING, "Invalid methods configuration - class name missing: {0}", config);
3739
return Stream.empty();
3840
}
39-
Set<String> internal = new HashSet<>();
40-
Set<String> server = new HashSet<>();
41-
Set<String> client = new HashSet<>();
4241

43-
List<DeclarativeConfigProperties> methods = config.getStructuredList("methods", emptyList());
44-
for (DeclarativeConfigProperties method : methods) {
42+
Map<String, SpanKind> methodNames = new LinkedHashMap<>();
43+
for (DeclarativeConfigProperties method : config.getStructuredList("methods", emptyList())) {
4544
String methodName = method.getString("name");
4645
if (isNullOrEmpty(methodName)) {
4746
logger.log(
4847
Level.WARNING, "Invalid methods configuration - method name missing: {0}", method);
4948
continue;
5049
}
51-
String spanKind = method.getString("span_kind", "internal");
52-
if ("internal".equalsIgnoreCase(spanKind)) {
53-
internal.add(methodName);
54-
} else if ("server".equalsIgnoreCase(spanKind)) {
55-
server.add(methodName);
56-
} else if ("client".equalsIgnoreCase(spanKind)) {
57-
client.add(methodName);
58-
} else {
59-
logger.log(Level.WARNING, "Invalid methods configuration - unknown span_kind: {0}", method);
50+
String spanKind = method.getString("span_kind", "INTERNAL");
51+
try {
52+
methodNames.put(methodName, SpanKind.valueOf(spanKind.toUpperCase(Locale.ROOT)));
53+
} catch (IllegalArgumentException e) {
54+
logger.log(
55+
Level.WARNING,
56+
"Invalid methods configuration - unknown span_kind: {0} for method: {1}",
57+
new Object[] {spanKind, methodName});
6058
}
6159
}
6260

63-
if (internal.isEmpty() && server.isEmpty() && client.isEmpty()) {
61+
if (methodNames.isEmpty()) {
6462
logger.log(Level.WARNING, "Invalid methods configuration - no methods defined: {0}", config);
6563
return Stream.empty();
6664
}
6765

68-
return Stream.of(new MethodInstrumentation(clazz, internal, server, client));
66+
return Stream.of(new MethodInstrumentation(clazz, methodNames));
6967
}
7068

7169
private static boolean isNullOrEmpty(String s) {

0 commit comments

Comments
 (0)