From 849e0d5229a1085a80de3867b7a013cf26aaa8d6 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 10 Aug 2023 03:39:25 -0700 Subject: [PATCH] Combine two netty context attributes into one (#9160) --- ...tChannelHandlerContextInstrumentation.java | 7 +-- .../netty/netty-4.1/library/build.gradle.kts | 3 ++ .../netty/v4_1/internal/AttributeKeys.java | 2 +- .../netty/v4_1/internal/ServerContext.java | 31 +++++++++++++ .../HttpServerRequestTracingHandler.java | 33 ++++++-------- .../HttpServerResponseTracingHandler.java | 43 ++++++++++--------- .../ratpack/TracingHandler.java | 10 ++--- .../ContextHandlerInstrumentation.java | 10 ++--- .../HttpTrafficHandlerInstrumentation.java | 10 ++--- 9 files changed, 89 insertions(+), 60 deletions(-) create mode 100644 instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/ServerContext.java 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 index 4944daacf3..ae5f3e0f43 100644 --- 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,6 +17,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.netty.common.internal.NettyErrorHolder; import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.Deque; @@ -60,10 +61,10 @@ public class AbstractChannelHandlerContextInstrumentation implements TypeInstrum instrumenter().end(clientContext, request, null, throwable); return; } - Deque contexts = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT).get(); - Context serverContext = contexts != null ? contexts.peekFirst() : null; + Deque serverContexts = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT).get(); + ServerContext serverContext = serverContexts != null ? serverContexts.peekFirst() : null; if (serverContext != null) { - NettyErrorHolder.set(serverContext, throwable); + NettyErrorHolder.set(serverContext.context(), throwable); } } } diff --git a/instrumentation/netty/netty-4.1/library/build.gradle.kts b/instrumentation/netty/netty-4.1/library/build.gradle.kts index eb20fd9db5..08f8adf1da 100644 --- a/instrumentation/netty/netty-4.1/library/build.gradle.kts +++ b/instrumentation/netty/netty-4.1/library/build.gradle.kts @@ -7,5 +7,8 @@ dependencies { implementation(project(":instrumentation:netty:netty-4-common:library")) implementation(project(":instrumentation:netty:netty-common:library")) + compileOnly("com.google.auto.value:auto-value-annotations") + annotationProcessor("com.google.auto.value:auto-value") + testImplementation(project(":instrumentation:netty:netty-4.1:testing")) } diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/AttributeKeys.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/AttributeKeys.java index 2cbb44573b..f18bd38bc8 100644 --- a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/AttributeKeys.java +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/AttributeKeys.java @@ -18,7 +18,7 @@ public final class AttributeKeys { // this is the context that has the server span // // note: this attribute key is also used by ratpack instrumentation - public static final AttributeKey> SERVER_CONTEXT = + public static final AttributeKey> SERVER_CONTEXT = AttributeKey.valueOf(AttributeKeys.class, "server-context"); public static final AttributeKey CLIENT_CONTEXT = diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/ServerContext.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/ServerContext.java new file mode 100644 index 0000000000..0bbbd7ec37 --- /dev/null +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/ServerContext.java @@ -0,0 +1,31 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.netty.v4_1.internal; + +import com.google.auto.value.AutoValue; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; + +/** + * A tuple of an {@link Context} and a {@link HttpRequestAndChannel}. + * + *

This class is internal and is hence not for public use. Its APIs are unstable and can change + * at any time. + */ +@AutoValue +public abstract class ServerContext { + + /** Create a new {@link ServerContext}. */ + public static ServerContext create(Context context, HttpRequestAndChannel request) { + return new AutoValue_ServerContext(context, request); + } + + /** Returns the {@link Context}. */ + public abstract Context context(); + + /** Returns the {@link HttpRequestAndChannel}. */ + public abstract HttpRequestAndChannel request(); +} diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerRequestTracingHandler.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerRequestTracingHandler.java index 2a198bae88..69f90c7e88 100644 --- a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerRequestTracingHandler.java +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerRequestTracingHandler.java @@ -17,6 +17,7 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import java.util.ArrayDeque; import java.util.Deque; @@ -26,9 +27,6 @@ import java.util.Deque; */ public class HttpServerRequestTracingHandler extends ChannelInboundHandlerAdapter { - static final AttributeKey> HTTP_SERVER_REQUEST = - AttributeKey.valueOf(HttpServerRequestTracingHandler.class, "http-server-request"); - private final Instrumenter instrumenter; public HttpServerRequestTracingHandler( @@ -39,14 +37,14 @@ public class HttpServerRequestTracingHandler extends ChannelInboundHandlerAdapte @Override public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { Channel channel = ctx.channel(); - Deque contexts = getOrCreate(channel, AttributeKeys.SERVER_CONTEXT); + Deque serverContexts = getOrCreate(channel, AttributeKeys.SERVER_CONTEXT); if (!(msg instanceof HttpRequest)) { - Context serverContext = contexts.peekLast(); + ServerContext serverContext = serverContexts.peekLast(); if (serverContext == null) { super.channelRead(ctx, msg); } else { - try (Scope ignored = serverContext.makeCurrent()) { + try (Scope ignored = serverContext.context().makeCurrent()) { super.channelRead(ctx, msg); } } @@ -61,16 +59,15 @@ public class HttpServerRequestTracingHandler extends ChannelInboundHandlerAdapte } Context context = instrumenter.start(parentContext, request); - contexts.addLast(context); - Deque requests = getOrCreate(channel, HTTP_SERVER_REQUEST); - requests.addLast(request); + serverContexts.addLast(ServerContext.create(context, request)); try (Scope ignored = context.makeCurrent()) { super.channelRead(ctx, msg); // the span is ended normally in HttpServerResponseTracingHandler } catch (Throwable throwable) { // make sure to remove the server context on end() call - instrumenter.end(contexts.removeLast(), requests.removeLast(), null, throwable); + ServerContext serverContext = serverContexts.removeLast(); + instrumenter.end(serverContext.context(), serverContext.request(), null, throwable); throw throwable; } } @@ -78,20 +75,16 @@ public class HttpServerRequestTracingHandler extends ChannelInboundHandlerAdapte @Override public void channelInactive(ChannelHandlerContext ctx) { // connection was closed, close all remaining requests - Attribute> contextAttr = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT); - Deque contexts = contextAttr.get(); - Attribute> requestAttr = ctx.channel().attr(HTTP_SERVER_REQUEST); - Deque requests = requestAttr.get(); + Attribute> contextAttr = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT); + Deque serverContexts = contextAttr.get(); - if (contexts == null || requests == null) { + if (serverContexts == null) { return; } - while (!contexts.isEmpty() || !requests.isEmpty()) { - Context context = contexts.pollFirst(); - HttpRequestAndChannel request = requests.pollFirst(); - - instrumenter.end(context, request, null, null); + ServerContext serverContext; + while ((serverContext = serverContexts.pollFirst()) != null) { + instrumenter.end(serverContext.context(), serverContext.request(), null, null); } } diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerResponseTracingHandler.java index 4fee50df88..22ab372f88 100644 --- a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/internal/server/HttpServerResponseTracingHandler.java @@ -5,8 +5,6 @@ package io.opentelemetry.instrumentation.netty.v4_1.internal.server; -import static io.opentelemetry.instrumentation.netty.v4_1.internal.server.HttpServerRequestTracingHandler.HTTP_SERVER_REQUEST; - import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelOutboundHandlerAdapter; @@ -22,6 +20,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.netty.common.internal.NettyErrorHolder; import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import java.util.Deque; import javax.annotation.Nullable; @@ -46,17 +45,15 @@ public class HttpServerResponseTracingHandler extends ChannelOutboundHandlerAdap @Override public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) throws Exception { - Attribute> contextAttr = ctx.channel().attr(AttributeKeys.SERVER_CONTEXT); + Attribute> serverContextAttr = + ctx.channel().attr(AttributeKeys.SERVER_CONTEXT); - Deque contexts = contextAttr.get(); - Context context = contexts != null ? contexts.peekFirst() : null; - if (context == null) { + Deque serverContexts = serverContextAttr.get(); + ServerContext serverContext = serverContexts != null ? serverContexts.peekFirst() : null; + if (serverContext == null) { super.write(ctx, msg, prm); return; } - Attribute> requestAttr = ctx.channel().attr(HTTP_SERVER_REQUEST); - Deque requests = requestAttr.get(); - HttpRequestAndChannel request = requests != null ? requests.peekFirst() : null; ChannelPromise writePromise; @@ -73,35 +70,39 @@ public class HttpServerResponseTracingHandler extends ChannelOutboundHandlerAdap // Going to finish the span after the write of the last content finishes. if (msg instanceof FullHttpResponse) { // Headers and body all sent together, we have the response information in the msg. - beforeCommitHandler.handle(context, (HttpResponse) msg); - contexts.removeFirst(); - requests.removeFirst(); + beforeCommitHandler.handle(serverContext.context(), (HttpResponse) msg); + serverContexts.removeFirst(); writePromise.addListener( - future -> end(context, request, (FullHttpResponse) msg, writePromise)); + future -> + end( + serverContext.context(), + serverContext.request(), + (FullHttpResponse) msg, + writePromise)); } else { // Body sent after headers. We stored the response information in the context when // encountering HttpResponse (which was not FullHttpResponse since it's not // LastHttpContent). - contexts.removeFirst(); - requests.removeFirst(); + serverContexts.removeFirst(); HttpResponse response = ctx.channel().attr(HTTP_SERVER_RESPONSE).getAndSet(null); - writePromise.addListener(future -> end(context, request, response, writePromise)); + writePromise.addListener( + future -> + end(serverContext.context(), serverContext.request(), response, writePromise)); } } else { writePromise = prm; if (msg instanceof HttpResponse) { // Headers before body has been sent, store them to use when finishing the span. - beforeCommitHandler.handle(context, (HttpResponse) msg); + beforeCommitHandler.handle(serverContext.context(), (HttpResponse) msg); ctx.channel().attr(HTTP_SERVER_RESPONSE).set((HttpResponse) msg); } } - try (Scope ignored = context.makeCurrent()) { + try (Scope ignored = serverContext.context().makeCurrent()) { super.write(ctx, msg, writePromise); } catch (Throwable throwable) { - contexts.removeFirst(); - requests.removeFirst(); - end(context, request, null, throwable); + serverContexts.removeFirst(); + end(serverContext.context(), serverContext.request(), null, throwable); throw throwable; } } diff --git a/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/TracingHandler.java b/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/TracingHandler.java index 1cc7d5f82e..eb61bbf5f5 100644 --- a/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/TracingHandler.java +++ b/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/TracingHandler.java @@ -12,6 +12,7 @@ import static io.opentelemetry.javaagent.instrumentation.ratpack.RatpackSingleto import io.netty.util.Attribute; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge; import java.util.Deque; import ratpack.handling.Context; @@ -25,17 +26,16 @@ public final class TracingHandler implements Handler { @Override public void handle(Context ctx) { - Attribute> serverContextAttribute = + Attribute> serverContextAttribute = ctx.getDirectChannelAccess().getChannel().attr(AttributeKeys.SERVER_CONTEXT); - Deque contexts = serverContextAttribute.get(); - io.opentelemetry.context.Context serverSpanContext = - contexts != null ? contexts.peekFirst() : null; + Deque serverContexts = serverContextAttribute.get(); + ServerContext serverContext = serverContexts != null ? serverContexts.peekFirst() : null; // Must use context from channel, as executor instrumentation is not accurate - Ratpack // internally queues events and then drains them in batches, causing executor instrumentation to // attach the same context to a batch of events from different requests. io.opentelemetry.context.Context parentOtelContext = - serverSpanContext != null ? serverSpanContext : Java8BytecodeBridge.currentContext(); + serverContext != null ? serverContext.context() : Java8BytecodeBridge.currentContext(); io.opentelemetry.context.Context callbackContext; if (instrumenter().shouldStart(parentOtelContext, INITIAL_SPAN_NAME)) { diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/ContextHandlerInstrumentation.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/ContextHandlerInstrumentation.java index 26844726a9..aa51160e5f 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/ContextHandlerInstrumentation.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/ContextHandlerInstrumentation.java @@ -9,9 +9,9 @@ import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import io.netty.channel.Channel; -import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.Deque; @@ -39,10 +39,10 @@ public class ContextHandlerInstrumentation implements TypeInstrumentation { @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope onEnter(@Advice.Argument(0) Channel channel) { // set context to the first unprocessed request - Deque contexts = channel.attr(AttributeKeys.SERVER_CONTEXT).get(); - Context context = contexts != null ? contexts.peekFirst() : null; - if (context != null) { - return context.makeCurrent(); + Deque serverContextx = channel.attr(AttributeKeys.SERVER_CONTEXT).get(); + ServerContext serverContext = serverContextx != null ? serverContextx.peekFirst() : null; + if (serverContext != null) { + return serverContext.context().makeCurrent(); } return null; } diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/HttpTrafficHandlerInstrumentation.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/HttpTrafficHandlerInstrumentation.java index 70f8b7537d..efe171194b 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/HttpTrafficHandlerInstrumentation.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/reactornetty/HttpTrafficHandlerInstrumentation.java @@ -9,9 +9,9 @@ import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesNoArguments; import io.netty.channel.ChannelHandlerContext; -import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys; +import io.opentelemetry.instrumentation.netty.v4_1.internal.ServerContext; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.Deque; @@ -40,11 +40,11 @@ public class HttpTrafficHandlerInstrumentation implements TypeInstrumentation { public static Scope onEnter( @Advice.FieldValue("ctx") ChannelHandlerContext channelHandlerContext) { // set context to the first unprocessed request - Deque contexts = + Deque serverContexts = channelHandlerContext.channel().attr(AttributeKeys.SERVER_CONTEXT).get(); - Context context = contexts != null ? contexts.peekFirst() : null; - if (context != null) { - return context.makeCurrent(); + ServerContext serverContext = serverContexts != null ? serverContexts.peekFirst() : null; + if (serverContext != null) { + return serverContext.context().makeCurrent(); } return null; }