From 9386e201b6529f0ae1e2e20ce746050b521b9ddd Mon Sep 17 00:00:00 2001 From: mateuszrzeszutek Date: Thu, 3 Sep 2020 16:06:55 +0200 Subject: [PATCH] Fix Servlet instrumentation for servlet-api 3.1+ - remove output stream wrapper --- .../src/test/groovy/DropwizardTest.groovy | 2 +- .../test/groovy/JaxRsHttpServerTest.groovy | 11 ++-- .../jetty/JettyHandlerInstrumentation.java | 2 - .../test/groovy/GlassFishServerTest.groovy | 2 +- .../v3_0/AsyncContextInstrumentation.java | 5 -- .../v3_0/CountingHttpServletResponse.java | 51 ------------------- .../servlet/v3_0/Servlet3Instrumentation.java | 1 - .../test/boot/SpringBootBasedTest.groovy | 2 +- 8 files changed, 6 insertions(+), 70 deletions(-) diff --git a/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy b/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy index 73af57fd9e5d..f487a8b3172e 100644 --- a/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy +++ b/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy @@ -122,7 +122,7 @@ class DropwizardTest extends HttpServerTest { "${SemanticAttributes.HTTP_USER_AGENT.key()}" TEST_USER_AGENT "${SemanticAttributes.HTTP_CLIENT_IP.key()}" TEST_CLIENT_IP // exception bodies are not yet recorded - "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == responseContentLength || /* async */it == null } + "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == 0 || /* async */it == null } "servlet.context" "" "servlet.path" "" if (endpoint.query) { diff --git a/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsHttpServerTest.groovy b/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsHttpServerTest.groovy index 74d650e2fcbc..68d4eba01ea9 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsHttpServerTest.groovy +++ b/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsHttpServerTest.groovy @@ -88,7 +88,7 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { String method = "GET", Long responseContentLength = null, ServerEndpoint endpoint = SUCCESS) { - serverSpan(trace, index, traceID, parentID, method, responseContentLength, + serverSpan(trace, index, traceID, parentID, method, endpoint == PATH_PARAM ? "/path/{id}/param" : endpoint.resolvePath(address).path, endpoint.resolve(address), endpoint.errored, @@ -101,7 +101,7 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { HttpUrl url, int statusCode) { def rawUrl = url.url() - serverSpan(trace, index, null, null, "GET", null, + serverSpan(trace, index, null, null, "GET", rawUrl.path, rawUrl.toURI(), statusCode >= 500, @@ -114,7 +114,6 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { String traceID, String parentID, String method, - Long responseContentLength, String path, URI fullUrl, boolean isError, @@ -139,11 +138,7 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { "${SemanticAttributes.HTTP_FLAVOR.key()}" "HTTP/1.1" "${SemanticAttributes.HTTP_USER_AGENT.key()}" TEST_USER_AGENT "${SemanticAttributes.HTTP_CLIENT_IP.key()}" TEST_CLIENT_IP - if (responseContentLength) { - "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" responseContentLength - } else { - "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long - } + "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long "servlet.path" String "servlet.context" String if (query) { diff --git a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerInstrumentation.java b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerInstrumentation.java index b04e304465b4..92c0c1d95536 100644 --- a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerInstrumentation.java +++ b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerInstrumentation.java @@ -59,9 +59,7 @@ public String[] helperClassNames() { "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer", "io.opentelemetry.instrumentation.auto.servlet.v3_0.Servlet3HttpServerTracer", "io.opentelemetry.instrumentation.auto.servlet.v3_0.TagSettingAsyncListener", - "io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletRequest", "io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse", - "io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse$CountingServletOutputStream", "io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse$CountingPrintWriter", packageName + ".JettyHttpServerTracer", }; diff --git a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy index c0afcb1d1334..6ee78286101b 100644 --- a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy +++ b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy @@ -110,7 +110,7 @@ class GlassFishServerTest extends HttpServerTest { "${SemanticAttributes.HTTP_CLIENT_IP.key()}" TEST_CLIENT_IP // exception bodies are not yet recorded // TODO(anuraaga): Bodies do seem to be recorded for these endpoints here, update to make assertion more precise. - "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == responseContentLength || endpoint == EXCEPTION || endpoint == ERROR || endpoint == NOT_FOUND || endpoint == REDIRECT } + "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == 0 || it == responseContentLength || endpoint == EXCEPTION || endpoint == ERROR || endpoint == NOT_FOUND || endpoint == REDIRECT } "servlet.context" "/$context" "servlet.path" endpoint.path if (endpoint.query) { diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/AsyncContextInstrumentation.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/AsyncContextInstrumentation.java index e95e2cbd5dc3..919042c33078 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/AsyncContextInstrumentation.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/AsyncContextInstrumentation.java @@ -50,11 +50,6 @@ public String[] helperClassNames() { return new String[] { "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter", "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer", - packageName + ".CountingHttpServletResponse", - packageName + ".CountingHttpServletResponse$CountingServletOutputStream", - packageName + ".CountingHttpServletResponse$CountingPrintWriter", - packageName + ".TagSettingAsyncListener", - packageName + ".Servlet3HttpServerTracer" }; } diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/CountingHttpServletResponse.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/CountingHttpServletResponse.java index 90b8e8b27ea2..e2d9c6a7681a 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/CountingHttpServletResponse.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/CountingHttpServletResponse.java @@ -19,13 +19,11 @@ import java.io.IOException; import java.io.PrintWriter; import java.io.Writer; -import javax.servlet.ServletOutputStream; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; /** HttpServletResponseWrapper since servlet 2.3, not applicable to 2.2 */ public class CountingHttpServletResponse extends HttpServletResponseWrapper { - private CountingServletOutputStream outputStream = null; private CountingPrintWriter printWriter = null; private int errorLength = 0; @@ -38,14 +36,6 @@ public CountingHttpServletResponse(HttpServletResponse response) { super(response); } - @Override - public ServletOutputStream getOutputStream() throws IOException { - if (outputStream == null) { - outputStream = new CountingServletOutputStream(super.getOutputStream()); - } - return outputStream; - } - @Override public PrintWriter getWriter() throws IOException { if (printWriter == null) { @@ -56,9 +46,6 @@ public PrintWriter getWriter() throws IOException { public int getContentLength() { int contentLength = errorLength; - if (outputStream != null) { - contentLength += outputStream.counter; - } if (printWriter != null) { contentLength += printWriter.counter; } @@ -74,44 +61,6 @@ public void sendError(int sc, String msg) throws IOException { } } - static class CountingServletOutputStream extends ServletOutputStream { - - private final ServletOutputStream delegate; - private int counter = 0; - - public CountingServletOutputStream(ServletOutputStream delegate) { - this.delegate = delegate; - } - - @Override - public void write(int b) throws IOException { - delegate.write(b); - counter++; - } - - @Override - public void write(byte[] b) throws IOException { - delegate.write(b); - counter += b.length; - } - - @Override - public void write(byte[] b, int off, int len) throws IOException { - delegate.write(b, off, len); - counter += len; - } - - @Override - public void flush() throws IOException { - delegate.flush(); - } - - @Override - public void close() throws IOException { - delegate.close(); - } - } - static class CountingPrintWriter extends PrintWriter { private int counter = 0; diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/Servlet3Instrumentation.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/Servlet3Instrumentation.java index 067609c9088f..310cda586874 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/Servlet3Instrumentation.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/auto/servlet/v3_0/Servlet3Instrumentation.java @@ -56,7 +56,6 @@ public String[] helperClassNames() { "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer", packageName + ".CountingHttpServletRequest", packageName + ".CountingHttpServletResponse", - packageName + ".CountingHttpServletResponse$CountingServletOutputStream", packageName + ".CountingHttpServletResponse$CountingPrintWriter", packageName + ".Servlet3Advice", packageName + ".Servlet3HttpServerTracer", diff --git a/instrumentation/spring/spring-webmvc-3.1/spring-webmvc-3.1-auto/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/spring-webmvc-3.1-auto/src/test/groovy/test/boot/SpringBootBasedTest.groovy index bd56144f1321..1d4486d725e0 100644 --- a/instrumentation/spring/spring-webmvc-3.1/spring-webmvc-3.1-auto/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/spring-webmvc-3.1-auto/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -163,7 +163,7 @@ class SpringBootBasedTest extends HttpServerTest "${SemanticAttributes.HTTP_USER_AGENT.key()}" TEST_USER_AGENT "${SemanticAttributes.HTTP_CLIENT_IP.key()}" TEST_CLIENT_IP // exception bodies are not yet recorded - "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" responseContentLength + "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" 0 "servlet.path" endpoint.path "servlet.context" "" if (endpoint.query) {