From 09ad2374cf83e80f871e0d47196414b6da8597a6 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 14 Aug 2019 09:20:48 -0700 Subject: [PATCH] Disable Grizzly instrumentation by default Otherwise it can interfere with the more common Servlet instrumentation (changing the root span name). Unify attribute/property name for saving span on a request/context. Also add tests for embedded GlassFish. --- .../agent/decorator/HttpServerDecorator.java | 2 + .../glassfish/glassfish.gradle | 19 +-- ...ion.java => GlassFishInstrumentation.java} | 6 +- .../test/groovy/GlassFishServerTest.groovy | 125 ++++++++++++++++++ .../groovy/GrizzlyTestInstrumentation.java | 20 +++ .../src/test/groovy/TestServlets.java | 79 +++++++++++ .../src/test/resources/WEB-INF/web.xml | 10 ++ .../glassfish/src/test/resources/error.jsp | 7 + .../GrizzlyHttpHandlerInstrumentation.java | 15 ++- .../src/test/groovy/GrizzlyTest.groovy | 4 + .../src/test/groovy/JerseyTest.groovy | 1 + .../v1/JaxRsClientV1Instrumentation.java | 5 +- .../jetty8/JettyHandlerAdvice.java | 6 +- .../servlet2/Servlet2Advice.java | 6 +- .../servlet3/AsyncContextInstrumentation.java | 6 +- .../servlet3/Servlet3Advice.java | 12 +- .../agent/test/base/HttpServerTest.groovy | 10 +- 17 files changed, 299 insertions(+), 34 deletions(-) rename dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/{GlassfishInstrumentation.java => GlassFishInstrumentation.java} (92%) create mode 100644 dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy create mode 100644 dd-java-agent/instrumentation/glassfish/src/test/groovy/GrizzlyTestInstrumentation.java create mode 100644 dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java create mode 100644 dd-java-agent/instrumentation/glassfish/src/test/resources/WEB-INF/web.xml create mode 100644 dd-java-agent/instrumentation/glassfish/src/test/resources/error.jsp diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java index 302461ed38..c8fa34d1da 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java @@ -12,6 +12,8 @@ import lombok.extern.slf4j.Slf4j; @Slf4j public abstract class HttpServerDecorator extends ServerDecorator { + public static final String DD_SPAN_ATTRIBUTE = "datadog.span"; + // Source: https://www.regextester.com/22 private static final Pattern VALID_IPV4_ADDRESS = Pattern.compile( diff --git a/dd-java-agent/instrumentation/glassfish/glassfish.gradle b/dd-java-agent/instrumentation/glassfish/glassfish.gradle index 7c72894da2..0051df0c82 100644 --- a/dd-java-agent/instrumentation/glassfish/glassfish.gradle +++ b/dd-java-agent/instrumentation/glassfish/glassfish.gradle @@ -1,6 +1,6 @@ -apply from: "${rootDir}/gradle/java.gradle" -apply plugin: 'idea' -apply plugin: 'org.unbroken-dome.test-sets' +ext { + maxJavaVersionForTests = JavaVersion.VERSION_1_8 +} muzzle { pass { @@ -11,6 +11,10 @@ muzzle { } } +apply from: "${rootDir}/gradle/java.gradle" + +apply plugin: 'org.unbroken-dome.test-sets' + testSets { latestDepTest { dirName = 'test' @@ -18,10 +22,9 @@ testSets { } dependencies { + testCompile project(':dd-java-agent:instrumentation:servlet-3') + testCompile project(':dd-java-agent:instrumentation:grizzly-2') - compile project(':dd-java-agent:agent-tooling') - - testCompile group: 'org.glassfish.main.extras', name: 'glassfish-embedded-all', version: '4.1.2' - - latestDepTestCompile sourceSets.test.output + testCompile group: 'org.glassfish.main.extras', name: 'glassfish-embedded-all', version: '4.0' + latestDepTestCompile group: 'org.glassfish.main.extras', name: 'glassfish-embedded-all', version: '+' } diff --git a/dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassfishInstrumentation.java b/dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassFishInstrumentation.java similarity index 92% rename from dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassfishInstrumentation.java rename to dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassFishInstrumentation.java index 2da4abed03..9516fdb391 100644 --- a/dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassfishInstrumentation.java +++ b/dd-java-agent/instrumentation/glassfish/src/main/java/datadog/trace/instrumentation/glassfish/GlassFishInstrumentation.java @@ -26,9 +26,9 @@ import net.bytebuddy.matcher.ElementMatchers; */ @Slf4j @AutoService(Instrumenter.class) -public final class GlassfishInstrumentation extends Instrumenter.Default { +public final class GlassFishInstrumentation extends Instrumenter.Default { - public GlassfishInstrumentation() { + public GlassFishInstrumentation() { super("glassfish"); } @@ -55,7 +55,7 @@ public final class GlassfishInstrumentation extends Instrumenter.Default { @Advice.OnMethodEnter(suppress = Throwable.class) public static void preventBlacklistingOfTracerClasses( @Advice.Argument(value = 0, readOnly = false) String name) { - for (String prefix : Constants.BOOTSTRAP_PACKAGE_PREFIXES) { + for (final String prefix : Constants.BOOTSTRAP_PACKAGE_PREFIXES) { if (name.startsWith(prefix)) { name = "__datadog_no_blacklist." + name; break; diff --git a/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy new file mode 100644 index 0000000000..af0bc70b4b --- /dev/null +++ b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy @@ -0,0 +1,125 @@ +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.servlet3.Servlet3Decorator +import io.opentracing.tag.Tags +import org.apache.catalina.servlets.DefaultServlet +import org.glassfish.embeddable.BootstrapProperties +import org.glassfish.embeddable.Deployer +import org.glassfish.embeddable.GlassFish +import org.glassfish.embeddable.GlassFishProperties +import org.glassfish.embeddable.GlassFishRuntime +import org.glassfish.embeddable.archive.ScatteredArchive + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +/** + * Unfortunately because we're using an embedded GlassFish instance, we aren't exercising the standard + * OSGi setup that required {@link datadog.trace.instrumentation.glassfish.GlassFishInstrumentation}. + */ +// TODO: Figure out a better way to test with OSGi included. +class GlassFishServerTest extends HttpServerTest { + +// static { +// System.setProperty("dd.integration.grizzly.enabled", "true") +// } + + @Override + URI buildAddress() { + return new URI("http://localhost:$port/$context/") + } + + String getContext() { + "test-gf" + } + + @Override + GlassFish startServer(int port) { + // Setup the deployment archive + def testDir = new File(TestServlets.protectionDomain.codeSource.location.path) + assert testDir.exists() && testDir.directory + def testResourcesDir = new File(TestServlets.getResource("error.jsp").path).parentFile + assert testResourcesDir.exists() && testResourcesDir.directory + ScatteredArchive archive = new ScatteredArchive(context, ScatteredArchive.Type.WAR, testResourcesDir) + archive.addClassPath(testDir) + + // Initialize the server + BootstrapProperties bootstrapProperties = new BootstrapProperties() + GlassFishRuntime glassfishRuntime = GlassFishRuntime.bootstrap(bootstrapProperties) + GlassFishProperties glassfishProperties = new GlassFishProperties() + glassfishProperties.setPort('http-listener', port) + def server = glassfishRuntime.newGlassFish(glassfishProperties) + server.start() + + // Deploy war to server + Deployer deployer = server.getDeployer() + println "Deploying $testDir.absolutePath with $testResourcesDir.absolutePath" + deployer.deploy(archive.toURI()) + + return server + } + + @Override + void stopServer(GlassFish server) { + server.stop() + } + + @Override + Servlet3Decorator decorator() { + return Servlet3Decorator.DECORATE + } + + @Override + String expectedOperationName() { + return "servlet.request" + } + + @Override + String expectedServiceName() { + context + } + + @Override + boolean redirectHasBody() { + true + } + + @Override + 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" { it.startsWith("TestServlets\$") || it == DefaultServlet.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" { 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/glassfish/src/test/groovy/GrizzlyTestInstrumentation.java b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GrizzlyTestInstrumentation.java new file mode 100644 index 0000000000..fb6b5390cf --- /dev/null +++ b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GrizzlyTestInstrumentation.java @@ -0,0 +1,20 @@ +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 GrizzlyTestInstrumentation implements Instrumenter { + + @Override + public AgentBuilder instrument(final AgentBuilder agentBuilder) { + return agentBuilder + .type(named("org.glassfish.grizzly.http.server.HttpServerFilter")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("handleRead"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + } +} diff --git a/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java b/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java new file mode 100644 index 0000000000..44824bf7bd --- /dev/null +++ b/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java @@ -0,0 +1,79 @@ +import datadog.trace.agent.test.base.HttpServerTest; +import groovy.lang.Closure; +import javax.servlet.annotation.WebServlet; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +public class TestServlets { + + @WebServlet("/success") + public static class Success extends HttpServlet { + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) { + final HttpServerTest.ServerEndpoint endpoint = + HttpServerTest.ServerEndpoint.forPath(req.getServletPath()); + HttpServerTest.controller( + endpoint, + new Closure(null) { + public Object doCall() throws Exception { + resp.setContentType("text/plain"); + resp.setStatus(endpoint.getStatus()); + resp.getWriter().print(endpoint.getBody()); + return null; + } + }); + } + } + + @WebServlet("/redirect") + public static class Redirect extends HttpServlet { + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) { + final HttpServerTest.ServerEndpoint endpoint = + HttpServerTest.ServerEndpoint.forPath(req.getServletPath()); + HttpServerTest.controller( + endpoint, + new Closure(null) { + public Object doCall() throws Exception { + resp.sendRedirect(endpoint.getBody()); + return null; + } + }); + } + } + + @WebServlet("/error") + public static class Error extends HttpServlet { + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) { + final HttpServerTest.ServerEndpoint endpoint = + HttpServerTest.ServerEndpoint.forPath(req.getServletPath()); + HttpServerTest.controller( + endpoint, + new Closure(null) { + public Object doCall() throws Exception { + resp.setContentType("text/plain"); + resp.sendError(endpoint.getStatus(), endpoint.getBody()); + return null; + } + }); + } + } + + @WebServlet("/exception") + public static class ExceptionServlet extends HttpServlet { + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) { + final HttpServerTest.ServerEndpoint endpoint = + HttpServerTest.ServerEndpoint.forPath(req.getServletPath()); + HttpServerTest.controller( + endpoint, + new Closure(null) { + public Object doCall() throws Exception { + throw new Exception(endpoint.getBody()); + } + }); + } + } +} diff --git a/dd-java-agent/instrumentation/glassfish/src/test/resources/WEB-INF/web.xml b/dd-java-agent/instrumentation/glassfish/src/test/resources/WEB-INF/web.xml new file mode 100644 index 0000000000..aa47059b6d --- /dev/null +++ b/dd-java-agent/instrumentation/glassfish/src/test/resources/WEB-INF/web.xml @@ -0,0 +1,10 @@ + + + + /error.jsp + + diff --git a/dd-java-agent/instrumentation/glassfish/src/test/resources/error.jsp b/dd-java-agent/instrumentation/glassfish/src/test/resources/error.jsp new file mode 100644 index 0000000000..9e701b8dac --- /dev/null +++ b/dd-java-agent/instrumentation/glassfish/src/test/resources/error.jsp @@ -0,0 +1,7 @@ +<%@ page contentType="text/plain;charset=UTF-8" isErrorPage="true" trimDirectiveWhitespaces="true" %> + +<% if (exception != null) {%> + <%= exception.getMessage() %> +<% } else { %> + <%= request.getAttribute("javax.servlet.error.message") %> +<% } %> diff --git a/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java b/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java index 3e3334d6dd..ddc098e488 100644 --- a/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java +++ b/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.grizzly; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.instrumentation.grizzly.GrizzlyDecorator.DECORATE; import static io.opentracing.propagation.Format.Builtin.TEXT_MAP; import static java.util.Collections.singletonMap; @@ -30,6 +31,11 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default { super("grizzly"); } + @Override + public boolean defaultEnabled() { + return false; + } + @Override public ElementMatcher typeMatcher() { return named("org.glassfish.grizzly.http.server.HttpHandler"); @@ -62,7 +68,7 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default { @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope methodEnter(@Advice.Argument(0) final Request request) { - if (request.getAttribute(SpanClosingListener.GRIZZLY_SPAN_SPAN) != null) { + if (request.getAttribute(DD_SPAN_ATTRIBUTE) != null) { return null; } @@ -80,7 +86,7 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default { ((TraceScope) scope).setAsyncPropagation(true); } - request.setAttribute(SpanClosingListener.GRIZZLY_SPAN_SPAN, span); + request.setAttribute(DD_SPAN_ATTRIBUTE, span); request.addAfterServiceListener(SpanClosingListener.LISTENER); return scope; @@ -104,14 +110,13 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default { } public static class SpanClosingListener implements AfterServiceListener { - public static final String GRIZZLY_SPAN_SPAN = "datadog.grizzly.span"; public static final SpanClosingListener LISTENER = new SpanClosingListener(); @Override public void onAfterService(final Request request) { - final Object spanAttr = request.getAttribute(GRIZZLY_SPAN_SPAN); + final Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); if (spanAttr instanceof Span) { - request.removeAttribute(GRIZZLY_SPAN_SPAN); + request.removeAttribute(DD_SPAN_ATTRIBUTE); final Span span = (Span) spanAttr; DECORATE.onResponse(span, request.getResponse()); DECORATE.beforeFinish(span); diff --git a/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyTest.groovy b/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyTest.groovy index 1f8031d039..44e96ba450 100644 --- a/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyTest.groovy +++ b/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyTest.groovy @@ -16,6 +16,10 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCES class GrizzlyTest extends HttpServerTest { + static { + System.setProperty("dd.integration.grizzly.enabled", "true") + } + @Override HttpServer startServer(int port) { ResourceConfig rc = new ResourceConfig() diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy index d5731bd625..8735f50e5e 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy @@ -7,6 +7,7 @@ import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace class JerseyTest extends AgentTestRunner { + // FIXME: migrate test. @Shared @ClassRule ResourceTestRule resources = ResourceTestRule.builder().addResource(new TestResource()).build() diff --git a/dd-java-agent/instrumentation/jax-rs-client-1.9/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java b/dd-java-agent/instrumentation/jax-rs-client-1.9/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java index 517fc5a94c..1db99fc39d 100644 --- a/dd-java-agent/instrumentation/jax-rs-client-1.9/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client-1.9/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.jaxrs.v1; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.jaxrs.v1.JaxRsClientV1Decorator.DECORATE; import static java.util.Collections.singletonMap; @@ -66,7 +67,7 @@ public final class JaxRsClientV1Instrumentation extends Instrumenter.Default { @Advice.This final ClientHandler thisObj) { // WARNING: this might be a chain...so we only have to trace the first in the chain. - final boolean isRootClientHandler = null == request.getProperties().get("dd.span"); + final boolean isRootClientHandler = null == request.getProperties().get(DD_SPAN_ATTRIBUTE); if (isRootClientHandler) { final Tracer tracer = GlobalTracer.get(); final Span span = @@ -76,7 +77,7 @@ public final class JaxRsClientV1Instrumentation extends Instrumenter.Default { .start(); DECORATE.afterStart(span); DECORATE.onRequest(span, request); - request.getProperties().put("dd.span", span); + request.getProperties().put(DD_SPAN_ATTRIBUTE, span); tracer.inject( span.context(), Format.Builtin.HTTP_HEADERS, new InjectAdapter(request.getHeaders())); 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 d5ce3e8ec9..7cb8193812 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 @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.jetty8; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.instrumentation.jetty8.JettyDecorator.DECORATE; import datadog.trace.api.DDTags; @@ -16,13 +17,12 @@ 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 (req.getAttribute(SERVLET_SPAN) != null) { + if (req.getAttribute(DD_SPAN_ATTRIBUTE) != null) { // Request already being traced elsewhere. return null; } @@ -48,7 +48,7 @@ public class JettyHandlerAdvice { if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(true); } - req.setAttribute(SERVLET_SPAN, span); + req.setAttribute(DD_SPAN_ATTRIBUTE, span); return scope; } 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 9b02f13edf..23cdd25894 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 @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.servlet2; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.instrumentation.servlet2.Servlet2Decorator.DECORATE; import datadog.trace.api.DDTags; @@ -19,7 +20,6 @@ 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( @@ -27,7 +27,7 @@ public class Servlet2Advice { @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); + final Object spanAttr = req.getAttribute(DD_SPAN_ATTRIBUTE); if (!(req instanceof HttpServletRequest) || spanAttr != null) { // Tracing might already be applied by the FilterChain. If so ignore this. return null; @@ -61,7 +61,7 @@ public class Servlet2Advice { ((TraceScope) scope).setAsyncPropagation(true); } - req.setAttribute(SERVLET_SPAN, span); + req.setAttribute(DD_SPAN_ATTRIBUTE, span); return scope; } diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java index 9c819412df..67c714240c 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.servlet3; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static datadog.trace.instrumentation.servlet3.Servlet3Advice.SERVLET_SPAN; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -63,9 +63,9 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default { } final ServletRequest request = context.getRequest(); - final Object spanAttr = request.getAttribute(SERVLET_SPAN); + final Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); if (spanAttr instanceof Span) { - request.removeAttribute(SERVLET_SPAN); + request.removeAttribute(DD_SPAN_ATTRIBUTE); final Span span = (Span) spanAttr; // Override propagation headers by injecting attributes from the current span // into the new request diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java index 0b2fb0d93b..7cc97a8a32 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.servlet3; +import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.instrumentation.servlet3.Servlet3Decorator.DECORATE; import datadog.trace.api.DDTags; @@ -19,12 +20,11 @@ import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; public class Servlet3Advice { - 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) { - final Object spanAttr = req.getAttribute(SERVLET_SPAN); + final Object spanAttr = req.getAttribute(DD_SPAN_ATTRIBUTE); if (!(req instanceof HttpServletRequest) || spanAttr != null) { // Tracing might already be applied by the FilterChain. If so ignore this. return null; @@ -54,7 +54,7 @@ public class Servlet3Advice { ((TraceScope) scope).setAsyncPropagation(true); } - req.setAttribute(SERVLET_SPAN, span); + req.setAttribute(DD_SPAN_ATTRIBUTE, span); return scope; } @@ -65,7 +65,7 @@ public class Servlet3Advice { @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { // Set user.principal regardless of who created this span. - final Object spanAttr = request.getAttribute(SERVLET_SPAN); + final Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); if (spanAttr instanceof Span && request instanceof HttpServletRequest) { final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); if (principal != null) { @@ -87,7 +87,7 @@ public class Servlet3Advice { } DECORATE.onError(span, throwable); DECORATE.beforeFinish(span); - req.removeAttribute(SERVLET_SPAN); + req.removeAttribute(DD_SPAN_ATTRIBUTE); span.finish(); // Finish the span manually since finishSpanOnClose was false } else { final AtomicBoolean activated = new AtomicBoolean(false); @@ -103,7 +103,7 @@ public class Servlet3Advice { if (!req.isAsyncStarted() && activated.compareAndSet(false, true)) { DECORATE.onResponse(span, resp); DECORATE.beforeFinish(span); - req.removeAttribute(SERVLET_SPAN); + req.removeAttribute(DD_SPAN_ATTRIBUTE); span.finish(); // Finish the span manually since finishSpanOnClose was false } } 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 984b8f43bc..d9da85a46c 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 @@ -8,9 +8,11 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.utils.OkHttpUtils import datadog.trace.agent.test.utils.PortUtils import datadog.trace.api.DDSpanTypes +import datadog.trace.context.TraceScope import groovy.transform.stc.ClosureParams import groovy.transform.stc.SimpleType import io.opentracing.tag.Tags +import io.opentracing.util.GlobalTracer import okhttp3.HttpUrl import okhttp3.OkHttpClient import okhttp3.Request @@ -89,6 +91,10 @@ abstract class HttpServerTest ext false } + boolean redirectHasBody() { + false + } + boolean testNotFound() { true } @@ -143,6 +149,7 @@ abstract class HttpServerTest ext } static T controller(ServerEndpoint endpoint, Closure closure) { + assert ((TraceScope) GlobalTracer.get().scopeManager().active()).asyncPropagating if (endpoint == NOT_FOUND) { return closure() } @@ -230,7 +237,7 @@ abstract class HttpServerTest ext response.code() == REDIRECT.status response.header("location") == REDIRECT.body || response.header("location") == "${address.resolve(REDIRECT.body)}" - response.body().contentLength() < 1 + response.body().contentLength() < 1 || redirectHasBody() and: cleanAndAssertTraces(1) { @@ -376,6 +383,7 @@ abstract class HttpServerTest ext } } } + if (reorderControllerSpan() || reorderHandlerSpan()) { // Some frameworks close the handler span before the controller returns, so we need to manually reorder it. TEST_WRITER.each {