Don't wrap request/response in Jetty (#1156)

* Don't wrap request/response in Jetty * Polish * Add test
open-telemetry · Sep 3, 2020 · ec9c20b · ec9c20b
1 parent 6f783bc
commit ec9c20b
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 20 deletions.
diff --git a/...tty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerAdvice.java b/...tty-8.0/src/main/java/io/opentelemetry/instrumentation/auto/jetty/JettyHandlerAdvice.java
@@ -20,8 +20,6 @@
 
 import io.grpc.Context;
 import io.opentelemetry.context.Scope;
-import io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletRequest;
-import io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse;
 import io.opentelemetry.instrumentation.auto.servlet.v3_0.TagSettingAsyncListener;
 import io.opentelemetry.trace.Span;
 import java.lang.reflect.Method;
@@ -37,7 +35,6 @@ public static void onEnter(
       @Advice.Origin Method method,
       @Advice.This Object source,
       @Advice.Argument(value = 2, readOnly = false) HttpServletRequest request,
-      @Advice.Argument(value = 3, readOnly = false) HttpServletResponse response,
       @Advice.Local("otelSpan") Span span,
       @Advice.Local("otelScope") Scope scope) {
 
@@ -49,11 +46,6 @@ public static void onEnter(
 
     span = TRACER.startSpan(request, request, method);
     scope = TRACER.startScope(span, request);
-
-    if (!(response instanceof CountingHttpServletResponse)) {
-      response = new CountingHttpServletResponse(response);
-      request = new CountingHttpServletRequest(request, response);
-    }
   }
 
   @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)

diff --git a/instrumentation/jetty-8.0/src/test/groovy/JettyHandlerTest.groovy b/instrumentation/jetty-8.0/src/test/groovy/JettyHandlerTest.groovy
@@ -14,14 +14,6 @@
  * limitations under the License.
  */
 
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS
-import static io.opentelemetry.trace.Span.Kind.SERVER
-
 import io.opentelemetry.auto.test.asserts.TraceAssert
 import io.opentelemetry.auto.test.base.HttpServerTest
 import io.opentelemetry.instrumentation.api.MoreAttributes
@@ -31,11 +23,20 @@ import javax.servlet.ServletException
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
 import org.eclipse.jetty.server.Request
+import org.eclipse.jetty.server.Response
 import org.eclipse.jetty.server.Server
 import org.eclipse.jetty.server.handler.AbstractHandler
 import org.eclipse.jetty.server.handler.ErrorHandler
 import spock.lang.Shared
 
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+import static io.opentelemetry.trace.Span.Kind.SERVER
+
 class JettyHandlerTest extends HttpServerTest<Server> {
 
   @Shared
@@ -108,8 +109,11 @@ class JettyHandlerTest extends HttpServerTest<Server> {
   static class TestHandler extends AbstractHandler {
     @Override
     void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+      //This line here is to verify that we don't break Jetty if it wants to cast to implementation class
+      //See https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/1096
+      Response jettyResponse = response as Response
       if (baseRequest.dispatcherType != DispatcherType.ERROR) {
-        handleRequest(baseRequest, response)
+        handleRequest(baseRequest, jettyResponse)
         baseRequest.handled = true
       } else {
         errorHandler.handle(target, baseRequest, response, response)
@@ -141,8 +145,6 @@ class JettyHandlerTest extends HttpServerTest<Server> {
         "${SemanticAttributes.HTTP_FLAVOR.key()}" "HTTP/1.1"
         "${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 || endpoint == EXCEPTION }
         "servlet.path" ''
         if (endpoint.query) {
           "$MoreAttributes.HTTP_QUERY" endpoint.query

diff --git a/instrumentation/spark-web-framework-2.3/src/test/groovy/SparkJavaBasedTest.groovy b/instrumentation/spark-web-framework-2.3/src/test/groovy/SparkJavaBasedTest.groovy
@@ -65,7 +65,6 @@ class SparkJavaBasedTest extends AgentTestRunner {
             "${SemanticAttributes.NET_PEER_IP.key()}" "127.0.0.1"
             "${SemanticAttributes.NET_PEER_PORT.key()}" Long
             "${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/param/asdf1234"
-            "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" content.length()
             "${SemanticAttributes.HTTP_METHOD.key()}" "GET"
             "${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key()}" "HTTP/1.1"