From c3975f22a82021d142d13a949056be261d26bb55 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 27 Aug 2021 20:12:34 -0700 Subject: [PATCH 1/3] Fix netty strict context leaks --- .../netty/common/NettyFutureInstrumentation.java | 10 +++++++++- .../v4_1/client/HttpClientResponseTracingHandler.java | 8 ++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java index f51cc7999a9e..52c6a21e6e37 100644 --- a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java +++ b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java @@ -61,7 +61,15 @@ public static class AddListenerAdvice { public static void wrapListener( @Advice.Argument(value = 0, readOnly = false) GenericFutureListener> listener) { - listener = FutureListenerWrappers.wrap(Java8BytecodeBridge.currentContext(), listener); + // wrapping our "end" listener leads to strict context leak failures since there will be an + // active scope when we call "end" + // wrapping internal netty listeners also leads to strict context leak failures since some of + // those are called after our "end" listener + String listenerClassName = listener.getClass().getName(); + if (!listenerClassName.startsWith("io.opentelemetry.javaagent.") + && !listenerClassName.startsWith("io.netty.")) { + listener = FutureListenerWrappers.wrap(Java8BytecodeBridge.currentContext(), listener); + } } } diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/HttpClientResponseTracingHandler.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/HttpClientResponseTracingHandler.java index d416d71e2c3f..7e5ff4cbcdb9 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/HttpClientResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/HttpClientResponseTracingHandler.java @@ -36,7 +36,6 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) { Context parentContext = parentContextAttr.get(); if (msg instanceof FullHttpResponse) { - tracer().end(context, (HttpResponse) msg); clientContextAttr.remove(); parentContextAttr.remove(); } else if (msg instanceof HttpResponse) { @@ -45,7 +44,6 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) { } else if (msg instanceof LastHttpContent) { // Not a FullHttpResponse so this is content that has been received after headers. Finish the // span using what we stored in attrs. - tracer().end(context, ctx.channel().attr(HTTP_RESPONSE).getAndRemove()); clientContextAttr.remove(); parentContextAttr.remove(); } @@ -58,5 +56,11 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) { } else { ctx.fireChannelRead(msg); } + + if (msg instanceof FullHttpResponse) { + tracer().end(context, (HttpResponse) msg); + } else if (msg instanceof LastHttpContent) { + tracer().end(context, ctx.channel().attr(HTTP_RESPONSE).getAndRemove()); + } } } From 004329b9270c5f8b6fdd9b3e34bef35e19c1b592 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 28 Aug 2021 16:05:40 -0700 Subject: [PATCH 2/3] And netty-4.0 --- .../HttpClientResponseTracingHandler.java | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/HttpClientResponseTracingHandler.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/HttpClientResponseTracingHandler.java index fcd423a7384f..dc8d1c4cc646 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/HttpClientResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/HttpClientResponseTracingHandler.java @@ -5,33 +5,51 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_0.client; +import static io.opentelemetry.javaagent.instrumentation.netty.v4_0.AttributeKeys.attributeKey; import static io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.NettyHttpClientTracer.tracer; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpResponse; +import io.netty.handler.codec.http.LastHttpContent; import io.netty.util.Attribute; +import io.netty.util.AttributeKey; import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.javaagent.instrumentation.netty.v4_0.AttributeKeys; public class HttpClientResponseTracingHandler extends ChannelInboundHandlerAdapter { + private static final AttributeKey HTTP_RESPONSE = + attributeKey(HttpClientResponseTracingHandler.class + ".http-response"); + @Override public void channelRead(ChannelHandlerContext ctx, Object msg) { - Context context = ctx.channel().attr(AttributeKeys.CLIENT_CONTEXT).get(); + Attribute clientContextAttr = ctx.channel().attr(AttributeKeys.CLIENT_CONTEXT); + Context context = clientContextAttr.get(); if (context == null) { ctx.fireChannelRead(msg); return; } - if (msg instanceof HttpResponse) { - tracer().end(context, (HttpResponse) msg); + Attribute parentContextAttr = ctx.channel().attr(AttributeKeys.CLIENT_PARENT_CONTEXT); + Context parentContext = parentContextAttr.get(); + + if (msg instanceof FullHttpResponse) { + clientContextAttr.remove(); + parentContextAttr.remove(); + } else if (msg instanceof HttpResponse) { + // Headers before body have been received, store them to use when finishing the span. + ctx.channel().attr(HTTP_RESPONSE).set((HttpResponse) msg); + } else if (msg instanceof LastHttpContent) { + // Not a FullHttpResponse so this is content that has been received after headers. Finish the + // span using what we stored in attrs. + clientContextAttr.remove(); + parentContextAttr.remove(); } // We want the callback in the scope of the parent, not the client span - Attribute parentAttr = ctx.channel().attr(AttributeKeys.CLIENT_PARENT_CONTEXT); - Context parentContext = parentAttr.get(); if (parentContext != null) { try (Scope ignored = parentContext.makeCurrent()) { ctx.fireChannelRead(msg); @@ -39,5 +57,11 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) { } else { ctx.fireChannelRead(msg); } + + if (msg instanceof FullHttpResponse) { + tracer().end(context, (HttpResponse) msg); + } else if (msg instanceof LastHttpContent) { + tracer().end(context, ctx.channel().attr(HTTP_RESPONSE).getAndRemove()); + } } } From 38b1ba2cc92420aef6f3b861596987ec2b82b5ab Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 28 Aug 2021 20:15:30 -0700 Subject: [PATCH 3/3] ClassValue and more --- .../netty/common/FutureListenerWrappers.java | 19 +++++++++++++++---- .../common/NettyFutureInstrumentation.java | 12 ++++-------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java index c8b903f25670..a6d345cc32c0 100644 --- a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java +++ b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java @@ -26,13 +26,24 @@ public final class FutureListenerWrappers { GenericFutureListener>, GenericFutureListener>> wrappers = Cache.newBuilder().setWeakKeys().setWeakValues().build(); + private static final ClassValue shouldWrap = + new ClassValue() { + @Override + protected Boolean computeValue(Class type) { + // we only want to wrap user callbacks + String className = type.getName(); + return !className.startsWith("io.opentelemetry.javaagent.") + && !className.startsWith("io.netty."); + } + }; + + public static boolean shouldWrap(GenericFutureListener> listener) { + return shouldWrap.get(listener.getClass()); + } + @SuppressWarnings("unchecked") public static GenericFutureListener wrap( Context context, GenericFutureListener> delegate) { - if (delegate instanceof WrappedFutureListener - || delegate instanceof WrappedProgressiveFutureListener) { - return delegate; - } return wrappers.computeIfAbsent( delegate, key -> { diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java index 52c6a21e6e37..add80a5450e2 100644 --- a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java +++ b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/NettyFutureInstrumentation.java @@ -61,13 +61,7 @@ public static class AddListenerAdvice { public static void wrapListener( @Advice.Argument(value = 0, readOnly = false) GenericFutureListener> listener) { - // wrapping our "end" listener leads to strict context leak failures since there will be an - // active scope when we call "end" - // wrapping internal netty listeners also leads to strict context leak failures since some of - // those are called after our "end" listener - String listenerClassName = listener.getClass().getName(); - if (!listenerClassName.startsWith("io.opentelemetry.javaagent.") - && !listenerClassName.startsWith("io.netty.")) { + if (FutureListenerWrappers.shouldWrap(listener)) { listener = FutureListenerWrappers.wrap(Java8BytecodeBridge.currentContext(), listener); } } @@ -86,7 +80,9 @@ public static void wrapListener( GenericFutureListener>[] wrappedListeners = new GenericFutureListener[listeners.length]; for (int i = 0; i < listeners.length; ++i) { - wrappedListeners[i] = FutureListenerWrappers.wrap(context, listeners[i]); + if (FutureListenerWrappers.shouldWrap(listeners[i])) { + wrappedListeners[i] = FutureListenerWrappers.wrap(context, listeners[i]); + } } listeners = wrappedListeners; }