From f3e9fa22ef422bebd462ae3218362baf14678a81 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 6 Aug 2019 15:14:51 -0700 Subject: [PATCH 1/4] Add HttpServerTest for redirects --- .../src/test/groovy/Netty40ServerTest.groovy | 2 +- ...tatusSavingHttpServletResponseWrapper.java | 6 +++++ .../groovy/server/VertxHttpServerTest.groovy | 2 +- ...VertxRxCircuitBreakerHttpServerTest.groovy | 2 +- .../server/VertxRxHttpServerTest.groovy | 2 +- .../agent/test/base/HttpServerTest.groovy | 26 ++++++++++++++++++- .../trace/agent/test/utils/OkHttpUtils.groovy | 2 +- 7 files changed, 36 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy b/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy index f2daaa19f9..c4539a0abf 100644 --- a/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy +++ b/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy @@ -62,7 +62,7 @@ class Netty40ServerTest extends HttpServerTest { break case REDIRECT: response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status)) - response.headers().set(HttpHeaderNames.LOCATION, endpoint.body) + response.headers().set(HttpHeaders.Names.LOCATION, endpoint.body) break case EXCEPTION: throw new Exception(endpoint.body) 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 index 28e50fb4cd..971857ca59 100644 --- 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 @@ -23,6 +23,12 @@ public class StatusSavingHttpServletResponseWrapper extends HttpServletResponseW super.sendError(status, message); } + @Override + public void sendRedirect(final String location) throws IOException { + status = 302; + super.sendRedirect(location); + } + @Override public void setStatus(final int status) { this.status = status; diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy index f4f753093b..4b5b98d583 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy @@ -85,7 +85,7 @@ class VertxHttpServerTest extends HttpServerTest { } router.route(REDIRECT.path).handler { ctx -> controller(REDIRECT) { - ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body) + ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body).end() } } router.route(ERROR.path).handler { ctx -> diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy index 56bbbbce2a..3887d0c3e1 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy @@ -56,7 +56,7 @@ class VertxRxCircuitBreakerHttpServerTest extends VertxHttpServerTest { } HttpServerTest.ServerEndpoint endpoint = it.result() controller(endpoint) { - ctx.response().setStatusCode(endpoint.status).putHeader("location", endpoint.body) + ctx.response().setStatusCode(endpoint.status).putHeader("location", endpoint.body).end() } } } diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy index a271ae0e93..e2e7637f28 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy @@ -31,7 +31,7 @@ class VertxRxHttpServerTest extends VertxHttpServerTest { } router.route(REDIRECT.path).handler { ctx -> controller(REDIRECT) { - ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body) + ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body).end() } } router.route(ERROR.path).handler { ctx -> diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index ba4ae11c3d..136c5b7df6 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -78,7 +78,7 @@ abstract class HttpServerTest extends Age enum ServerEndpoint { SUCCESS("success", 200, "success"), - REDIRECT("redirect", 302, null), + REDIRECT("redirect", 302, "/redirected"), ERROR("error", 500, "controller error"), EXCEPTION("exception", 500, "controller exception"), NOT_FOUND("notFound", 404, "not found"), @@ -184,6 +184,30 @@ abstract class HttpServerTest extends Age body = null } + def "test redirect"() { + setup: + def request = request(REDIRECT, method, body).build() + def response = client.newCall(request).execute() + + expect: + response.code() == REDIRECT.status + response.header("location") == REDIRECT.body || + response.header("location") == "${address.resolve(REDIRECT.body)}" + response.body().contentLength() < 1 + + and: + cleanAndAssertTraces(1) { + trace(0, 2) { + serverSpan(it, 0, null, null, method, REDIRECT) + controllerSpan(it, 1, span(0)) + } + } + + where: + method = "GET" + body = null + } + def "test error"() { setup: def request = request(ERROR, method, body).build() diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy index 81a9ca13a3..839c8cb2fa 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy @@ -15,6 +15,6 @@ class OkHttpUtils { } static client() { - clientBuilder().build() + clientBuilder().followRedirects(false).build() } } From 49249c0c6e48563cbc5859bd0e3c9e1209c4b537 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 6 Aug 2019 15:21:58 -0700 Subject: [PATCH 2/4] Move server field to HttpServerTest --- .../AkkaHttpServerInstrumentationTest.groovy | 14 ++++++------- .../src/test/groovy/Netty40ServerTest.groovy | 16 +++++++-------- .../src/test/groovy/Netty41ServerTest.groovy | 15 +++++++------- .../src/test/groovy/JettyServlet2Test.groovy | 15 +++++++------- .../test/groovy/AbstractServlet3Test.groovy | 2 +- .../src/test/groovy/JettyServlet3Test.groovy | 19 +++++++----------- .../src/test/groovy/TomcatServlet3Test.groovy | 20 ++++++++----------- .../groovy/server/VertxHttpServerTest.groovy | 13 +++++------- .../agent/test/base/HttpServerTest.groovy | 18 ++++++++++------- 9 files changed, 62 insertions(+), 70 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy index 5c158ef414..78e0092e18 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy @@ -7,7 +7,7 @@ import io.opentracing.tag.Tags import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS -abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest { +abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest { @Override AkkaHttpServerDecorator decorator() { @@ -26,12 +26,12 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest { - @Shared - EventLoopGroup eventLoopGroup +class Netty40ServerTest extends HttpServerTest { @Override - void startServer(int port) { - eventLoopGroup = new NioEventLoopGroup() + EventLoopGroup startServer(int port) { + def eventLoopGroup = new NioEventLoopGroup() ServerBootstrap bootstrap = new ServerBootstrap() .group(eventLoopGroup) @@ -91,11 +89,13 @@ class Netty40ServerTest extends HttpServerTest { } ] as ChannelInitializer).channel(NioServerSocketChannel) bootstrap.bind(port).sync() + + return eventLoopGroup } @Override - void stopServer() { - eventLoopGroup?.shutdownGracefully() + void stopServer(EventLoopGroup server) { + server?.shutdownGracefully() } @Override diff --git a/dd-java-agent/instrumentation/netty-4.1/src/test/groovy/Netty41ServerTest.groovy b/dd-java-agent/instrumentation/netty-4.1/src/test/groovy/Netty41ServerTest.groovy index 911861eae0..2eb3f6f53b 100644 --- a/dd-java-agent/instrumentation/netty-4.1/src/test/groovy/Netty41ServerTest.groovy +++ b/dd-java-agent/instrumentation/netty-4.1/src/test/groovy/Netty41ServerTest.groovy @@ -19,7 +19,6 @@ import io.netty.handler.codec.http.HttpServerCodec import io.netty.handler.logging.LogLevel import io.netty.handler.logging.LoggingHandler import io.netty.util.CharsetUtil -import spock.lang.Shared import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION @@ -31,13 +30,11 @@ import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1 -class Netty41ServerTest extends HttpServerTest { - @Shared - EventLoopGroup eventLoopGroup +class Netty41ServerTest extends HttpServerTest { @Override - void startServer(int port) { - eventLoopGroup = new NioEventLoopGroup() + EventLoopGroup startServer(int port) { + def eventLoopGroup = new NioEventLoopGroup() ServerBootstrap bootstrap = new ServerBootstrap() .group(eventLoopGroup) @@ -91,11 +88,13 @@ class Netty41ServerTest extends HttpServerTest { } ] as ChannelInitializer).channel(NioServerSocketChannel) bootstrap.bind(port).sync() + + return eventLoopGroup } @Override - void stopServer() { - eventLoopGroup?.shutdownGracefully() + void stopServer(EventLoopGroup server) { + server?.shutdownGracefully() } @Override 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 023f2ea1d6..aa335de360 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 @@ -14,14 +14,13 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS -class JettyServlet2Test extends HttpServerTest { +class JettyServlet2Test extends HttpServerTest { private static final CONTEXT = "ctx" - private Server jettyServer @Override - void startServer(int port) { - jettyServer = new Server(port) + Server startServer(int port) { + def jettyServer = new Server(port) jettyServer.connectors.each { it.resolveNames = true } // get localhost instead of 127.0.0.1 ServletContextHandler servletContext = new ServletContextHandler(null, "/$CONTEXT") servletContext.errorHandler = new ErrorHandler() { @@ -43,12 +42,14 @@ class JettyServlet2Test extends HttpServerTest { jettyServer.setHandler(servletContext) jettyServer.start() + + return jettyServer } @Override - void stopServer() { - jettyServer.stop() - jettyServer.destroy() + void stopServer(Server server) { + server.stop() + server.destroy() } @Override diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/AbstractServlet3Test.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/AbstractServlet3Test.groovy index f1c97c8a8c..8b62d164f4 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/AbstractServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/AbstractServlet3Test.groovy @@ -13,7 +13,7 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS -abstract class AbstractServlet3Test extends HttpServerTest { +abstract class AbstractServlet3Test extends HttpServerTest { @Override URI buildAddress() { return new URI("http://localhost:$port/$context/") 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 7908912522..4dbbe8a558 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 @@ -10,7 +10,6 @@ import org.apache.catalina.core.ApplicationFilterChain import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.ErrorHandler import org.eclipse.jetty.servlet.ServletContextHandler -import spock.lang.Shared import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED @@ -20,10 +19,7 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRE import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS import static datadog.trace.agent.test.utils.TraceUtils.basicSpan -abstract class JettyServlet3Test extends AbstractServlet3Test { - - @Shared - private Server jettyServer +abstract class JettyServlet3Test extends AbstractServlet3Test { @Override boolean testNotFound() { @@ -31,8 +27,8 @@ abstract class JettyServlet3Test extends AbstractServlet3Test { - - @Shared - Tomcat tomcatServer +abstract class TomcatServlet3Test extends AbstractServlet3Test { @Override - void startServer(int port) { - tomcatServer = new Tomcat() + Tomcat startServer(int port) { + def tomcatServer = new Tomcat() def baseDir = Files.createTempDir() baseDir.deleteOnExit() @@ -62,14 +58,14 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test { (tomcatServer.host as StandardHost).errorReportValveClass = ErrorHandlerValve.name tomcatServer.start() - System.out.println( - "Tomcat server: http://" + tomcatServer.getHost().getName() + ":" + port + "/") + + return tomcatServer } @Override - void stopServer() { - tomcatServer.stop() - tomcatServer.destroy() + void stopServer(Tomcat server) { + server.stop() + server.destroy() } @Override diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy index 4b5b98d583..f3f963b769 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxHttpServerTest.groovy @@ -12,7 +12,6 @@ import io.vertx.core.Vertx import io.vertx.core.VertxOptions import io.vertx.core.json.JsonObject import io.vertx.ext.web.Router -import spock.lang.Shared import java.util.concurrent.CompletableFuture @@ -21,15 +20,12 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS -class VertxHttpServerTest extends HttpServerTest { +class VertxHttpServerTest extends HttpServerTest { public static final String CONFIG_HTTP_SERVER_PORT = "http.server.port" - @Shared - Vertx server - @Override - void startServer(int port) { - server = Vertx.vertx(new VertxOptions() + Vertx startServer(int port) { + def server = Vertx.vertx(new VertxOptions() // Useful for debugging: // .setBlockedThreadCheckInterval(Integer.MAX_VALUE) .setClusterPort(port)) @@ -45,6 +41,7 @@ class VertxHttpServerTest extends HttpServerTest { } future.get() + return server } protected Class verticle() { @@ -52,7 +49,7 @@ class VertxHttpServerTest extends HttpServerTest { } @Override - void stopServer() { + void stopServer(Vertx server) { server.close() } diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 136c5b7df6..c07a502ce6 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -24,14 +24,17 @@ import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace 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 import static datadog.trace.agent.test.utils.TraceUtils.basicSpan import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace import static org.junit.Assume.assumeTrue @Unroll -abstract class HttpServerTest extends AgentTestRunner { +abstract class HttpServerTest extends AgentTestRunner { + @Shared + SERVER server @Shared OkHttpClient client = OkHttpUtils.client() @Shared @@ -47,18 +50,19 @@ abstract class HttpServerTest extends Age DECORATOR serverDecorator = decorator() def setupSpec() { - startServer(port) - println "Http server started at: http://localhost:$port/" + server = startServer(port) + println getClass().name + " http server started at: http://localhost:$port/" } - abstract void startServer(int port) + abstract SERVER startServer(int port) def cleanupSpec() { - stopServer() - println "Http server stopped at: http://localhost:$port/" + stopServer(server) + server = null + println getClass().name + " http server stopped at: http://localhost:$port/" } - abstract void stopServer() + abstract void stopServer(SERVER server) abstract DECORATOR decorator() From 6dd729b8432eef1a1c24d813871c41d1f5b1ceed Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 7 Aug 2019 08:32:32 -0700 Subject: [PATCH 3/4] Jetty 8: ignore parent and move to HttpServerTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../instrumentation/jetty-8/jetty-8.gradle | 2 + .../jetty8/JettyHandlerAdvice.java | 7 +- ....java => JettyHandlerInstrumentation.java} | 4 +- .../JettyContinuationHandlerTest.groovy | 62 ++++ .../src/test/groovy/JettyHandlerTest.groovy | 281 +++++++----------- .../test/groovy/JettyTestInstrumentation.java | 33 ++ 6 files changed, 211 insertions(+), 178 deletions(-) rename dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/{HandlerInstrumentation.java => JettyHandlerInstrumentation.java} (94%) create mode 100644 dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyContinuationHandlerTest.groovy create mode 100644 dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyTestInstrumentation.java 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())); + } +} From 2d08464be3bb8f9db0b354c61dbb9279c0354aa6 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 7 Aug 2019 10:48:14 -0700 Subject: [PATCH 4/4] Fixes --- .../src/test/groovy/JettyHandlerTest.groovy | 31 ++++++++++++------- .../src/test/groovy/SpringWebfluxTest.groovy | 2 +- .../trace/agent/test/utils/OkHttpUtils.groovy | 4 +-- 3 files changed, 22 insertions(+), 15 deletions(-) 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 eb38402f26..8b7a234ff7 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 @@ -3,6 +3,7 @@ import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes import datadog.trace.instrumentation.jetty8.JettyDecorator import io.opentracing.tag.Tags +import javax.servlet.DispatcherType import javax.servlet.ServletException import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse @@ -23,20 +24,22 @@ class JettyHandlerTest extends HttpServerTest { System.setProperty("dd.integration.jetty.enabled", "true") } + static errorHandler = 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) + } + } + } + @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.addBean(errorHandler) server.start() return server } @@ -95,8 +98,12 @@ class JettyHandlerTest extends HttpServerTest { @Override void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { - handleRequest(baseRequest, response) - baseRequest.handled = true + if (baseRequest.dispatcherType != DispatcherType.ERROR) { + handleRequest(baseRequest, response) + baseRequest.handled = true + } else { + errorHandler.handle(target, baseRequest, response, response) + } } } diff --git a/dd-java-agent/instrumentation/spring-webflux/src/test/groovy/SpringWebfluxTest.groovy b/dd-java-agent/instrumentation/spring-webflux/src/test/groovy/SpringWebfluxTest.groovy index 4ac26f8f0b..8e09585c34 100644 --- a/dd-java-agent/instrumentation/spring-webflux/src/test/groovy/SpringWebfluxTest.groovy +++ b/dd-java-agent/instrumentation/spring-webflux/src/test/groovy/SpringWebfluxTest.groovy @@ -34,7 +34,7 @@ class SpringWebfluxTest extends AgentTestRunner { @LocalServerPort private int port - OkHttpClient client = OkHttpUtils.client() + OkHttpClient client = OkHttpUtils.client(true) def "Basic GET test #testName"() { setup: diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy index 839c8cb2fa..aa630a2c52 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy @@ -14,7 +14,7 @@ class OkHttpUtils { .readTimeout(1, unit) } - static client() { - clientBuilder().followRedirects(false).build() + static client(boolean followRedirects = false) { + clientBuilder().followRedirects(followRedirects).build() } }