Netty: preserve caught exception in the context instead of calling end() (#4413)

Mateusz Rzeszutek · web-flow · commit 2e97a4d3d5ff · 2021-10-19T13:40:11.000-07:00
diff --git a/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts b/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts
@@ -21,6 +21,8 @@ dependencies {
   compileOnly("com.google.auto.value:auto-value-annotations")
   annotationProcessor("com.google.auto.value:auto-value")
 
+  implementation(project(":instrumentation:netty:netty-common:javaagent"))
+
   compileOnly("io.netty:netty:3.8.0.Final")
 
   testLibrary("io.netty:netty:3.8.0.Final")
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/DefaultChannelPipelineInstrumentation.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/DefaultChannelPipelineInstrumentation.java
@@ -12,7 +12,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
-import io.opentelemetry.javaagent.instrumentation.netty.v3_8.server.NettyServerErrorHandler;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -39,7 +39,7 @@ public static class NotifyHandlerExceptionAdvice {
     @Advice.OnMethodEnter
     public static void onEnter(@Advice.Argument(1) Throwable throwable) {
       if (throwable != null) {
-        NettyServerErrorHandler.onError(Java8BytecodeBridge.currentContext(), throwable);
+        NettyErrorHolder.set(Java8BytecodeBridge.currentContext(), throwable);
       }
     }
   }
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/HttpClientResponseTracingHandler.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/HttpClientResponseTracingHandler.java
@@ -9,6 +9,7 @@
 
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.api.field.VirtualField;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import org.jboss.netty.channel.Channel;
 import org.jboss.netty.channel.ChannelHandlerContext;
 import org.jboss.netty.channel.MessageEvent;
@@ -35,7 +36,7 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent msg) {
               requestAndContexts.context(),
               requestAndContexts.request(),
               (HttpResponse) msg.getMessage(),
-              null);
+              NettyErrorHolder.getOrDefault(requestAndContexts.context(), null));
       requestContextsField.set(ctx.getChannel(), null);
     }
 
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyClientSingletons.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyClientSingletons.java
@@ -11,6 +11,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v3_8.HttpRequestAndChannel;
 import org.jboss.netty.handler.codec.http.HttpResponse;
 
@@ -35,6 +36,8 @@ final class NettyClientSingletons {
             .addAttributesExtractor(
                 PeerServiceAttributesExtractor.create(netClientAttributesExtractor))
             .addRequestMetrics(HttpClientMetrics.get())
+            .addContextCustomizer(
+                (context, requestAndChannel, startAttributes) -> NettyErrorHolder.init(context))
             .newClientInstrumenter(new HttpRequestHeadersSetter());
   }
 
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java
@@ -10,6 +10,7 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.api.field.VirtualField;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v3_8.HttpRequestAndChannel;
 import org.jboss.netty.channel.Channel;
 import org.jboss.netty.channel.ChannelHandlerContext;
@@ -35,12 +36,15 @@ public void writeRequested(ChannelHandlerContext ctx, MessageEvent msg) {
     HttpRequestAndChannel request = requestAndContext.request();
     HttpResponse response = (HttpResponse) msg.getMessage();
 
+    Throwable error = null;
     try (Scope ignored = context.makeCurrent()) {
       ctx.sendDownstream(msg);
-      instrumenter().end(context, request, response, null);
-    } catch (Throwable throwable) {
-      instrumenter().end(context, request, response, throwable);
-      throw throwable;
+    } catch (Throwable t) {
+      error = t;
+      throw t;
+    } finally {
+      error = NettyErrorHolder.getOrDefault(context, error);
+      instrumenter().end(context, request, response, error);
     }
   }
 }
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerErrorHandler.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerErrorHandler.java
diff --git a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerSingletons.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerSingletons.java
@@ -10,6 +10,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v3_8.HttpRequestAndChannel;
 import org.jboss.netty.handler.codec.http.HttpResponse;
 
