Skip to content

Commit 8a4225b

Browse files
dmarkwatdmarkwat
authored
fix: corrects and stabilizes finagle instrumentation for http (#17867)
1 parent b9b76a4 commit 8a4225b

34 files changed

Lines changed: 1275 additions & 179 deletions

‎instrumentation/finagle-http-23.11/compile-stub/build.gradle.kts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
plugins {
2+
id("otel.java-conventions")
3+
}

‎instrumentation/finagle-http-23.11/compile-stub/src/main/java/com/twitter/util/Promise.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package com.twitter.util;
7+
8+
// public accessible stubs of mirrored types in com.twitter.util to ensure compilation;
9+
// these classes are consumed as a compileOnly module and replaced at runtime with their
10+
// stubbed counterparts
11+
public class Promise {
12+
private Promise() {}
13+
14+
@SuppressWarnings("ClassNamedLikeTypeParameter")
15+
public abstract static class K {}
16+
17+
public abstract static class Interruptible {}
18+
}

‎instrumentation/finagle-http-23.11/javaagent/build.gradle.kts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,17 @@ dependencies {
3232

3333
library("${scalified("com.twitter:finagle-http")}:$finagleVersion")
3434

35-
// should wire netty contexts
3635
testInstrumentation(project(":instrumentation:netty:netty-4.1:javaagent"))
3736

37+
// Exclude the Promise stub and its nested classes;
38+
// this allows us to compile against these types in the instrumentation
39+
// despite them being private in their original inner class;
40+
// this is required for VirtualField to have a concrete type to find/get/set on.
41+
compileOnly(project(":instrumentation:finagle-http-23.11:compile-stub"))
42+
3843
implementation(project(":instrumentation:netty:netty-4.1:javaagent"))
3944
implementation(project(":instrumentation:netty:netty-4.1:library"))
45+
implementation(project(":instrumentation:netty:netty-common-4.0:javaagent"))
4046
implementation(project(":instrumentation:netty:netty-common-4.0:library"))
4147
}
4248

@@ -48,6 +54,17 @@ tasks {
4854
}
4955

5056
test {
57+
jvmArgs("-Dotel.instrumentation.http.client.emit-experimental-telemetry=true")
58+
jvmArgs("-Dotel.instrumentation.http.server.emit-experimental-telemetry=true")
59+
jvmArgs("-Dio.opentelemetry.context.enableStrictContext=true")
60+
61+
// force the netty event loop into constrained territory
62+
systemProperty("io.netty.eventLoopThreads", "2")
63+
// ensure concurrent tests are competing for netty workers
64+
systemProperty("com.twitter.finagle.netty4.numWorkers", "2")
65+
// ensure concurrent tests are competing for offload pool workers
66+
systemProperty("com.twitter.finagle.offload.numWorkers", "2")
67+
5168
systemProperty(
5269
"metadataConfig",
5370
"otel.instrumentation.http.client.emit-experimental-telemetry=true," +
@@ -59,6 +76,15 @@ tasks {
5976
testClassesDirs = sourceSets.test.get().output.classesDirs
6077
classpath = sourceSets.test.get().runtimeClasspath
6178
jvmArgs("-Dotel.semconv-stability.opt-in=service.peer")
79+
jvmArgs("-Dio.opentelemetry.context.enableStrictContext=true")
80+
81+
// force the netty event loop into constrained territory
82+
systemProperty("io.netty.eventLoopThreads", "2")
83+
// ensure concurrent tests are competing for netty workers
84+
systemProperty("com.twitter.finagle.netty4.numWorkers", "2")
85+
// ensure concurrent tests are competing for offload pool workers
86+
systemProperty("com.twitter.finagle.offload.numWorkers", "2")
87+
6288
systemProperty(
6389
"metadataConfig",
6490
"otel.instrumentation.http.client.emit-experimental-telemetry=true," +

‎instrumentation/finagle-http-23.11/javaagent/src/main/java/com/twitter/finagle/Netty4HttpPackageHelpers.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package com.twitter.finagle;
7+
8+
import com.twitter.finagle.netty4.http.package$;
9+
10+
public class Netty4HttpPackageHelpers {
11+
private Netty4HttpPackageHelpers() {}
12+
13+
public static String getHttpCodecName() {
14+
return package$.MODULE$.HttpCodecName();
15+
}
16+
}

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
7+
8+
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
9+
import static net.bytebuddy.matcher.ElementMatchers.named;
10+
11+
import com.twitter.finagle.http.Request;
12+
import io.netty.handler.codec.http.FullHttpRequest;
13+
import io.netty.handler.codec.http.HttpRequest;
14+
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
15+
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
16+
import net.bytebuddy.asm.Advice;
17+
import net.bytebuddy.description.type.TypeDescription;
18+
import net.bytebuddy.matcher.ElementMatcher;
19+
20+
/** Bridges the Context from Netty request types to finagle request types. */
21+
class BijectionsNettyInstrumentation implements TypeInstrumentation {
22+
23+
@Override
24+
public ElementMatcher<TypeDescription> typeMatcher() {
25+
return named("com.twitter.finagle.netty4.http.Bijections$netty$");
26+
}
27+
28+
@Override
29+
public void transform(TypeTransformer transformer) {
30+
transformer.applyAdviceToMethod(
31+
isMethod().and(named("fullRequestToFinagle")), getClass().getName() + "$FullRequestAdvice");
32+
transformer.applyAdviceToMethod(
33+
isMethod().and(named("chunkedRequestToFinagle")),
34+
getClass().getName() + "$ChunkedRequestAdvice");
35+
}
36+
37+
@SuppressWarnings("unused")
38+
public static class FullRequestAdvice {
39+
@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
40+
public static void onApplyExit(
41+
@Advice.Return Request ret, @Advice.Argument(0) FullHttpRequest in) {
42+
Helpers.chainContextToFinagle(in, ret);
43+
}
44+
}
45+
46+
@SuppressWarnings("unused")
47+
public static class ChunkedRequestAdvice {
48+
@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
49+
public static void onApplyExit(@Advice.Return Request ret, @Advice.Argument(0) HttpRequest in) {
50+
Helpers.chainContextToFinagle(in, ret);
51+
}
52+
}
53+
}

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

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,17 @@
55

66
package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
77

8+
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
89
import static net.bytebuddy.matcher.ElementMatchers.named;
910

10-
import io.opentelemetry.context.Context;
11-
import io.opentelemetry.context.Scope;
11+
import io.netty.channel.Channel;
1212
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1313
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
14-
import javax.annotation.Nullable;
1514
import net.bytebuddy.asm.Advice;
1615
import net.bytebuddy.description.type.TypeDescription;
1716
import net.bytebuddy.matcher.ElementMatcher;
18-
import scala.Option;
1917

18+
/** Amends the tail of the Netty pipeline to bridge the netty request to its finagle request. */
2019
class ChannelTransportInstrumentation implements TypeInstrumentation {
2120
@Override
2221
public ElementMatcher<TypeDescription> typeMatcher() {
@@ -25,27 +24,15 @@ public ElementMatcher<TypeDescription> typeMatcher() {
2524

2625
@Override
2726
public void transform(TypeTransformer transformer) {
28-
transformer.applyAdviceToMethod(named("write"), getClass().getName() + "$WriteAdvice");
27+
transformer.applyAdviceToMethod(isConstructor(), getClass().getName() + "$ConstructorAdvice");
2928
}
3029

3130
@SuppressWarnings("unused")
32-
public static class WriteAdvice {
31+
public static class ConstructorAdvice {
3332

34-
@Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
35-
@Nullable
36-
public static Scope methodEnter() {
37-
Option<Context> ref = Helpers.contextLocal().apply();
38-
if (ref.isDefined()) {
39-
return ref.get().makeCurrent();
40-
}
41-
return null;
42-
}
43-
44-
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
45-
public static void methodExit(@Advice.Enter @Nullable Scope scope) {
46-
if (scope != null) {
47-
scope.close();
48-
}
33+
@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
34+
public static void methodExit(@Advice.Argument(0) Channel ch) {
35+
Helpers.mutateHandlerPipeline(ch);
4936
}
5037
}
5138
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ public FinagleHttpInstrumentationModule() {
2424
@Override
2525
public List<TypeInstrumentation> typeInstrumentations() {
2626
return asList(
27+
new BijectionsNettyInstrumentation(),
2728
new GenStreamingServerDispatcherInstrumentation(),
2829
new ChannelTransportInstrumentation(),
2930
new H2StreamChannelInitInstrumentation());
@@ -40,12 +41,14 @@ public List<String> injectedClassNames() {
4041
// these are injected so that they can access package-private members
4142
return asList(
4243
"com.twitter.finagle.ChannelTransportHelpers",
44+
"com.twitter.finagle.Netty4HttpPackageHelpers",
4345
"io.netty.channel.OpenTelemetryChannelInitializerDelegate");
4446
}
4547

4648
@Override
4749
public boolean isHelperClass(String className) {
4850
return className.equals("com.twitter.finagle.ChannelTransportHelpers")
51+
|| className.equals("com.twitter.finagle.Netty4HttpPackageHelpers")
4952
|| className.equals("io.netty.channel.OpenTelemetryChannelInitializerDelegate");
5053
}
5154
}

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

Lines changed: 0 additions & 24 deletions
This file was deleted.

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
7+
8+
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
9+
import static net.bytebuddy.matcher.ElementMatchers.named;
10+
11+
import com.twitter.util.Future;
12+
import com.twitter.util.Try;
13+
import io.opentelemetry.context.Context;
14+
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
15+
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
16+
import net.bytebuddy.asm.Advice;
17+
import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument;
18+
import net.bytebuddy.description.type.TypeDescription;
19+
import net.bytebuddy.matcher.ElementMatcher;
20+
import scala.Function1;
21+
import scala.runtime.BoxedUnit;
22+
23+
/** Instruments additional Future types that aren't captured by Promise.K. */
24+
class FutureInstrumentation implements TypeInstrumentation {
25+
26+
@Override
27+
public ElementMatcher<TypeDescription> typeMatcher() {
28+
return named("com.twitter.util.ConstFuture");
29+
}
30+
31+
@Override
32+
public void transform(TypeTransformer transformer) {
33+
transformer.applyAdviceToMethod(
34+
isMethod().and(named("respond")), getClass().getName() + "$RespondAdvice");
35+
36+
// transformTry is documented as not being run in the scheduler, so it's not handled
37+
transformer.applyAdviceToMethod(
38+
isMethod().and(named("transform")), getClass().getName() + "$TransformAdvice");
39+
}
40+
41+
@SuppressWarnings("unused")
42+
public static class RespondAdvice {
43+
@Advice.OnMethodEnter(suppress = Throwable.class)
44+
@Advice.AssignReturned.ToArguments(@ToArgument(0))
45+
public static Function1<Try<?>, BoxedUnit> onEnter(
46+
@Advice.Argument(0) Function1<Try<?>, BoxedUnit> f) {
47+
return TwitterUtilCoreHelpers.wrap(Context.current(), f);
48+
}
49+
}
50+
51+
@SuppressWarnings("unused")
52+
public static class TransformAdvice {
53+
@Advice.OnMethodEnter(suppress = Throwable.class)
54+
@Advice.AssignReturned.ToArguments(@ToArgument(0))
55+
public static Function1<Try<?>, Future<?>> onEnter(
56+
@Advice.Argument(0) Function1<Try<?>, Future<?>> f) {
57+
return TwitterUtilCoreHelpers.wrap(Context.current(), f);
58+
}
59+
}
60+
}

instrumentation/finagle-http-23.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/finaglehttp/v23_11/PromiseMonitoredInstrumentation.java renamed to instrumentation/finagle-http-23.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/finaglehttp/v23_11/FuturePoolInstrumentation.java

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,43 +5,41 @@
55

66
package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
77

8-
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
8+
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
99
import static net.bytebuddy.matcher.ElementMatchers.named;
10-
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
1110

11+
import io.opentelemetry.context.Context;
1212
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1313
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
1414
import net.bytebuddy.asm.Advice;
1515
import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument;
1616
import net.bytebuddy.description.type.TypeDescription;
1717
import net.bytebuddy.matcher.ElementMatcher;
18-
import scala.Function1;
18+
import scala.Function0;
1919

20-
class PromiseMonitoredInstrumentation implements TypeInstrumentation {
20+
/**
21+
* Instruments {@link com.twitter.util.ExecutorServiceFuturePool#apply} to wrap the submitted {@link
22+
* Function0} so it executes under the caller's otel {@link Context}.
23+
*/
24+
class FuturePoolInstrumentation implements TypeInstrumentation {
2125

2226
@Override
2327
public ElementMatcher<TypeDescription> typeMatcher() {
24-
return named("com.twitter.util.Promise$Monitored");
28+
return named("com.twitter.util.ExecutorServiceFuturePool");
2529
}
2630

2731
@Override
2832
public void transform(TypeTransformer transformer) {
2933
transformer.applyAdviceToMethod(
30-
isConstructor().and(takesArgument(1, named("scala.Function1"))),
31-
getClass().getName() + "$WrapFunctionAdvice");
34+
isMethod().and(named("apply")), getClass().getName() + "$ApplyAdvice");
3235
}
3336

3437
@SuppressWarnings("unused")
35-
public static class WrapFunctionAdvice {
36-
37-
@Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
38-
@Advice.AssignReturned.ToArguments(@ToArgument(1))
39-
public static Function1<?, ?> wrap(@Advice.Argument(1) Function1<?, ?> function1) {
40-
if (function1 == null) {
41-
return null;
42-
}
43-
44-
return Function1Wrapper.wrap(function1);
38+
public static class ApplyAdvice {
39+
@Advice.OnMethodEnter(suppress = Throwable.class)
40+
@Advice.AssignReturned.ToArguments(@ToArgument(0))
41+
public static Function0<?> onApplyEnter(@Advice.Argument(0) Function0<?> f) {
42+
return TwitterUtilCoreHelpers.wrap(Context.current(), f);
4543
}
4644
}
4745
}

0 commit comments

Comments
 (0)