Netty: preserve caught exception in the context instead of calling en…

…d() (open-telemetry#4413)

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")

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);
       }
     }
   }

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);
     }
 

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());
   }
 

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);
     }
   }
 }

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());
   }
 

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")
 }

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);
             })

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);
       }
     }
   }

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);
   }
 }

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);
       }
     }
   }

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);
   }
 }

instrumentation/netty/netty-common/javaagent/build.gradle.kts

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

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);
+  }
+}

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")