Jetty 8: ignore parent and move to HttpServerTest

This method of using jetty doesn’t seem to work with Servlet’s Async. Native Jetty uses Continuations which we don’t support and should investigate instrumenting.
2019-08-07 08:32:32 -07:00 · 2019-08-07 08:32:32 -07:00 · 6dd729b843
parent 49249c0c6e
commit 6dd729b843
6 changed files with 211 additions and 178 deletions
--- a/dd-java-agent/instrumentation/jetty-8/jetty-8.gradle
+++ b/dd-java-agent/instrumentation/jetty-8/jetty-8.gradle
@ -24,6 +24,8 @@ dependencies {
  testCompile(project(':dd-java-agent:testing')) {
    exclude group: 'org.eclipse.jetty', module: 'jetty-server'
  }
+  testCompile project(':dd-java-agent:instrumentation:java-concurrent')
+
  testCompile group: 'org.eclipse.jetty', name: 'jetty-server', version: '8.0.0.v20110901'
  testCompile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '8.0.0.v20110901'
  testCompile group: 'org.eclipse.jetty', name: 'jetty-continuation', version: '8.0.0.v20110901'
--- a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerAdvice.java
+++ b/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerAdvice.java
@ -16,13 +16,14 @@ import javax.servlet.http.HttpServletResponse;
 import net.bytebuddy.asm.Advice;

 public class JettyHandlerAdvice {
+  public static final String SERVLET_SPAN = "datadog.servlet.span";

  @Advice.OnMethodEnter(suppress = Throwable.class)
  public static Scope startSpan(
      @Advice.This final Object source, @Advice.Argument(2) final HttpServletRequest req) {

-    if (GlobalTracer.get().activeSpan() != null) {
-      // Tracing might already be applied.  If so ignore this.
+    if (req.getAttribute(SERVLET_SPAN) != null) {
+      // Request already being traced elsewhere.
      return null;
    }

@ -32,6 +33,7 @@ public class JettyHandlerAdvice {
    final Scope scope =
        GlobalTracer.get()
            .buildSpan("jetty.request")
+            .ignoreActiveSpan()
            .asChildOf(extractedContext)
            .withTag("span.origin.type", source.getClass().getName())
            .startActive(false);
@ -46,6 +48,7 @@ public class JettyHandlerAdvice {
    if (scope instanceof TraceScope) {
      ((TraceScope) scope).setAsyncPropagation(true);
    }
+    req.setAttribute(SERVLET_SPAN, span);
    return scope;
  }

--- a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerInstrumentation.java
+++ b/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerInstrumentation.java
@ -16,9 +16,9 @@ import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;

@AutoService(Instrumenter.class)
-public final class HandlerInstrumentation extends Instrumenter.Default {
+public final class JettyHandlerInstrumentation extends Instrumenter.Default {

-  public HandlerInstrumentation() {
+  public JettyHandlerInstrumentation() {
    super("jetty", "jetty-8");
  }

--- a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyContinuationHandlerTest.groovy
+++ b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyContinuationHandlerTest.groovy
@ -0,0 +1,62 @@
+import javax.servlet.ServletException
+import javax.servlet.http.HttpServletRequest
+import javax.servlet.http.HttpServletResponse
+import org.eclipse.jetty.continuation.Continuation
+import org.eclipse.jetty.continuation.ContinuationSupport
+import org.eclipse.jetty.server.Request
+import org.eclipse.jetty.server.handler.AbstractHandler
+
+import java.util.concurrent.ExecutorService
+import java.util.concurrent.Executors
+
+// FIXME: We don't currently handle jetty continuations properly (at all).
+abstract class JettyContinuationHandlerTest extends JettyHandlerTest {
+
+  @Override
+  AbstractHandler handler() {
+    ContinuationTestHandler.INSTANCE
+  }
+
+  static class ContinuationTestHandler extends AbstractHandler {
+    static final ContinuationTestHandler INSTANCE = new ContinuationTestHandler()
+    final ExecutorService executorService = Executors.newSingleThreadExecutor()
+
+    @Override
+    void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+      final Continuation continuation = ContinuationSupport.getContinuation(request)
+      if (continuation.initial) {
+        continuation.suspend()
+        executorService.execute {
+          continuation.resume()
+        }
+      } else {
+        handleRequest(baseRequest, response)
+      }
+      baseRequest.handled = true
+    }
+  }
+
+//  // This server seems to generate a TEST_SPAN twice... once for the initial request, and once for the continuation.
+//  void cleanAndAssertTraces(
+//    final int size,
+//    @ClosureParams(value = SimpleType, options = "datadog.trace.agent.test.asserts.ListWriterAssert")
+//    @DelegatesTo(value = ListWriterAssert, strategy = Closure.DELEGATE_FIRST)
+//    final Closure spec) {
+//
+//    // If this is failing, make sure HttpServerTestAdvice is applied correctly.
+//    TEST_WRITER.waitForTraces(size * 3)
+//    // TEST_WRITER is a CopyOnWriteArrayList, which doesn't support remove()
+//    def toRemove = TEST_WRITER.findAll {
+//      it.size() == 1 && it.get(0).operationName == "TEST_SPAN"
+//    }
+//    toRemove.each {
+//      assertTrace(it, 1) {
+//        basicSpan(it, 0, "TEST_SPAN", "ServerEntry")
+//      }
+//    }
+//    assert toRemove.size() == size * 2
+//    TEST_WRITER.removeAll(toRemove)
+//
+//    assertTraces(size, spec)
+//  }
+}
--- a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy
+++ b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy
@ -1,204 +1,137 @@
-import datadog.trace.agent.test.AgentTestRunner
-import datadog.trace.agent.test.utils.OkHttpUtils
-import datadog.trace.agent.test.utils.PortUtils
+import datadog.trace.agent.test.asserts.TraceAssert
+import datadog.trace.agent.test.base.HttpServerTest
 import datadog.trace.api.DDSpanTypes
-import okhttp3.OkHttpClient
-import org.eclipse.jetty.continuation.Continuation
-import org.eclipse.jetty.continuation.ContinuationSupport
-import org.eclipse.jetty.server.Handler
-import org.eclipse.jetty.server.Request
-import org.eclipse.jetty.server.Server
-import org.eclipse.jetty.server.handler.AbstractHandler
-
-import javax.servlet.DispatcherType
+import datadog.trace.instrumentation.jetty8.JettyDecorator
+import io.opentracing.tag.Tags
 import javax.servlet.ServletException
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
-import java.util.concurrent.atomic.AtomicBoolean
+import org.eclipse.jetty.server.Request
+import org.eclipse.jetty.server.Server
+import org.eclipse.jetty.server.handler.AbstractHandler
+import org.eclipse.jetty.server.handler.ErrorHandler

-class JettyHandlerTest extends AgentTestRunner {
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+
+class JettyHandlerTest extends HttpServerTest<Server, JettyDecorator> {

  static {
    System.setProperty("dd.integration.jetty.enabled", "true")
  }

-  int port = PortUtils.randomOpenPort()
-  Server server = new Server(port)
+  @Override
+  Server startServer(int port) {
+    def server = new Server(port)
+    server.setHandler(handler())
+    server.addBean(new ErrorHandler() {
+      @Override
+      protected void handleErrorPage(HttpServletRequest request, Writer writer, int code, String message) throws IOException {
+        Throwable th = (Throwable) request.getAttribute("javax.servlet.error.exception")
+        message = th ? th.message : message
+        if (message) {
+          writer.write(message)
+        }
+      }
+    })
+    server.start()
+    return server
+  }

-  OkHttpClient client = OkHttpUtils.client()
+  AbstractHandler handler() {
+    TestHandler.INSTANCE
+  }

-  def cleanup() {
+  @Override
+  void stopServer(Server server) {
    server.stop()
  }

-  def "call to jetty creates a trace"() {
-    setup:
-    Handler handler = new AbstractHandler() {
-      @Override
-      void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
-        response.setContentType("text/plain;charset=utf-8")
-        response.setStatus(HttpServletResponse.SC_OK)
-        baseRequest.setHandled(true)
-        response.getWriter().println("Hello World")
-      }
-    }
-    server.setHandler(handler)
-    server.start()
-    def request = new okhttp3.Request.Builder()
-      .url("http://localhost:$port/")
-      .get()
-      .build()
-    def response = client.newCall(request).execute()
+  @Override
+  JettyDecorator decorator() {
+    return JettyDecorator.DECORATE
+  }

-    expect:
-    response.body().string().trim() == "Hello World"
+  @Override
+  String expectedOperationName() {
+    return "jetty.request"
+  }

-    assertTraces(1) {
-      trace(0, 1) {
-        span(0) {
-          serviceName "unnamed-java-app"
-          operationName "jetty.request"
-          resourceName "GET ${handler.class.name}"
-          spanType DDSpanTypes.HTTP_SERVER
-          errored false
-          parent()
-          tags {
-            "http.url" "http://localhost:$port/"
-            "http.method" "GET"
-            "span.kind" "server"
-            "component" "jetty-handler"
-            "span.origin.type" handler.class.name
-            "http.status_code" 200
-            "peer.hostname" "127.0.0.1"
-            "peer.ipv4" "127.0.0.1"
-            "peer.port" Integer
-            defaultTags()
-          }
-        }
+  @Override
+  boolean testExceptionBody() {
+    false
+  }
+
+  static void handleRequest(Request request, HttpServletResponse response) {
+    ServerEndpoint endpoint = ServerEndpoint.forPath(request.requestURI)
+    controller(endpoint) {
+      response.contentType = "text/plain"
+      switch (endpoint) {
+        case SUCCESS:
+          response.status = endpoint.status
+          response.writer.print(endpoint.body)
+          break
+        case REDIRECT:
+          response.sendRedirect(endpoint.body)
+          break
+        case ERROR:
+          response.sendError(endpoint.status, endpoint.body)
+          break
+        case EXCEPTION:
+          throw new Exception(endpoint.body)
+        default:
+          response.status = NOT_FOUND.status
+          response.writer.print(NOT_FOUND.body)
+          break
      }
    }
  }

-  def "handler instrumentation clears state after async request"() {
-    setup:
-    Handler handler = new AbstractHandler() {
-      @Override
-      void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
-        final Continuation continuation = ContinuationSupport.getContinuation(request)
-        continuation.suspend(response)
-        // By the way, this is a terrible async server
-        new Thread() {
-          @Override
-          void run() {
-            continuation.getServletResponse().setContentType("text/plain;charset=utf-8")
-            continuation.getServletResponse().getWriter().println("Hello World")
-            continuation.complete()
-          }
-        }.start()
+  static class TestHandler extends AbstractHandler {
+    static final TestHandler INSTANCE = new TestHandler()

-        baseRequest.setHandled(true)
-      }
-    }
-    server.setHandler(handler)
-    server.start()
-    def request = new okhttp3.Request.Builder()
-      .url("http://localhost:$port/")
-      .get()
-      .build()
-    def numTraces = 10
-    for (int i = 0; i < numTraces; ++i) {
-      assert client.newCall(request).execute().body().string().trim() == "Hello World"
-    }
-
-    expect:
-    assertTraces(numTraces) {
-      for (int i = 0; i < numTraces; ++i) {
-        trace(i, 1) {
-          span(0) {
-            serviceName "unnamed-java-app"
-            operationName "jetty.request"
-            resourceName "GET ${handler.class.name}"
-            spanType DDSpanTypes.HTTP_SERVER
-          }
-        }
-      }
+    @Override
+    void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+      handleRequest(baseRequest, response)
+      baseRequest.handled = true
    }
  }

-  def "call to jetty with error creates a trace"() {
-    setup:
-    def errorHandlerCalled = new AtomicBoolean(false)
-    Handler handler = new AbstractHandler() {
-      @Override
-      void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
-        if (baseRequest.dispatcherType == DispatcherType.ERROR) {
-          errorHandlerCalled.set(true)
-          baseRequest.setHandled(true)
-        } else {
-          throw new RuntimeException()
-        }
+  @Override
+  void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
+    def handlerName = handler().class.name
+    trace.span(index) {
+      serviceName expectedServiceName()
+      operationName expectedOperationName()
+      resourceName endpoint.status == 404 ? "404" : "$method $handlerName"
+      spanType DDSpanTypes.HTTP_SERVER
+      errored endpoint.errored
+      if (parentID != null) {
+        traceId traceID
+        parentId parentID
+      } else {
+        parent()
      }
-    }
-    server.setHandler(handler)
-    server.start()
-    def request = new okhttp3.Request.Builder()
-      .url("http://localhost:$port/")
-      .get()
-      .build()
-    def response = client.newCall(request).execute()
-
-    expect:
-    response.body().string().trim() == ""
-
-    assertTraces(errorHandlerCalled.get() ? 2 : 1) {
-      trace(0, 1) {
-        span(0) {
-          serviceName "unnamed-java-app"
-          operationName "jetty.request"
-          resourceName "GET ${handler.class.name}"
-          spanType DDSpanTypes.HTTP_SERVER
-          errored true
-          parent()
-          tags {
-            "http.url" "http://localhost:$port/"
-            "http.method" "GET"
-            "span.kind" "server"
-            "component" "jetty-handler"
-            "span.origin.type" handler.class.name
-            "http.status_code" 500
-            "peer.hostname" "127.0.0.1"
-            "peer.ipv4" "127.0.0.1"
-            "peer.port" Integer
-            errorTags RuntimeException
-            defaultTags()
-          }
-        }
-      }
-      if (errorHandlerCalled.get()) {
-        // FIXME: This doesn't ever seem to be called.
-        trace(1, 1) {
-          span(0) {
-            serviceName "unnamed-java-app"
-            operationName "jetty.request"
-            resourceName "GET ${handler.class.name}"
-            spanType DDSpanTypes.HTTP_SERVER
-            errored true
-            parent()
-            tags {
-              "http.url" "http://localhost:$port/"
-              "http.method" "GET"
-              "span.kind" "server"
-              "component" "jetty-handler"
-              "span.origin.type" handler.class.name
-              "http.status_code" 500
-              "peer.hostname" "127.0.0.1"
-              "peer.ipv4" "127.0.0.1"
-              "peer.port" Integer
-              "error" true
-              defaultTags()
-            }
-          }
+      tags {
+        "span.origin.type" handlerName
+        defaultTags(true)
+        "$Tags.COMPONENT.key" serverDecorator.component()
+        if (endpoint.errored) {
+          "$Tags.ERROR.key" endpoint.errored
+          "error.msg" { it == null || it == EXCEPTION.body }
+          "error.type" { it == null || it == Exception.name }
+          "error.stack" { it == null || it instanceof String }
        }
+        "$Tags.HTTP_STATUS.key" endpoint.status
+        "$Tags.HTTP_URL.key" "${endpoint.resolve(address)}"
+        "$Tags.PEER_HOSTNAME.key" { it == "localhost" || it == "127.0.0.1" }
+        "$Tags.PEER_PORT.key" Integer
+        "$Tags.PEER_HOST_IPV4.key" { it == null || it == "127.0.0.1" } // Optional
+        "$Tags.HTTP_METHOD.key" method
+        "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER
      }
    }
  }
--- a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyTestInstrumentation.java
+++ b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyTestInstrumentation.java
@ -0,0 +1,33 @@
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+import com.google.auto.service.AutoService;
+import datadog.trace.agent.test.base.HttpServerTestAdvice;
+import datadog.trace.agent.tooling.Instrumenter;
+import net.bytebuddy.agent.builder.AgentBuilder;
+
+@AutoService(Instrumenter.class)
+public class JettyTestInstrumentation implements Instrumenter {
+
+  @Override
+  public AgentBuilder instrument(final AgentBuilder agentBuilder) {
+    return agentBuilder
+        // Jetty 8.0
+        .type(named("org.eclipse.jetty.server.HttpConnection"))
+        .transform(
+            new AgentBuilder.Transformer.ForAdvice()
+                .advice(
+                    named("handleRequest"), HttpServerTestAdvice.ServerEntryAdvice.class.getName()))
+        // Jetty 8.?
+        .type(named("org.eclipse.jetty.server.AbstractHttpConnection"))
+        .transform(
+            new AgentBuilder.Transformer.ForAdvice()
+                .advice(
+                    named("headerComplete"),
+                    HttpServerTestAdvice.ServerEntryAdvice.class.getName()))
+        // Jetty 9
+        .type(named("org.eclipse.jetty.server.HttpChannel"))
+        .transform(
+            new AgentBuilder.Transformer.ForAdvice()
+                .advice(named("handle"), HttpServerTestAdvice.ServerEntryAdvice.class.getName()));
+  }
+}