From 9e60bcfca2045ee9045056b2c328cc444c2958ba Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 4 May 2023 13:45:55 +0300 Subject: [PATCH] Fix http pipelining test on Netty 4.1 --- .../finatra/FinatraServerLatestTest.scala | 1 - .../finatra/FinatraServerTest.scala | 1 - .../HttpServerResponseTracingHandler.java | 33 ++++++++++--------- .../netty/v4_1/AbstractNetty41ServerTest.java | 1 - .../server/VertxRxHttpServerTest.groovy | 5 --- .../server/VertxRxHttpServerTest.groovy | 5 --- .../server/VertxLatestHttpServerTest.groovy | 5 --- 7 files changed, 18 insertions(+), 33 deletions(-) diff --git a/instrumentation/finatra-2.9/javaagent/src/latestDepTest/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerLatestTest.scala b/instrumentation/finatra-2.9/javaagent/src/latestDepTest/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerLatestTest.scala index ea9b33d1f610..dcc771532373 100644 --- a/instrumentation/finatra-2.9/javaagent/src/latestDepTest/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerLatestTest.scala +++ b/instrumentation/finatra-2.9/javaagent/src/latestDepTest/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerLatestTest.scala @@ -55,7 +55,6 @@ class FinatraServerLatestTest extends AbstractHttpServerTest[HttpServer] { override def test(endpoint: ServerEndpoint): Boolean = endpoint != ServerEndpoint.NOT_FOUND }) - options.setTestHttpPipelining(false) } override protected def assertHandlerSpan( diff --git a/instrumentation/finatra-2.9/javaagent/src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala b/instrumentation/finatra-2.9/javaagent/src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala index 71cb477ecfec..2679d67b0829 100644 --- a/instrumentation/finatra-2.9/javaagent/src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala +++ b/instrumentation/finatra-2.9/javaagent/src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala @@ -56,7 +56,6 @@ class FinatraServerTest extends AbstractHttpServerTest[HttpServer] { override def test(endpoint: ServerEndpoint): Boolean = endpoint != ServerEndpoint.NOT_FOUND }) - options.setTestHttpPipelining(false) } override protected def assertHandlerSpan( 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 c9a0860434d1..001ebf9df356 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 @@ -7,7 +7,6 @@ import static io.opentelemetry.instrumentation.netty.v4_1.internal.server.HttpServerRequestTracingHandler.HTTP_SERVER_REQUEST; -import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelOutboundHandlerAdapter; @@ -69,18 +68,18 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) thr if (msg instanceof FullHttpResponse) { // Headers and body all sent together, we have the response information in the msg. beforeCommitHandler.handle(context, (HttpResponse) msg); + contextAttr.set(null); + HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null); writePromise.addListener( - future -> end(ctx.channel(), (FullHttpResponse) msg, writePromise)); + future -> end(context, 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). - writePromise.addListener( - future -> - end( - ctx.channel(), - ctx.channel().attr(HTTP_SERVER_RESPONSE).getAndSet(null), - writePromise)); + contextAttr.set(null); + HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null); + HttpResponse response = ctx.channel().attr(HTTP_SERVER_RESPONSE).getAndSet(null); + writePromise.addListener(future -> end(context, request, response, writePromise)); } } else { writePromise = prm; @@ -94,20 +93,24 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) thr try (Scope ignored = context.makeCurrent()) { super.write(ctx, msg, writePromise); } catch (Throwable throwable) { - end(ctx.channel(), null, throwable); + contextAttr.set(null); + HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null); + end(context, request, null, throwable); throw throwable; } } - private void end(Channel channel, HttpResponse response, ChannelFuture future) { + private void end( + Context context, HttpRequestAndChannel request, HttpResponse response, ChannelFuture future) { Throwable error = future.isSuccess() ? null : future.cause(); - end(channel, response, error); + end(context, request, response, error); } - // make sure to remove the server context on end() call - private void end(Channel channel, @Nullable HttpResponse response, @Nullable Throwable error) { - Context context = channel.attr(AttributeKeys.SERVER_CONTEXT).getAndSet(null); - HttpRequestAndChannel request = channel.attr(HTTP_SERVER_REQUEST).getAndSet(null); + private void end( + Context context, + HttpRequestAndChannel request, + @Nullable HttpResponse response, + @Nullable Throwable error) { error = NettyErrorHolder.getOrDefault(context, error); instrumenter.end(context, request, response, error); } diff --git a/instrumentation/netty/netty-4.1/testing/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/AbstractNetty41ServerTest.java b/instrumentation/netty/netty-4.1/testing/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/AbstractNetty41ServerTest.java index 42fafe429c73..30f1012d89c2 100644 --- a/instrumentation/netty/netty-4.1/testing/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/AbstractNetty41ServerTest.java +++ b/instrumentation/netty/netty-4.1/testing/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/AbstractNetty41ServerTest.java @@ -62,7 +62,6 @@ protected void configure(HttpServerTestOptions options) { unused -> Sets.difference( DEFAULT_HTTP_ATTRIBUTES, Collections.singleton(SemanticAttributes.HTTP_ROUTE))); - options.setTestHttpPipelining(false); } @Override diff --git a/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/latestDepTest/groovy/server/VertxRxHttpServerTest.groovy b/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/latestDepTest/groovy/server/VertxRxHttpServerTest.groovy index 3a62c403be25..d732601f5b7c 100644 --- a/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/latestDepTest/groovy/server/VertxRxHttpServerTest.groovy +++ b/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/latestDepTest/groovy/server/VertxRxHttpServerTest.groovy @@ -68,11 +68,6 @@ class VertxRxHttpServerTest extends HttpServerTest implements AgentTestTr return false } - @Override - boolean testHttpPipelining() { - false - } - protected Class verticle() { return VertxReactiveWebServer } diff --git a/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/version35Test/groovy/server/VertxRxHttpServerTest.groovy b/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/version35Test/groovy/server/VertxRxHttpServerTest.groovy index 5852e5575c2d..d74f7ab9e48b 100644 --- a/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/version35Test/groovy/server/VertxRxHttpServerTest.groovy +++ b/instrumentation/vertx/vertx-rx-java-3.5/javaagent/src/version35Test/groovy/server/VertxRxHttpServerTest.groovy @@ -62,11 +62,6 @@ class VertxRxHttpServerTest extends HttpServerTest implements AgentTestTr return true } - @Override - boolean testHttpPipelining() { - false - } - @Override boolean verifyServerSpanEndTime() { // server spans are ended inside of the controller spans diff --git a/instrumentation/vertx/vertx-web-3.0/javaagent/src/latestDepTest/groovy/server/VertxLatestHttpServerTest.groovy b/instrumentation/vertx/vertx-web-3.0/javaagent/src/latestDepTest/groovy/server/VertxLatestHttpServerTest.groovy index 1d48830bd8d7..7a679da334b1 100644 --- a/instrumentation/vertx/vertx-web-3.0/javaagent/src/latestDepTest/groovy/server/VertxLatestHttpServerTest.groovy +++ b/instrumentation/vertx/vertx-web-3.0/javaagent/src/latestDepTest/groovy/server/VertxLatestHttpServerTest.groovy @@ -13,9 +13,4 @@ class VertxLatestHttpServerTest extends AbstractVertxHttpServerTest { protected Class verticle() { return VertxLatestWebServer } - - @Override - boolean testHttpPipelining() { - false - } }