From a05d2f52c7114603487b02f8db2fe13d6a416b22 Mon Sep 17 00:00:00 2001 From: Shashank Patidar Date: Tue, 13 Jun 2023 16:27:10 +0530 Subject: [PATCH 1/3] fix netty connection keep alive context --- .../HttpServerResponseTracingHandler.java | 2 + ...tractNetty41ServerInstrumentationTest.java | 101 ++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java index f5ddfe8b..78d980da 100644 --- a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java @@ -103,6 +103,8 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) { span.setStatus(code >= 100 && code < 500 ? StatusCode.UNSET : StatusCode.ERROR); } if (msg instanceof LastHttpContent) { + ctx.channel().attr(io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys.SERVER_CONTEXT).set(null); + ctx.channel().attr(AttributeKeys.REQUEST).set(null); span.end(); } } diff --git a/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java b/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java index da205c06..e0d7de71 100644 --- a/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java +++ b/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java @@ -206,4 +206,105 @@ public void blocking() throws IOException, TimeoutException, InterruptedExceptio .getAttributes() .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_BODY))); } + + @Test + public void connectionKeepAlive() throws IOException, TimeoutException, InterruptedException { + Request request = + new Request.Builder() + .url(String.format("http://localhost:%d/post", port)) + .header(REQUEST_HEADER_NAME, REQUEST_HEADER_VALUE) + .header("first", "1st") + .header("connection", "keep-alive") + .get() + .build(); + + try (Response response = httpClient.newCall(request).execute()) { + Assertions.assertEquals(200, response.code()); + Assertions.assertEquals(RESPONSE_BODY, response.body().string()); + } + + List> traces = TEST_WRITER.getTraces(); + TEST_WRITER.waitForTraces(1); + Assertions.assertEquals(1, traces.size()); + List trace = traces.get(0); + Assertions.assertEquals(1, trace.size()); + SpanData spanData = trace.get(0); + + Assertions.assertEquals( + REQUEST_HEADER_VALUE, + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); + Assertions.assertEquals( + "1st", + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("first"))); + Assertions.assertEquals( + "keep-alive", + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); + Assertions.assertEquals( + RESPONSE_HEADER_VALUE, + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); + Assertions.assertNull( + spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY)); + Assertions.assertEquals( + RESPONSE_BODY, + spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY)); + + + RequestBody requestBody = blockedRequestBody(true, 3000, 75); + Request request2 = + new Request.Builder() + .url(String.format("http://localhost:%d/post", port)) + .header(REQUEST_HEADER_NAME, "REQUEST_HEADER_VALUE") + .header("second", "2nd") + .header("connection", "keep-alive") + .post(requestBody) + .build(); + + try (Response response = httpClient.newCall(request2).execute()) { + Assertions.assertEquals(403, response.code()); + Assertions.assertTrue(response.body().string().isEmpty()); + } + + List> traces2 = TEST_WRITER.getTraces(); + TEST_WRITER.waitForTraces(2); + Assertions.assertEquals(2, traces2.size()); + List trace2 = traces2.get(1); + Assertions.assertEquals(1, trace2.size()); + SpanData spanData2 = trace2.get(0); + + Assertions.assertEquals( + "REQUEST_HEADER_VALUE", + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); + Assertions.assertEquals( + "2nd", + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("second"))); + Assertions.assertEquals( + "keep-alive", + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); + Assertions.assertNull( + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("first"))); + Assertions.assertNull( + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); + Assertions.assertNull( + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_BODY))); + } } From c2f6ae21fa8ab8fef9dfa197b7a0b25496656b55 Mon Sep 17 00:00:00 2001 From: Shashank Patidar Date: Tue, 13 Jun 2023 16:38:01 +0530 Subject: [PATCH 2/3] format --- .../HttpServerResponseTracingHandler.java | 4 +- ...tractNetty41ServerInstrumentationTest.java | 101 ++++++++---------- 2 files changed, 49 insertions(+), 56 deletions(-) diff --git a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java index 78d980da..394ec638 100644 --- a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java @@ -103,7 +103,9 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) { span.setStatus(code >= 100 && code < 500 ? StatusCode.UNSET : StatusCode.ERROR); } if (msg instanceof LastHttpContent) { - ctx.channel().attr(io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys.SERVER_CONTEXT).set(null); + ctx.channel() + .attr(io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys.SERVER_CONTEXT) + .set(null); ctx.channel().attr(AttributeKeys.REQUEST).set(null); span.end(); } diff --git a/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java b/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java index e0d7de71..4bd0a1fa 100644 --- a/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java +++ b/instrumentation/netty/netty-4.1/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/AbstractNetty41ServerInstrumentationTest.java @@ -210,13 +210,13 @@ public void blocking() throws IOException, TimeoutException, InterruptedExceptio @Test public void connectionKeepAlive() throws IOException, TimeoutException, InterruptedException { Request request = - new Request.Builder() - .url(String.format("http://localhost:%d/post", port)) - .header(REQUEST_HEADER_NAME, REQUEST_HEADER_VALUE) - .header("first", "1st") - .header("connection", "keep-alive") - .get() - .build(); + new Request.Builder() + .url(String.format("http://localhost:%d/post", port)) + .header(REQUEST_HEADER_NAME, REQUEST_HEADER_VALUE) + .header("first", "1st") + .header("connection", "keep-alive") + .get() + .build(); try (Response response = httpClient.newCall(request).execute()) { Assertions.assertEquals(200, response.code()); @@ -231,41 +231,36 @@ public void connectionKeepAlive() throws IOException, TimeoutException, Interrup SpanData spanData = trace.get(0); Assertions.assertEquals( - REQUEST_HEADER_VALUE, - spanData - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); + REQUEST_HEADER_VALUE, + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); Assertions.assertEquals( - "1st", - spanData - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader("first"))); + "1st", + spanData.getAttributes().get(HypertraceSemanticAttributes.httpRequestHeader("first"))); Assertions.assertEquals( - "keep-alive", - spanData - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); + "keep-alive", + spanData.getAttributes().get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); Assertions.assertEquals( - RESPONSE_HEADER_VALUE, - spanData - .getAttributes() - .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); + RESPONSE_HEADER_VALUE, + spanData + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); Assertions.assertNull( - spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY)); + spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY)); Assertions.assertEquals( - RESPONSE_BODY, - spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY)); - + RESPONSE_BODY, + spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY)); RequestBody requestBody = blockedRequestBody(true, 3000, 75); Request request2 = - new Request.Builder() - .url(String.format("http://localhost:%d/post", port)) - .header(REQUEST_HEADER_NAME, "REQUEST_HEADER_VALUE") - .header("second", "2nd") - .header("connection", "keep-alive") - .post(requestBody) - .build(); + new Request.Builder() + .url(String.format("http://localhost:%d/post", port)) + .header(REQUEST_HEADER_NAME, "REQUEST_HEADER_VALUE") + .header("second", "2nd") + .header("connection", "keep-alive") + .post(requestBody) + .build(); try (Response response = httpClient.newCall(request2).execute()) { Assertions.assertEquals(403, response.code()); @@ -280,31 +275,27 @@ public void connectionKeepAlive() throws IOException, TimeoutException, Interrup SpanData spanData2 = trace2.get(0); Assertions.assertEquals( - "REQUEST_HEADER_VALUE", - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); + "REQUEST_HEADER_VALUE", + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER_NAME))); Assertions.assertEquals( - "2nd", - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader("second"))); + "2nd", + spanData2.getAttributes().get(HypertraceSemanticAttributes.httpRequestHeader("second"))); Assertions.assertEquals( - "keep-alive", - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); + "keep-alive", + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpRequestHeader("connection"))); Assertions.assertNull( - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpRequestHeader("first"))); + spanData2.getAttributes().get(HypertraceSemanticAttributes.httpRequestHeader("first"))); Assertions.assertNull( - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_HEADER_NAME))); Assertions.assertNull( - spanData2 - .getAttributes() - .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_BODY))); + spanData2 + .getAttributes() + .get(HypertraceSemanticAttributes.httpResponseHeader(RESPONSE_BODY))); } } From 748274b9136fe0bef8ff844e0d6fdd5a60a07fe8 Mon Sep 17 00:00:00 2001 From: Shashank Patidar Date: Thu, 15 Jun 2023 16:46:58 +0530 Subject: [PATCH 3/3] added comment --- .../netty/v4_1/server/HttpServerResponseTracingHandler.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java index 394ec638..f2831aa8 100644 --- a/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.1/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/netty/v4_1/server/HttpServerResponseTracingHandler.java @@ -103,6 +103,10 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) { span.setStatus(code >= 100 && code < 500 ? StatusCode.UNSET : StatusCode.ERROR); } if (msg instanceof LastHttpContent) { + // When we end the span, we should set the server context and request attr to null so that + // for the next request a new context and request is made and stored in channel. + // Else, when using a Connection: keep-alive header, the same server context and request attr + // is used in the subsequent requests. ctx.channel() .attr(io.opentelemetry.instrumentation.netty.v4_1.internal.AttributeKeys.SERVER_CONTEXT) .set(null);