@@ -30,6 +31,8 @@ final class NettyServerSingletons {
             .addAttributesExtractor(httpServerAttributesExtractor)
             .addAttributesExtractor(new NettyNetServerAttributesExtractor())
             .addRequestMetrics(HttpServerMetrics.get())
+            .addContextCustomizer(
+                (context, requestAndChannel, startAttributes) -> NettyErrorHolder.init(context))
             .newServerInstrumenter(new NettyHeadersGetter());
   }
 
diff --git a/instrumentation/netty/netty-4-common/javaagent/build.gradle.kts b/instrumentation/netty/netty-4-common/javaagent/build.gradle.kts
@@ -6,5 +6,7 @@ dependencies {
   compileOnly("com.google.auto.value:auto-value-annotations")
   annotationProcessor("com.google.auto.value:auto-value")
 
+  api(project(":instrumentation:netty:netty-common:javaagent"))
+
   compileOnly("io.netty:netty-codec-http:4.0.0.Final")
 }
diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyErrorHandler.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyErrorHandler.java
diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/server/NettyServerInstrumenterFactory.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/server/NettyServerInstrumenterFactory.java
@@ -14,6 +14,7 @@
 import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
 import io.opentelemetry.javaagent.instrumentation.netty.common.NettyCommonNetAttributesExtractor;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 
 public final class NettyServerInstrumenterFactory {
 
@@ -33,6 +34,7 @@ public static Instrumenter<HttpRequestAndChannel, HttpResponse> create(
         .addRequestMetrics(HttpServerMetrics.get())
         .addContextCustomizer(
             (context, request, attributes) -> {
+              context = NettyErrorHolder.init(context);
               // netty is not exactly a "container", but it's the best match out of these
               return ServerSpanNaming.init(context, ServerSpanNaming.Source.CONTAINER);
             })
diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/AbstractChannelHandlerContextInstrumentation.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/AbstractChannelHandlerContextInstrumentation.java
@@ -17,7 +17,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
-import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHandler;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -62,7 +62,7 @@ public static void onEnter(
 
       Context serverContext = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT).get();
       if (serverContext != null) {
-        NettyErrorHandler.onError(serverContext, throwable);
+        NettyErrorHolder.set(serverContext, throwable);
       }
     }
   }
diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java
@@ -15,6 +15,7 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_0.AttributeKeys;
 import javax.annotation.Nullable;
 
@@ -41,6 +42,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) {
   private static void end(Channel channel, HttpResponse response, @Nullable Throwable error) {
     Context context = channel.attr(AttributeKeys.SERVER_CONTEXT).getAndRemove();
     HttpRequestAndChannel request = channel.attr(AttributeKeys.SERVER_REQUEST).getAndRemove();
+    error = NettyErrorHolder.getOrDefault(context, error);
     instrumenter().end(context, request, response, error);
   }
 }
diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/AbstractChannelHandlerContextInstrumentation.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/AbstractChannelHandlerContextInstrumentation.java
@@ -17,7 +17,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
-import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHandler;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_1.client.NettyClientSingletons;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
@@ -60,7 +60,7 @@ public static void onEnter(
 
       Context serverContext = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT).get();
       if (serverContext != null) {
-        NettyErrorHandler.onError(serverContext, throwable);
+        NettyErrorHolder.set(serverContext, throwable);
       }
     }
   }
diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/server/HttpServerResponseTracingHandler.java
@@ -20,6 +20,7 @@
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.netty.v4_1.AttributeKeys;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import javax.annotation.Nullable;
 
 public class HttpServerResponseTracingHandler extends ChannelOutboundHandlerAdapter {
@@ -87,6 +88,7 @@ private static void end(
       Channel channel, @Nullable HttpResponse response, @Nullable Throwable error) {
     Context context = channel.attr(AttributeKeys.SERVER_CONTEXT).getAndRemove();
     HttpRequestAndChannel request = channel.attr(NettyServerSingletons.HTTP_REQUEST).getAndRemove();
+    error = NettyErrorHolder.getOrDefault(context, error);
     instrumenter().end(context, request, response, error);
   }
 }
