diff --git a/dd-java-agent/instrumentation/jetty-8/jetty-8.gradle b/dd-java-agent/instrumentation/jetty-8/jetty-8.gradle index efefb998d8..0a54a83ebf 100644 --- 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' diff --git 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 index fc68e2964b..d5ce3e8ec9 100644 --- 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; } diff --git a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/HandlerInstrumentation.java b/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerInstrumentation.java similarity index 94% rename from dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/HandlerInstrumentation.java rename to dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyHandlerInstrumentation.java index bef72ef28c..68b0b875b7 100644 --- a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/HandlerInstrumentation.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"); } diff --git a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyContinuationHandlerTest.groovy b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyContinuationHandlerTest.groovy new file mode 100644 index 0000000000..cb8328db97 --- /dev/null +++ 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) +// } +} diff --git a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy index 3a3c8c9aee..eb38402f26 100644 --- 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 { 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 } } } diff --git a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyTestInstrumentation.java b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyTestInstrumentation.java new file mode 100644 index 0000000000..115329ae07 --- /dev/null +++ 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())); + } +}