fix: corrects and stabilizes finagle instrumentation for http

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

@@ -0,0 +1,3 @@
+plugins {
+  id("otel.java-conventions")
+}

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

@@ -0,0 +1,18 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.twitter.util;
+
+// public accessible stubs of mirrored types in com.twitter.util to ensure compilation;
+// these classes are consumed as a compileOnly module and replaced at runtime with their
+// stubbed counterparts
+public class Promise {
+  private Promise() {}
+
+  @SuppressWarnings("ClassNamedLikeTypeParameter")
+  public abstract static class K {}
+
+  public abstract static class Interruptible {}
+}

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

@@ -32,11 +32,17 @@ dependencies {
 
   library("${scalified("com.twitter:finagle-http")}:$finagleVersion")
 
-  // should wire netty contexts
   testInstrumentation(project(":instrumentation:netty:netty-4.1:javaagent"))
 
+  // Exclude the Promise stub and its nested classes;
+  // this allows us to compile against these types in the instrumentation
+  // despite them being private in their original inner class;
+  // this is required for VirtualField to have a concrete type to find/get/set on.
+  compileOnly(project(":instrumentation:finagle-http-23.11:compile-stub"))
+
   implementation(project(":instrumentation:netty:netty-4.1:javaagent"))
   implementation(project(":instrumentation:netty:netty-4.1:library"))
+  implementation(project(":instrumentation:netty:netty-common-4.0:javaagent"))
   implementation(project(":instrumentation:netty:netty-common-4.0:library"))
 }
 
