Skip to content

Commit c166bd8

Browse files
dmarkwatdmarkwat
committed
refine after review
1 parent c34bb32 commit c166bd8

File tree

3 files changed

+30
-127
lines changed

3 files changed

+30
-127
lines changed

‎instrumentation/finagle-http-23.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/v23_11/ChannelTransportInstrumentation.java

+14-10
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,14 @@
99
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
1010
import static net.bytebuddy.matcher.ElementMatchers.named;
1111

12-
import com.twitter.util.Future;
12+
import io.opentelemetry.context.Context;
13+
import io.opentelemetry.context.Scope;
1314
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1415
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
15-
import java.lang.invoke.MethodHandle;
1616
import net.bytebuddy.asm.Advice;
1717
import net.bytebuddy.description.type.TypeDescription;
1818
import net.bytebuddy.matcher.ElementMatcher;
19+
import scala.Option;
1920

2021
public class ChannelTransportInstrumentation implements TypeInstrumentation {
2122
@Override
@@ -38,17 +39,20 @@ public void transform(TypeTransformer transformer) {
3839
@SuppressWarnings("unused")
3940
public static class WriteAdvice {
4041

41-
@Advice.OnMethodEnter(suppress = Throwable.class, skipOn = Advice.OnDefaultValue.class)
42-
public static boolean methodEnter() {
43-
// only call when we've effectively wrapped the method (hence: isRecursed)
44-
return Helpers.WRITE_GUARD.isRecursed();
42+
@Advice.OnMethodEnter(suppress = Throwable.class)
43+
public static void methodEnter(@Advice.Local("otelScope") Scope scope) {
44+
Option<Context> ref = Helpers.CONTEXT_LOCAL.apply();
45+
if (ref.isDefined()) {
46+
scope = ref.get().makeCurrent();
47+
}
4548
}
4649

47-
@Advice.OnMethodExit(suppress = Throwable.class)
50+
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
4851
public static void methodExit(
49-
@Advice.SelfCallHandle MethodHandle handle,
50-
@Advice.Return(readOnly = false) Future<?> ret) {
51-
ret = Helpers.callWrite(handle, ret);
52+
@Advice.Local("otelScope") Scope scope, @Advice.Thrown Throwable thrown) {
53+
if (scope != null) {
54+
scope.close();
55+
}
5256
}
5357
}
5458
}

‎instrumentation/finagle-http-23.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/v23_11/GenStreamingServerDispatcherInstrumentation.java

+15-11
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,9 @@
1010
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
1111
import static net.bytebuddy.matcher.ElementMatchers.named;
1212

13-
import com.twitter.util.Future;
13+
import io.opentelemetry.context.Context;
1414
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1515
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
16-
import java.lang.invoke.MethodHandle;
1716
import net.bytebuddy.asm.Advice;
1817
import net.bytebuddy.description.type.TypeDescription;
1918
import net.bytebuddy.matcher.ElementMatcher;
@@ -39,17 +38,22 @@ public void transform(TypeTransformer transformer) {
3938
@SuppressWarnings("unused")
4039
public static class LoopAdvice {
4140

42-
@Advice.OnMethodEnter(suppress = Throwable.class, skipOn = Advice.OnDefaultValue.class)
43-
public static boolean methodEnter() {
44-
// only call when we've effectively wrapped the method (hence: isRecursed)
45-
return Helpers.LOOP_GUARD.isRecursed();
41+
@Advice.OnMethodEnter(suppress = Throwable.class)
42+
public static void methodEnter() {
43+
// this works bc at this point in the server evaluation, the netty
44+
// instrumentation has already gone to work and assigned the context to the
45+
// local thread;
46+
//
47+
// this works specifically in finagle's netty stack bc at this point the loop()
48+
// method is running on a netty thread with the necessary access to the
49+
// java-native ThreadLocal where the Context is stored
50+
Helpers.CONTEXT_LOCAL.update(Context.current());
4651
}
4752

48-
@Advice.OnMethodExit(suppress = Throwable.class)
49-
public static void methodExit(
50-
@Advice.SelfCallHandle MethodHandle handle,
51-
@Advice.Return(readOnly = false) Future<?> ret) {
52-
ret = Helpers.loopAdviceExit(handle, ret);
53+
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
54+
public static void methodExit(@Advice.Thrown Throwable thrown) {
55+
// always clear this
56+
Helpers.CONTEXT_LOCAL.clear();
5357
}
5458
}
5559
}

‎instrumentation/finagle-http-23.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/v23_11/Helpers.java

+1-106
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,6 @@
77

88
import static io.opentelemetry.instrumentation.netty.v4_1.internal.client.HttpClientRequestTracingHandler.HTTP_CLIENT_REQUEST;
99

10-
import com.twitter.finagle.context.Contexts;
11-
import com.twitter.finagle.context.LocalContext;
12-
import com.twitter.util.Future;
1310
import com.twitter.util.Local;
1411
import io.netty.channel.Channel;
1512
import io.netty.channel.ChannelHandler;
@@ -25,115 +22,13 @@
2522
import io.opentelemetry.instrumentation.netty.v4_1.internal.server.HttpServerTracingHandler;
2623
import io.opentelemetry.javaagent.instrumentation.netty.v4_1.NettyHttpServerResponseBeforeCommitHandler;
2724
import io.opentelemetry.javaagent.instrumentation.netty.v4_1.NettyServerSingletons;
28-
import java.lang.invoke.MethodHandle;
2925
import java.util.Deque;
30-
import java.util.concurrent.Callable;
31-
import scala.Function0;
32-
import scala.Option;
3326

3427
public class Helpers {
3528

3629
private Helpers() {}
3730

38-
public static final LocalCallGuard LOOP_GUARD = new LocalCallGuard();
39-
40-
public static final LocalCallGuard WRITE_GUARD = new LocalCallGuard();
41-
42-
// used for finagle's LocalContext: carries a reference to the Context observed at the start
43-
// of the server request processing or the client Service call
44-
static class ContextRef {
45-
46-
private final LocalContext.Key<Context> key;
47-
public static final ContextRef INSTANCE = new ContextRef(Contexts.local().newKey());
48-
49-
private ContextRef(LocalContext.Key<Context> var1) {
50-
this.key = var1;
51-
}
52-
53-
public LocalContext.Key<Context> getKey() {
54-
return this.key;
55-
}
56-
}
57-
58-
// uses twitter util's Local bc the finagle/twitter stack is essentially incompatible with
59-
// java-native TLVs;
60-
// as these are referenced across twitter Future compositions, no assumptions are made and they
61-
// are used to adhere strictly to the interface, allowing the guard test to succeed at execution
62-
// time
63-
public static class LocalCallGuard {
64-
65-
private final Local<Object> guard = new Local<>();
66-
67-
// is the current thread presently inside a call guarded by this LocalCallGuard?
68-
public boolean isRecursed() {
69-
return guard.apply().isDefined();
70-
}
71-
72-
// safely guard the Function0 call;
73-
// (Function0 -> Java Supplier, in our cases -- it wraps existing calls)
74-
public <T> T guardedCall(Function0<T> f, T defaultVal) {
75-
if (isRecursed()) {
76-
return defaultVal;
77-
}
78-
return guard.let(null, f);
79-
}
80-
}
81-
82-
@SuppressWarnings({"ThrowSpecificExceptions", "CheckedExceptionNotThrown"})
83-
public static Future<?> loopAdviceExit(MethodHandle handle, Future<?> ret) {
84-
return LOOP_GUARD.guardedCall(
85-
() ->
86-
Contexts.local()
87-
.let(
88-
ContextRef.INSTANCE.getKey(),
89-
// this works bc at this point in the server evaluation, the netty
90-
// instrumentation has already gone to work and assigned the context to the
91-
// local thread;
92-
//
93-
// this works specifically in finagle's netty stack bc at this point the loop()
94-
// method is running on a netty thread with the necessary access to the
95-
// java-native ThreadLocal where the Context is stored
96-
Context.current(),
97-
() -> {
98-
try {
99-
// all access to Context.current() from this point forward should now
100-
// succeed as expected
101-
return (Future<?>) handle.invoke();
102-
} catch (Throwable e) {
103-
throw new RuntimeException(e);
104-
}
105-
}),
106-
ret);
107-
}
108-
109-
@SuppressWarnings("ThrowSpecificExceptions")
110-
public static Future<?> callWrite(MethodHandle handle, Future<?> ret) {
111-
return WRITE_GUARD.guardedCall(
112-
() -> {
113-
Option<Context> ref = Contexts.local().get(ContextRef.INSTANCE.getKey());
114-
Callable<Future<?>> call =
115-
() -> {
116-
try {
117-
return (Future<?>) handle.invoke();
118-
} catch (Exception e) {
119-
// don't wrap needlessly
120-
throw e;
121-
} catch (Throwable e) {
122-
throw new RuntimeException(e);
123-
}
124-
};
125-
if (ref.isDefined()) {
126-
// wrap the call if ContextRef contains a set Context
127-
call = ref.get().wrap(call);
128-
}
129-
try {
130-
return call.call();
131-
} catch (Exception e) {
132-
throw new RuntimeException(e);
133-
}
134-
},
135-
ret);
136-
}
31+
public static final Local<Context> CONTEXT_LOCAL = new Local<>();
13732

13833
public static <C extends Channel> ChannelInitializer<C> wrapServer(ChannelInitializer<C> inner) {
13934
return new ChannelInitializerDelegate<C>(inner) {

0 commit comments

Comments
 (0)