diff --git a/dd-java-agent/instrumentation/servlet-2/servlet-2.gradle b/dd-java-agent/instrumentation/servlet-2/servlet-2.gradle index 1910352f6c..fa45b539cf 100644 --- a/dd-java-agent/instrumentation/servlet-2/servlet-2.gradle +++ b/dd-java-agent/instrumentation/servlet-2/servlet-2.gradle @@ -11,6 +11,14 @@ muzzle { apply from: "${rootDir}/gradle/java.gradle" +apply plugin: 'org.unbroken-dome.test-sets' + +testSets { + latestDepTest { + dirName = 'test' + } +} + dependencies { compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.3' @@ -26,4 +34,7 @@ dependencies { } testCompile group: 'org.eclipse.jetty', name: 'jetty-server', version: '7.0.0.v20091005' testCompile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '7.0.0.v20091005' + + latestDepTestCompile group: 'org.eclipse.jetty', name: 'jetty-server', version: '7.+' + latestDepTestCompile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '7.+' } diff --git a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/AbstractServlet2Instrumentation.java b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/AbstractServlet2Instrumentation.java index cb2ed723a1..e1ece26995 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/AbstractServlet2Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/AbstractServlet2Instrumentation.java @@ -24,9 +24,10 @@ public abstract class AbstractServlet2Instrumentation extends Instrumenter.Defau "datadog.trace.agent.decorator.BaseDecorator", "datadog.trace.agent.decorator.ServerDecorator", "datadog.trace.agent.decorator.HttpServerDecorator", + packageName + ".Servlet2Decorator", packageName + ".HttpServletRequestExtractAdapter", packageName + ".HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", - packageName + ".Servlet2Decorator", + packageName + ".StatusSavingHttpServletResponseWrapper", }; } } diff --git a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java index f116523fd9..9b02f13edf 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java +++ b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java @@ -8,23 +8,35 @@ import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.SpanContext; import io.opentracing.propagation.Format; +import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.security.Principal; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; +import net.bytebuddy.implementation.bytecode.assign.Assigner; public class Servlet2Advice { + public static final String SERVLET_SPAN = "datadog.servlet.span"; @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope startSpan( - @Advice.This final Object servlet, @Advice.Argument(0) final ServletRequest req) { - if (GlobalTracer.get().activeSpan() != null || !(req instanceof HttpServletRequest)) { + @Advice.This final Object servlet, + @Advice.Argument(0) final ServletRequest req, + @Advice.Argument(value = 1, readOnly = false, typing = Assigner.Typing.DYNAMIC) + ServletResponse resp) { + final Object spanAttr = req.getAttribute(SERVLET_SPAN); + if (!(req instanceof HttpServletRequest) || spanAttr != null) { // Tracing might already be applied by the FilterChain. If so ignore this. return null; } + if (resp instanceof HttpServletResponse) { + resp = new StatusSavingHttpServletResponseWrapper((HttpServletResponse) resp); + } + final HttpServletRequest httpServletRequest = (HttpServletRequest) req; final SpanContext extractedContext = GlobalTracer.get() @@ -35,6 +47,7 @@ public class Servlet2Advice { final Scope scope = GlobalTracer.get() .buildSpan("servlet.request") + .ignoreActiveSpan() .asChildOf(extractedContext) .withTag("span.origin.type", servlet.getClass().getName()) .startActive(true); @@ -47,6 +60,8 @@ public class Servlet2Advice { if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(true); } + + req.setAttribute(SERVLET_SPAN, span); return scope; } @@ -68,9 +83,18 @@ public class Servlet2Advice { } if (scope != null) { - DECORATE.onResponse(scope.span(), response); - DECORATE.onError(scope.span(), throwable); - DECORATE.beforeFinish(scope.span()); + final Span span = scope.span(); + DECORATE.onResponse(span, response); + if (throwable != null) { + if (response instanceof StatusSavingHttpServletResponseWrapper + && ((StatusSavingHttpServletResponseWrapper) response).status + == HttpServletResponse.SC_OK) { + // exception was thrown but status code wasn't set + Tags.HTTP_STATUS.set(span, 500); + } + DECORATE.onError(span, throwable); + } + DECORATE.beforeFinish(span); if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(false); diff --git a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java index 597ac0bcfe..50c6998339 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java +++ b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java @@ -43,13 +43,18 @@ public class Servlet2Decorator @Override protected Integer peerPort(final HttpServletRequest httpServletRequest) { + // HttpServletResponse doesn't have accessor for remote port. return null; } @Override protected Integer status(final ServletResponse httpServletResponse) { - // HttpServletResponse doesn't have accessor for status code. - return null; + if (httpServletResponse instanceof StatusSavingHttpServletResponseWrapper) { + return ((StatusSavingHttpServletResponseWrapper) httpServletResponse).status; + } else { + // HttpServletResponse doesn't have accessor for status code. + return null; + } } @Override diff --git a/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java new file mode 100644 index 0000000000..28e50fb4cd --- /dev/null +++ b/dd-java-agent/instrumentation/servlet-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java @@ -0,0 +1,37 @@ +package datadog.trace.instrumentation.servlet2; + +import java.io.IOException; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; + +public class StatusSavingHttpServletResponseWrapper extends HttpServletResponseWrapper { + public int status = 200; + + public StatusSavingHttpServletResponseWrapper(final HttpServletResponse response) { + super(response); + } + + @Override + public void sendError(final int status) throws IOException { + this.status = status; + super.sendError(status); + } + + @Override + public void sendError(final int status, final String message) throws IOException { + this.status = status; + super.sendError(status, message); + } + + @Override + public void setStatus(final int status) { + this.status = status; + super.setStatus(status); + } + + @Override + public void setStatus(final int status, final String message) { + this.status = status; + super.setStatus(status, message); + } +} diff --git a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy index a2904fc103..023f2ea1d6 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy +++ b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy @@ -1,197 +1,116 @@ -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 datadog.trace.api.DDTags -import okhttp3.Credentials -import okhttp3.Interceptor -import okhttp3.OkHttpClient -import okhttp3.Request -import okhttp3.Response -import org.eclipse.jetty.http.HttpHeaders -import org.eclipse.jetty.http.security.Constraint -import org.eclipse.jetty.security.ConstraintMapping -import org.eclipse.jetty.security.ConstraintSecurityHandler -import org.eclipse.jetty.security.HashLoginService -import org.eclipse.jetty.security.LoginService -import org.eclipse.jetty.security.authentication.BasicAuthenticator +import datadog.trace.instrumentation.servlet2.Servlet2Decorator +import io.opentracing.tag.Tags +import javax.servlet.http.HttpServletRequest import org.eclipse.jetty.server.Server +import org.eclipse.jetty.server.handler.ErrorHandler import org.eclipse.jetty.servlet.ServletContextHandler -class JettyServlet2Test extends AgentTestRunner { +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED +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.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS - OkHttpClient client = OkHttpUtils.clientBuilder().addNetworkInterceptor(new Interceptor() { - @Override - Response intercept(Interceptor.Chain chain) throws IOException { - def response = chain.proceed(chain.request()) - TEST_WRITER.waitForTraces(1) - return response - } - }) - .build() +class JettyServlet2Test extends HttpServerTest { - int port + private static final CONTEXT = "ctx" private Server jettyServer - private ServletContextHandler servletContext - def setup() { - port = PortUtils.randomOpenPort() + @Override + void startServer(int port) { jettyServer = new Server(port) - servletContext = new ServletContextHandler() - servletContext.contextPath = "/ctx" + jettyServer.connectors.each { it.resolveNames = true } // get localhost instead of 127.0.0.1 + ServletContextHandler servletContext = new ServletContextHandler(null, "/$CONTEXT") + servletContext.errorHandler = new ErrorHandler() { + protected void handleErrorPage(HttpServletRequest request, Writer writer, int code, String message) throws IOException { + Throwable th = (Throwable) request.getAttribute("javax.servlet.error.exception") + writer.write(th ? th.message : message) + } + } - ConstraintSecurityHandler security = setupAuthentication(jettyServer) + // FIXME: Add tests for security/authentication. +// ConstraintSecurityHandler security = setupAuthentication(jettyServer) +// servletContext.setSecurityHandler(security) - servletContext.setSecurityHandler(security) - servletContext.addServlet(TestServlet2.Sync, "/sync") - servletContext.addServlet(TestServlet2.Sync, "/auth/sync") + servletContext.addServlet(TestServlet2.Sync, SUCCESS.path) + servletContext.addServlet(TestServlet2.Sync, ERROR.path) + servletContext.addServlet(TestServlet2.Sync, EXCEPTION.path) + servletContext.addServlet(TestServlet2.Sync, REDIRECT.path) + servletContext.addServlet(TestServlet2.Sync, AUTH_REQUIRED.path) jettyServer.setHandler(servletContext) jettyServer.start() } - def cleanup() { + @Override + void stopServer() { jettyServer.stop() jettyServer.destroy() } - def "test #path servlet call (auth: #auth, distributed tracing: #distributedTracing)"() { - setup: - def requestBuilder = new Request.Builder() - .url("http://localhost:$port/ctx/$path") - .get() - if (distributedTracing) { - requestBuilder.header("x-datadog-trace-id", "123") - requestBuilder.header("x-datadog-parent-id", "456") - } - if (auth) { - requestBuilder.header(HttpHeaders.AUTHORIZATION, Credentials.basic("user", "password")) - } - def response = client.newCall(requestBuilder.build()).execute() - - expect: - response.body().string().trim() == expectedResponse - - assertTraces(1) { - trace(0, 1) { - span(0) { - if (distributedTracing) { - traceId "123" - parentId "456" - } else { - parent() - } - serviceName "ctx" - operationName "servlet.request" - resourceName "GET /ctx/$path" - spanType DDSpanTypes.HTTP_SERVER - errored false - tags { - "http.url" "http://localhost:$port/ctx/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "peer.hostname" "127.0.0.1" - "peer.ipv4" "127.0.0.1" - "span.origin.type" "TestServlet2\$Sync" - "servlet.context" "/ctx" - if (auth) { - "$DDTags.USER_NAME" "user" - } - defaultTags(distributedTracing) - } - } - } - } - - where: - path | expectedResponse | auth | distributedTracing - "sync" | "Hello Sync" | false | false - "auth/sync" | "Hello Sync" | true | false - "sync" | "Hello Sync" | false | true - "auth/sync" | "Hello Sync" | true | true + @Override + URI buildAddress() { + return new URI("http://localhost:$port/$CONTEXT/") } - def "test #path error servlet call"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/ctx/$path?error=true") - .get() - .build() - def response = client.newCall(request).execute() - - expect: - response.body().string().trim() != expectedResponse - - assertTraces(1) { - trace(0, 1) { - span(0) { - serviceName "ctx" - operationName "servlet.request" - resourceName "GET /ctx/$path" - spanType DDSpanTypes.HTTP_SERVER - errored true - parent() - tags { - "http.url" "http://localhost:$port/ctx/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "peer.hostname" "127.0.0.1" - "peer.ipv4" "127.0.0.1" - "span.origin.type" "TestServlet2\$Sync" - "servlet.context" "/ctx" - errorTags(RuntimeException, "some $path error") - defaultTags() - } - } - } - } - - where: - path | expectedResponse - "sync" | "Hello Sync" + @Override + Servlet2Decorator decorator() { + return Servlet2Decorator.DECORATE } - def "test #path non-throwing-error servlet call"() { - // This doesn't actually detect the error because we can't get the status code via the old servlet API. - setup: - def request = new Request.Builder() - .url("http://localhost:$port/ctx/$path?non-throwing-error=true") - .get() - .build() - def response = client.newCall(request).execute() + @Override + String expectedServiceName() { + CONTEXT + } - expect: - response.body().string().trim() != expectedResponse + @Override + String expectedOperationName() { + return "servlet.request" + } - assertTraces(1) { - trace(0, 1) { - span(0) { - serviceName "ctx" - operationName "servlet.request" - resourceName "GET /ctx/$path" - spanType DDSpanTypes.HTTP_SERVER - errored false - parent() - tags { - "http.url" "http://localhost:$port/ctx/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "peer.hostname" "127.0.0.1" - "peer.ipv4" "127.0.0.1" - "span.origin.type" "TestServlet2\$Sync" - "servlet.context" "/ctx" - defaultTags() - } + @Override + boolean testNotFound() { + false + } + + // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) + void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + serviceName expectedServiceName() + operationName expectedOperationName() + resourceName endpoint.status == 404 ? "404" : "$method ${endpoint.resolve(address).path}" + spanType DDSpanTypes.HTTP_SERVER + errored endpoint.errored + if (parentID != null) { + traceId traceID + parentId parentID + } else { + parent() + } + tags { + "servlet.context" "/$CONTEXT" + "span.origin.type" TestServlet2.Sync.name + + 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" "localhost" + // No peer port + "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" + "$Tags.HTTP_METHOD.key" method + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER } } - - where: - path | expectedResponse - "sync" | "Hello Sync" } /** @@ -204,26 +123,26 @@ class JettyServlet2Test extends AgentTestRunner { * @param jettyServer server to attach login service * @return SecurityHandler that can be assigned to servlet */ - private ConstraintSecurityHandler setupAuthentication(Server jettyServer) { - ConstraintSecurityHandler security = new ConstraintSecurityHandler() - - Constraint constraint = new Constraint() - constraint.setName("auth") - constraint.setAuthenticate(true) - constraint.setRoles("role") - - ConstraintMapping mapping = new ConstraintMapping() - mapping.setPathSpec("/auth/*") - mapping.setConstraint(constraint) - - security.setConstraintMappings(mapping) - security.setAuthenticator(new BasicAuthenticator()) - - LoginService loginService = new HashLoginService("TestRealm", - "src/test/resources/realm.properties") - security.setLoginService(loginService) - jettyServer.addBean(loginService) - - security - } +// private ConstraintSecurityHandler setupAuthentication(Server jettyServer) { +// ConstraintSecurityHandler security = new ConstraintSecurityHandler() +// +// Constraint constraint = new Constraint() +// constraint.setName("auth") +// constraint.setAuthenticate(true) +// constraint.setRoles("role") +// +// ConstraintMapping mapping = new ConstraintMapping() +// mapping.setPathSpec("/auth/*") +// mapping.setConstraint(constraint) +// +// security.setConstraintMappings(mapping) +// security.setAuthenticator(new BasicAuthenticator()) +// +// LoginService loginService = new HashLoginService("TestRealm", +// "src/test/resources/realm.properties") +// security.setLoginService(loginService) +// jettyServer.addBean(loginService) +// +// security +// } } diff --git a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/ServletTestInstrumentation.java b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/ServletTestInstrumentation.java new file mode 100644 index 0000000000..05b6073c54 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/ServletTestInstrumentation.java @@ -0,0 +1,28 @@ +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 ServletTestInstrumentation implements Instrumenter { + + @Override + public AgentBuilder instrument(final AgentBuilder agentBuilder) { + return agentBuilder + // Jetty 7.0 + .type(named("org.eclipse.jetty.server.HttpConnection")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("handleRequest"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())) + // Jetty 7.latest + .type(named("org.eclipse.jetty.server.AbstractHttpConnection")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("headerComplete"), + HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + } +} diff --git a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/TestServlet2.groovy b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/TestServlet2.groovy index 9ddcc8d92b..45691654cc 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/TestServlet2.groovy +++ b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/TestServlet2.groovy @@ -1,21 +1,37 @@ +import datadog.trace.agent.test.base.HttpServerTest import groovy.servlet.AbstractHttpServlet - import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +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.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + class TestServlet2 { static class Sync extends AbstractHttpServlet { @Override - void doGet(HttpServletRequest req, HttpServletResponse resp) { - if (req.getParameter("error") != null) { - throw new RuntimeException("some sync error") + protected void service(HttpServletRequest req, HttpServletResponse resp) { + req.getRequestDispatcher() + HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath) + HttpServerTest.controller(endpoint) { + resp.contentType = "text/plain" + switch (endpoint) { + case SUCCESS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break + case REDIRECT: + resp.sendRedirect(endpoint.body) + break + case ERROR: + resp.sendError(endpoint.status, endpoint.body) + break + case EXCEPTION: + throw new Exception(endpoint.body) + } } - if (req.getParameter("non-throwing-error") != null) { - resp.sendError(500, "some sync error") - return - } - resp.writer.print("Hello Sync") } } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy index 92150f16bf..7908912522 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy @@ -43,7 +43,7 @@ abstract class JettyServlet3Test extends AbstractServlet3Test