@@ -48,6 +54,17 @@ tasks {
   }
 
   test {
+    jvmArgs("-Dotel.instrumentation.http.client.emit-experimental-telemetry=true")
+    jvmArgs("-Dotel.instrumentation.http.server.emit-experimental-telemetry=true")
+    jvmArgs("-Dio.opentelemetry.context.enableStrictContext=true")
+
+    // force the netty event loop into constrained territory
+    systemProperty("io.netty.eventLoopThreads", "2")
+    // ensure concurrent tests are competing for netty workers
+    systemProperty("com.twitter.finagle.netty4.numWorkers", "2")
+    // ensure concurrent tests are competing for offload pool workers
+    systemProperty("com.twitter.finagle.offload.numWorkers", "2")
+
     systemProperty(
       "metadataConfig",
       "otel.instrumentation.http.client.emit-experimental-telemetry=true," +
@@ -59,6 +76,15 @@ tasks {
     testClassesDirs = sourceSets.test.get().output.classesDirs
     classpath = sourceSets.test.get().runtimeClasspath
     jvmArgs("-Dotel.semconv-stability.opt-in=service.peer")
+    jvmArgs("-Dio.opentelemetry.context.enableStrictContext=true")
+
+    // force the netty event loop into constrained territory
+    systemProperty("io.netty.eventLoopThreads", "2")
+    // ensure concurrent tests are competing for netty workers
+    systemProperty("com.twitter.finagle.netty4.numWorkers", "2")
+    // ensure concurrent tests are competing for offload pool workers
+    systemProperty("com.twitter.finagle.offload.numWorkers", "2")
+
     systemProperty(
       "metadataConfig",
       "otel.instrumentation.http.client.emit-experimental-telemetry=true," +

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

@@ -0,0 +1,16 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.twitter.finagle;
+
+import com.twitter.finagle.netty4.http.package$;
+
+public class Netty4HttpPackageHelpers {
+  private Netty4HttpPackageHelpers() {}
+
+  public static String getHttpCodecName() {
+    return package$.MODULE$.HttpCodecName();
+  }
+}

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

@@ -0,0 +1,53 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
+
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+import com.twitter.finagle.http.Request;
+import io.netty.handler.codec.http.FullHttpRequest;
+import io.netty.handler.codec.http.HttpRequest;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+
+/** Bridges the Context from Netty request types to finagle request types. */
+class BijectionsNettyInstrumentation implements TypeInstrumentation {
+
+  @Override
+  public ElementMatcher<TypeDescription> typeMatcher() {
+    return named("com.twitter.finagle.netty4.http.Bijections$netty$");
+  }
+
+  @Override
+  public void transform(TypeTransformer transformer) {
+    transformer.applyAdviceToMethod(
+        isMethod().and(named("fullRequestToFinagle")), getClass().getName() + "$FullRequestAdvice");
+    transformer.applyAdviceToMethod(
+        isMethod().and(named("chunkedRequestToFinagle")),
+        getClass().getName() + "$ChunkedRequestAdvice");
+  }
+
+  @SuppressWarnings("unused")
+  public static class FullRequestAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    public static void onApplyExit(
+        @Advice.Return Request ret, @Advice.Argument(0) FullHttpRequest in) {
+      Helpers.chainContextToFinagle(in, ret);
+    }
+  }
+
+  @SuppressWarnings("unused")
+  public static class ChunkedRequestAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    public static void onApplyExit(@Advice.Return Request ret, @Advice.Argument(0) HttpRequest in) {
+      Helpers.chainContextToFinagle(in, ret);
+    }
+  }
+}

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

@@ -5,18 +5,17 @@
 
 package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
 
+import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 
-import io.opentelemetry.context.Context;
-import io.opentelemetry.context.Scope;
+import io.netty.channel.Channel;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
-import javax.annotation.Nullable;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
-import scala.Option;
 
+/** Amends the tail of the Netty pipeline to bridge the netty request to its finagle request. */
 class ChannelTransportInstrumentation implements TypeInstrumentation {
   @Override
   public ElementMatcher<TypeDescription> typeMatcher() {
@@ -25,27 +24,15 @@ public ElementMatcher<TypeDescription> typeMatcher() {
 
   @Override
   public void transform(TypeTransformer transformer) {
-    transformer.applyAdviceToMethod(named("write"), getClass().getName() + "$WriteAdvice");
+    transformer.applyAdviceToMethod(isConstructor(), getClass().getName() + "$ConstructorAdvice");
   }
 
   @SuppressWarnings("unused")
-  public static class WriteAdvice {
+  public static class ConstructorAdvice {
 
-    @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
-    @Nullable
-    public static Scope methodEnter() {
-      Option<Context> ref = Helpers.contextLocal().apply();
-      if (ref.isDefined()) {
-        return ref.get().makeCurrent();
-      }
-      return null;
-    }
-
-    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-    public static void methodExit(@Advice.Enter @Nullable Scope scope) {
-      if (scope != null) {
-        scope.close();
-      }
+    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    public static void methodExit(@Advice.Argument(0) Channel ch) {
+      Helpers.mutateHandlerPipeline(ch);
     }
   }
 }

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

@@ -24,6 +24,7 @@ public FinagleHttpInstrumentationModule() {
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(
+        new BijectionsNettyInstrumentation(),
         new GenStreamingServerDispatcherInstrumentation(),
         new ChannelTransportInstrumentation(),
         new H2StreamChannelInitInstrumentation());
@@ -40,12 +41,14 @@ public List<String> injectedClassNames() {
     // these are injected so that they can access package-private members
     return asList(
         "com.twitter.finagle.ChannelTransportHelpers",
+        "com.twitter.finagle.Netty4HttpPackageHelpers",
         "io.netty.channel.OpenTelemetryChannelInitializerDelegate");
   }
 
   @Override
   public boolean isHelperClass(String className) {
     return className.equals("com.twitter.finagle.ChannelTransportHelpers")
+        || className.equals("com.twitter.finagle.Netty4HttpPackageHelpers")
         || className.equals("io.netty.channel.OpenTelemetryChannelInitializerDelegate");
   }
 }

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

@@ -0,0 +1,60 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
+
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+import com.twitter.util.Future;
+import com.twitter.util.Try;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import scala.Function1;
+import scala.runtime.BoxedUnit;
+
+/** Instruments additional Future types that aren't captured by Promise.K. */
+class FutureInstrumentation implements TypeInstrumentation {
+
+  @Override
+  public ElementMatcher<TypeDescription> typeMatcher() {
+    return named("com.twitter.util.ConstFuture");
+  }
+
+  @Override
+  public void transform(TypeTransformer transformer) {
+    transformer.applyAdviceToMethod(
+        isMethod().and(named("respond")), getClass().getName() + "$RespondAdvice");
+
+    // transformTry is documented as not being run in the scheduler, so it's not handled
+    transformer.applyAdviceToMethod(
+        isMethod().and(named("transform")), getClass().getName() + "$TransformAdvice");
+  }
+
+  @SuppressWarnings("unused")
+  public static class RespondAdvice {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    @Advice.AssignReturned.ToArguments(@ToArgument(0))
+    public static Function1<Try<?>, BoxedUnit> onEnter(
+        @Advice.Argument(0) Function1<Try<?>, BoxedUnit> f) {
+      return TwitterUtilCoreHelpers.wrap(Context.current(), f);
+    }
+  }
+
+  @SuppressWarnings("unused")
+  public static class TransformAdvice {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    @Advice.AssignReturned.ToArguments(@ToArgument(0))
+    public static Function1<Try<?>, Future<?>> onEnter(
+        @Advice.Argument(0) Function1<Try<?>, Future<?>> f) {
+      return TwitterUtilCoreHelpers.wrap(Context.current(), f);
+    }
+  }
+}

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

@@ -5,43 +5,41 @@
 
 package io.opentelemetry.javaagent.instrumentation.finaglehttp.v23_11;
 
-import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
+import io.opentelemetry.context.Context;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
-import scala.Function1;
+import scala.Function0;
 
-class PromiseMonitoredInstrumentation implements TypeInstrumentation {
+/**
+ * Instruments {@link com.twitter.util.ExecutorServiceFuturePool#apply} to wrap the submitted {@link
+ * Function0} so it executes under the caller's otel {@link Context}.
+ */
+class FuturePoolInstrumentation implements TypeInstrumentation {
 
   @Override
   public ElementMatcher<TypeDescription> typeMatcher() {
-    return named("com.twitter.util.Promise$Monitored");
+    return named("com.twitter.util.ExecutorServiceFuturePool");
   }
 
   @Override
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
-        isConstructor().and(takesArgument(1, named("scala.Function1"))),
-        getClass().getName() + "$WrapFunctionAdvice");
+        isMethod().and(named("apply")), getClass().getName() + "$ApplyAdvice");
   }
 
   @SuppressWarnings("unused")
-  public static class WrapFunctionAdvice {
-
-    @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
-    @Advice.AssignReturned.ToArguments(@ToArgument(1))
-    public static Function1<?, ?> wrap(@Advice.Argument(1) Function1<?, ?> function1) {
-      if (function1 == null) {
-        return null;
-      }
-
-      return Function1Wrapper.wrap(function1);
+  public static class ApplyAdvice {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    @Advice.AssignReturned.ToArguments(@ToArgument(0))
+    public static Function0<?> onApplyEnter(@Advice.Argument(0) Function0<?> f) {
+      return TwitterUtilCoreHelpers.wrap(Context.current(), f);
     }
   }
 }