diff --git a/instrumentation/netty/netty-common/javaagent/build.gradle.kts b/instrumentation/netty/netty-common/javaagent/build.gradle.kts
@@ -0,0 +1,3 @@
+plugins {
+  id("otel.javaagent-instrumentation")
+}
diff --git a/instrumentation/netty/netty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyErrorHolder.java b/instrumentation/netty/netty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyErrorHolder.java
@@ -0,0 +1,51 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common;
+
+import static io.opentelemetry.context.ContextKey.named;
+
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.ContextKey;
+import io.opentelemetry.context.ImplicitContextKeyed;
+import javax.annotation.Nullable;
+
+public final class NettyErrorHolder implements ImplicitContextKeyed {
+
+  private static final ContextKey<NettyErrorHolder> KEY = named("opentelemetry-netty-error");
+
+  private volatile Throwable error;
+
+  private NettyErrorHolder() {}
+
+  public static Context init(Context context) {
+    if (context.get(KEY) != null) {
+      return context;
+    }
+    return context.with(new NettyErrorHolder());
+  }
+
+  public static void set(Context context, Throwable error) {
+    NettyErrorHolder holder = context.get(KEY);
+    if (holder != null) {
+      holder.error = error;
+    }
+  }
+
+  @Nullable
+  public static Throwable getOrDefault(Context context, @Nullable Throwable error) {
+    Throwable result = null;
+    NettyErrorHolder holder = context.get(KEY);
+    if (holder != null) {
+      result = holder.error;
+    }
+    return result == null ? error : result;
+  }
+
+  @Override
+  public Context storeInContext(Context context) {
+    return context.with(KEY, this);
+  }
+}
diff --git a/settings.gradle.kts b/settings.gradle.kts
@@ -256,6 +256,7 @@ include(":instrumentation:netty:netty-4.0:javaagent")
 include(":instrumentation:netty:netty-4.1:library")
 include(":instrumentation:netty:netty-4.1:javaagent")
 include(":instrumentation:netty:netty-4-common:javaagent")
+include(":instrumentation:netty:netty-common:javaagent")
 include(":instrumentation:okhttp:okhttp-2.2:javaagent")
 include(":instrumentation:okhttp:okhttp-3.0:javaagent")
 include(":instrumentation:okhttp:okhttp-3.0:library")

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@`
`12`	`12`	`import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;`
`13`	`13`	`import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;`
`14`	`14`	`import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;`
`15`		`-import io.opentelemetry.javaagent.instrumentation.netty.v3_8.server.NettyServerErrorHandler;`
	`15`	`+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;`
`16`	`16`	`import net.bytebuddy.asm.Advice;`
`17`	`17`	`import net.bytebuddy.description.type.TypeDescription;`
`18`	`18`	`import net.bytebuddy.matcher.ElementMatcher;`
`@@ -39,7 +39,7 @@ public static class NotifyHandlerExceptionAdvice {`
`39`	`39`	`@Advice.OnMethodEnter`
`40`	`40`	`public static void onEnter(@Advice.Argument(1) Throwable throwable) {`
`41`	`41`	`if (throwable != null) {`
`42`		`- NettyServerErrorHandler.onError(Java8BytecodeBridge.currentContext(), throwable);`
	`42`	`+ NettyErrorHolder.set(Java8BytecodeBridge.currentContext(), throwable);`
`43`	`43`	`}`
`44`	`44`	`}`
`45`	`45`	`}`
Original file line number	Diff line number	Diff line change
`@@ -6,5 +6,7 @@ dependencies {`
`6`	`6`	`compileOnly("com.google.auto.value:auto-value-annotations")`
`7`	`7`	`annotationProcessor("com.google.auto.value:auto-value")`
`8`	`8`
	`9`	`+ api(project(":instrumentation:netty:netty-common:javaagent"))`
	`10`	`+`
`9`	`11`	`compileOnly("io.netty:netty-codec-http:4.0.0.Final")`
`10`	`12`	`}`