diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AbstractServlet3Instrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AbstractServlet3Instrumentation.java deleted file mode 100644 index 70843b55f39..00000000000 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AbstractServlet3Instrumentation.java +++ /dev/null @@ -1,19 +0,0 @@ -package datadog.trace.instrumentation.servlet3; - -import datadog.trace.agent.tooling.Instrumenter; - -public abstract class AbstractServlet3Instrumentation extends Instrumenter.Default { - - public AbstractServlet3Instrumentation() { - super("servlet", "servlet-3"); - } - - @Override - public String[] helperClassNames() { - return new String[] { - "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter", - "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", - "datadog.trace.instrumentation.servlet3.TagSettingAsyncListener" - }; - } -} 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 new file mode 100644 index 00000000000..13200e532c0 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java @@ -0,0 +1,97 @@ +package datadog.trace.instrumentation.servlet3; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.servlet3.Servlet3Advice.SERVLET_SPAN; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.bootstrap.CallDepthThreadLocalMap; +import io.opentracing.Span; +import io.opentracing.propagation.Format; +import io.opentracing.util.GlobalTracer; +import java.util.Collections; +import java.util.Map; +import javax.servlet.AsyncContext; +import javax.servlet.ServletRequest; +import javax.servlet.http.HttpServletRequest; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class AsyncContextInstrumentation extends Instrumenter.Default { + + public AsyncContextInstrumentation() { + super("servlet", "servlet-3"); + } + + @Override + public String[] helperClassNames() { + return new String[] {"datadog.trace.instrumentation.servlet3.HttpServletRequestInjectAdapter"}; + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()).and(safeHasSuperType(named("javax.servlet.AsyncContext"))); + } + + @Override + public Map transformers() { + return Collections.singletonMap( + isMethod().and(isPublic()).and(named("dispatch")), DispatchAdvice.class.getName()); + } + + /** + * When a request is dispatched, we want to close out the existing request and replace the + * propagation info in the headers with the closed span. + */ + public static class DispatchAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static boolean enter( + @Advice.This final AsyncContext context, @Advice.AllArguments final Object[] args) { + final int depth = CallDepthThreadLocalMap.incrementCallDepth(AsyncContext.class); + if (depth > 0) { + return false; + } + + final ServletRequest request = context.getRequest(); + final Object spanAttr = request.getAttribute(SERVLET_SPAN); + if (spanAttr instanceof Span) { + request.removeAttribute(SERVLET_SPAN); + final Span span = (Span) spanAttr; + // Override propagation headers by injecting attributes with new values. + if (request instanceof HttpServletRequest) { + GlobalTracer.get() + .inject( + span.context(), + Format.Builtin.TEXT_MAP, + new HttpServletRequestInjectAdapter((HttpServletRequest) request)); + } + final String path; + if (args.length == 1 && args[0] instanceof String) { + path = (String) args[0]; + } else if (args.length == 2 && args[1] instanceof String) { + path = (String) args[1]; + } else { + path = "true"; + } + span.setTag("servlet.dispatch", path); + span.finish(); + } + return true; + } + + @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class) + public static void exit(@Advice.Enter final boolean topLevel) { + if (topLevel) { + CallDepthThreadLocalMap.reset(AsyncContext.class); + } + } + } +} diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java index cf22f3f3981..85009e75a30 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java @@ -15,7 +15,19 @@ import net.bytebuddy.matcher.ElementMatcher; @AutoService(Instrumenter.class) -public final class FilterChain3Instrumentation extends AbstractServlet3Instrumentation { +public final class FilterChain3Instrumentation extends Instrumenter.Default { + public FilterChain3Instrumentation() { + super("servlet", "servlet-3"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter", + "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", + "datadog.trace.instrumentation.servlet3.TagSettingAsyncListener" + }; + } @Override public ElementMatcher typeMatcher() { diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java index fc4afda4162..e653a88bde0 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java @@ -15,7 +15,19 @@ import net.bytebuddy.matcher.ElementMatcher; @AutoService(Instrumenter.class) -public final class HttpServlet3Instrumentation extends AbstractServlet3Instrumentation { +public final class HttpServlet3Instrumentation extends Instrumenter.Default { + public HttpServlet3Instrumentation() { + super("servlet", "servlet-3"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter", + "datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", + "datadog.trace.instrumentation.servlet3.TagSettingAsyncListener" + }; + } @Override public ElementMatcher typeMatcher() { diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java index 0ecb00f9c07..e50ad6f6939 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java @@ -3,6 +3,7 @@ import io.opentracing.propagation.TextMap; import java.util.AbstractMap; import java.util.ArrayList; +import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; import java.util.Iterator; @@ -52,6 +53,20 @@ protected Map> servletHeadersToMultiMap( headersResult.put(headerName, valuesList); } + /* + * Read from the attributes and override the headers. + * This is used by HttpServletRequestInjectAdapter when a request is async-dispatched. + */ + final Enumeration attributeNamesIt = httpServletRequest.getAttributeNames(); + while (attributeNamesIt.hasMoreElements()) { + final String attributeName = attributeNamesIt.nextElement(); + + final Object valuesIt = httpServletRequest.getAttribute(attributeName); + if (valuesIt instanceof String) { + headersResult.put(attributeName, Collections.singletonList((String) valuesIt)); + } + } + return headersResult; } diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java new file mode 100644 index 00000000000..967243bbf26 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java @@ -0,0 +1,27 @@ +package datadog.trace.instrumentation.servlet3; + +import io.opentracing.propagation.TextMap; +import java.util.Iterator; +import java.util.Map; +import javax.servlet.http.HttpServletRequest; + +/** Inject into request attributes since the request headers can't be modified. */ +public class HttpServletRequestInjectAdapter implements TextMap { + + private final HttpServletRequest httpServletRequest; + + public HttpServletRequestInjectAdapter(final HttpServletRequest httpServletRequest) { + this.httpServletRequest = httpServletRequest; + } + + @Override + public Iterator> iterator() { + throw new UnsupportedOperationException( + "This class should be used only with Tracer.extract()!"); + } + + @Override + public void put(final String key, final String value) { + httpServletRequest.setAttribute(key, value); + } +} 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 06176194540..16dd74c6c93 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 @@ -21,11 +21,13 @@ 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) { - if (GlobalTracer.get().activeSpan() != null || !(req instanceof HttpServletRequest)) { + 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; } @@ -53,6 +55,8 @@ public static Scope startSpan( if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(true); } + + req.setAttribute(SERVLET_SPAN, scope.span()); return scope; } @@ -63,13 +67,11 @@ public static void stopSpan( @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { // Set user.principal regardless of who created this span. - final Span currentSpan = GlobalTracer.get().activeSpan(); - if (currentSpan != null) { - if (request instanceof HttpServletRequest) { - final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); - if (principal != null) { - currentSpan.setTag(DDTags.USER_NAME, principal.getName()); - } + final Object spanAttr = request.getAttribute(SERVLET_SPAN); + if (spanAttr instanceof Span && request instanceof HttpServletRequest) { + final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); + if (principal != null) { + ((Span) spanAttr).setTag(DDTags.USER_NAME, principal.getName()); } } @@ -90,19 +92,23 @@ public static void stopSpan( ((TraceScope) scope).setAsyncPropagation(false); } scope.close(); + req.removeAttribute(SERVLET_SPAN); span.finish(); // Finish the span manually since finishSpanOnClose was false - } else if (req.isAsyncStarted()) { - final AtomicBoolean activated = new AtomicBoolean(false); - // what if async is already finished? This would not be called - req.getAsyncContext().addListener(new TagSettingAsyncListener(activated, span)); - scope.close(); } else { - Tags.HTTP_STATUS.set(span, resp.getStatus()); - if (scope instanceof TraceScope) { - ((TraceScope) scope).setAsyncPropagation(false); + final AtomicBoolean activated = new AtomicBoolean(false); + if (req.isAsyncStarted()) { + req.getAsyncContext().addListener(new TagSettingAsyncListener(activated, span)); + } + // Check again in case the request finished before adding the listener. + if (!req.isAsyncStarted() && activated.compareAndSet(false, true)) { + Tags.HTTP_STATUS.set(span, resp.getStatus()); + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(false); + } + req.removeAttribute(SERVLET_SPAN); + span.finish(); // Finish the span manually since finishSpanOnClose was false } scope.close(); - span.finish(); // Finish the span manually since finishSpanOnClose was false } } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java index e99645e3872..08b5a39d754 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java @@ -10,6 +10,7 @@ import java.io.IOException; import java.util.Collections; import java.util.concurrent.atomic.AtomicBoolean; +import javax.servlet.AsyncContext; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; import javax.servlet.http.HttpServletResponse; @@ -72,6 +73,12 @@ public void onError(final AsyncEvent event) throws IOException { } } + /** Re-attach listener for dispatch. */ @Override - public void onStartAsync(final AsyncEvent event) throws IOException {} + public void onStartAsync(final AsyncEvent event) { + final AsyncContext eventAsyncContext = event.getAsyncContext(); + if (eventAsyncContext != null) { + eventAsyncContext.addListener(this, event.getSuppliedRequest(), event.getSuppliedResponse()); + } + } } 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 new file mode 100644 index 00000000000..b9ba46e14f9 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/AbstractServlet3Test.groovy @@ -0,0 +1,370 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.TestUtils +import datadog.trace.agent.test.utils.OkHttpUtils +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.apache.catalina.core.ApplicationFilterChain +import spock.lang.Shared +import spock.lang.Unroll + +import javax.servlet.Servlet + +// Need to be explicit to unroll inherited tests: +@Unroll +abstract class AbstractServlet3Test extends AgentTestRunner { + + @Shared + 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() + + @Shared + int port = TestUtils.randomOpenPort() + @Shared + protected String user = "user" + @Shared + protected String pass = "password" + + abstract String getContext() + + abstract void addServlet(CONTEXT context, String url, Class servlet) + + protected void setupServlets(CONTEXT context) { + addServlet(context, "/sync", TestServlet3.Sync) + addServlet(context, "/auth/sync", TestServlet3.Sync) + addServlet(context, "/async", TestServlet3.Async) + addServlet(context, "/auth/async", TestServlet3.Async) + addServlet(context, "/blocking", TestServlet3.BlockingAsync) + addServlet(context, "/dispatch/sync", TestServlet3.DispatchSync) + addServlet(context, "/dispatch/async", TestServlet3.DispatchAsync) + addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive) + addServlet(context, "/recursive", TestServlet3.DispatchRecursive) + addServlet(context, "/fake", TestServlet3.FakeAsync) + } + + def "test #path servlet call (auth: #auth, distributed tracing: #distributedTracing)"() { + setup: + def requestBuilder = new Request.Builder() + .url("http://localhost:$port/$context/$path") + .get() + if (distributedTracing) { + requestBuilder.header("x-datadog-trace-id", "123") + requestBuilder.header("x-datadog-parent-id", "456") + } + if (auth) { + requestBuilder.header("Authorization", Credentials.basic(user, pass)) + } + 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 context + operationName "servlet.request" + resourceName "GET /$context/$path" + spanType DDSpanTypes.WEB_SERVLET + errored false + tags { + "http.url" "http://localhost:$port/$context/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.origin.type" { it == "TestServlet3\$$origin" || it == ApplicationFilterChain.name } + "span.type" DDSpanTypes.WEB_SERVLET + "servlet.context" "/$context" + "http.status_code" 200 + if (auth) { + "$DDTags.USER_NAME" user + } + defaultTags(distributedTracing) + } + } + } + } + + where: + path | expectedResponse | auth | origin | distributedTracing + "async" | "Hello Async" | false | "Async" | false + "sync" | "Hello Sync" | false | "Sync" | false + "auth/async" | "Hello Async" | true | "Async" | false + "auth/sync" | "Hello Sync" | true | "Sync" | false + "blocking" | "Hello BlockingAsync" | false | "BlockingAsync" | false + "fake" | "Hello FakeAsync" | false | "FakeAsync" | false + "async" | "Hello Async" | false | "Async" | true + "sync" | "Hello Sync" | false | "Sync" | true + "auth/async" | "Hello Async" | true | "Async" | true + "auth/sync" | "Hello Sync" | true | "Sync" | true + "blocking" | "Hello BlockingAsync" | false | "BlockingAsync" | true + "fake" | "Hello FakeAsync" | false | "FakeAsync" | true + } + + def "test dispatch #path with depth #depth"() { + setup: + def requestBuilder = new Request.Builder() + .url("http://localhost:$port/$context/dispatch/$path?depth=$depth") + .get() + if (distributedTracing) { + requestBuilder.header("x-datadog-trace-id", "123") + requestBuilder.header("x-datadog-parent-id", "456") + } + def response = client.newCall(requestBuilder.build()).execute() + + expect: + response.body().string().trim() == "Hello $origin" + + assertTraces(2 + depth) { + for (int i = 0; i < depth; i++) { + trace(i, 1) { + span(0) { + if (i == 0) { + if (distributedTracing) { + traceId "123" + parentId "456" + } else { + parent() + } + } else { + childOf TEST_WRITER[i - 1][0] + } + serviceName context + operationName "servlet.request" + resourceName "GET /$context/dispatch/$path" + spanType DDSpanTypes.WEB_SERVLET + errored false + tags { + "http.url" "http://localhost:$port/$context/dispatch/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.origin.type" { it == "TestServlet3\$Dispatch$origin" || it == ApplicationFilterChain.name } + "span.type" DDSpanTypes.WEB_SERVLET + "http.status_code" 200 + "servlet.context" "/$context" + "servlet.dispatch" "/dispatch/recursive?depth=${depth - i - 1}" + defaultTags(i > 0 ? true : distributedTracing) + } + } + } + } + trace(depth, 1) { + span(0) { + if (depth > 0) { + childOf TEST_WRITER[depth - 1][0] + } else { + if (distributedTracing) { + traceId "123" + parentId "456" + } else { + parent() + } + } + serviceName context + operationName "servlet.request" + resourceName "GET /$context/dispatch/$path" + spanType DDSpanTypes.WEB_SERVLET + errored false + tags { + "http.url" "http://localhost:$port/$context/dispatch/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.origin.type" { it == "TestServlet3\$Dispatch$origin" || it == ApplicationFilterChain.name } + "span.type" DDSpanTypes.WEB_SERVLET + "http.status_code" 200 + "servlet.context" "/$context" + "servlet.dispatch" "/$path" + defaultTags(depth > 0 ? true : distributedTracing) + } + } + } + trace(depth + 1, 1) { + span(0) { + serviceName context + operationName "servlet.request" + resourceName "GET /$context/$path" + spanType DDSpanTypes.WEB_SERVLET + errored false + childOf TEST_WRITER[depth][0] + tags { + "http.url" "http://localhost:$port/$context/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.origin.type" { + it == "TestServlet3\$$origin" || it == "TestServlet3\$DispatchRecursive" || it == ApplicationFilterChain.name + } + "span.type" DDSpanTypes.WEB_SERVLET + "http.status_code" 200 + "servlet.context" "/$context" + defaultTags(true) + } + } + } + } + + where: + path | distributedTracing | depth + "sync" | true | 0 + "sync" | false | 0 + "async" | true | 0 + "async" | false | 0 + "recursive" | true | 0 + "recursive" | false | 0 + "recursive" | true | 1 + "recursive" | false | 1 + "recursive" | true | 20 + "recursive" | false | 20 + + origin = path.capitalize() + } + + def "servlet instrumentation clears state after async request"() { + setup: + def request = new Request.Builder() + .url("http://localhost:$port/$context/$path") + .get() + .build() + def numTraces = 1 + for (int i = 0; i < numTraces; ++i) { + client.newCall(request).execute() + } + + expect: + assertTraces(dispatched ? numTraces * 2 : numTraces) { + for (int i = 0; (dispatched ? i + 1 : i) < TEST_WRITER.size(); i += (dispatched ? 2 : 1)) { + if (dispatched) { + trace(i, 1) { + span(0) { + operationName "servlet.request" + resourceName "GET /$context/dispatch/async" + parent() + } + } + } + trace(dispatched ? i + 1 : i, 1) { + span(0) { + operationName "servlet.request" + resourceName "GET /$context/async" + if (dispatched) { + childOf TEST_WRITER[i][0] + } else { + parent() + } + } + } + } + } + + where: + path | dispatched + "async" | false + "dispatch/async" | true + } + + def "test #path error servlet call"() { + setup: + def request = new Request.Builder() + .url("http://localhost:$port/$context/$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 context + operationName "servlet.request" + resourceName "GET /$context/$path" + spanType DDSpanTypes.WEB_SERVLET + errored true + parent() + tags { + "http.url" "http://localhost:$port/$context/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.type" DDSpanTypes.WEB_SERVLET + "span.origin.type" { it == "TestServlet3\$$origin" || it == ApplicationFilterChain.name } + "servlet.context" "/$context" + "http.status_code" 500 + errorTags(RuntimeException, "some $path error") + defaultTags() + } + } + } + } + + where: + path | expectedResponse + //"async" | "Hello Async" // FIXME: I can't seem get the async error handler to trigger + "sync" | "Hello Sync" + + origin = path.capitalize() + } + + def "test #path non-throwing-error servlet call"() { + setup: + def request = new Request.Builder() + .url("http://localhost:$port/$context/$path?non-throwing-error=true") + .get() + .build() + def response = client.newCall(request).execute() + + expect: + response.body().string().trim() != expectedResponse + + assertTraces(1) { + trace(0, 1) { + span(0) { + serviceName context + operationName "servlet.request" + resourceName "GET /$context/$path" + spanType DDSpanTypes.WEB_SERVLET + errored true + parent() + tags { + "http.url" "http://localhost:$port/$context/$path" + "http.method" "GET" + "span.kind" "server" + "component" "java-web-servlet" + "span.type" DDSpanTypes.WEB_SERVLET + "span.origin.type" { it == "TestServlet3\$$origin" || it == ApplicationFilterChain.name } + "servlet.context" "/$context" + "http.status_code" 500 + "error" true + defaultTags() + } + } + } + } + + where: + path | expectedResponse + "sync" | "Hello Sync" + + origin = path.capitalize() + } +} 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 052c96c4512..edaae29fd2d 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 @@ -1,13 +1,4 @@ -import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.TestUtils -import datadog.trace.agent.test.utils.OkHttpUtils -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.security.ConstraintMapping import org.eclipse.jetty.security.ConstraintSecurityHandler import org.eclipse.jetty.security.HashLoginService @@ -18,39 +9,22 @@ import org.eclipse.jetty.servlet.ServletContextHandler import org.eclipse.jetty.util.security.Constraint import spock.lang.Shared -class JettyServlet3Test extends AgentTestRunner { +import javax.servlet.Servlet - 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 JettyServlet3Test extends AbstractServlet3Test { - @Shared - int port @Shared private Server jettyServer - @Shared - private ServletContextHandler servletContext def setupSpec() { port = TestUtils.randomOpenPort() jettyServer = new Server(port) - servletContext = new ServletContextHandler() - - ConstraintSecurityHandler security = setupAuthentication(jettyServer) - - servletContext.setSecurityHandler(security) - servletContext.addServlet(TestServlet3.Sync, "/sync") - servletContext.addServlet(TestServlet3.Sync, "/auth/sync") - servletContext.addServlet(TestServlet3.Async, "/async") - servletContext.addServlet(TestServlet3.Async, "/auth/async") + ServletContextHandler servletContext = new ServletContextHandler(null, "/$context") + setupAuthentication(jettyServer, servletContext) + setupServlets(servletContext) jettyServer.setHandler(servletContext) + jettyServer.start() System.out.println( @@ -62,184 +36,18 @@ class JettyServlet3Test extends AgentTestRunner { jettyServer.destroy() } - def "test #path servlet call (auth: #auth, distributed tracing: #distributedTracing)"() { - setup: - def requestBuilder = new Request.Builder() - .url("http://localhost:$port/$path") - .get() - if (distributedTracing) { - requestBuilder.header("x-datadog-trace-id", "123") - requestBuilder.header("x-datadog-parent-id", "456") - } - if (auth) { - requestBuilder.header("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 "unnamed-java-app" - operationName "servlet.request" - resourceName "GET /$path" - spanType DDSpanTypes.WEB_SERVLET - errored false - tags { - "http.url" "http://localhost:$port/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.origin.type" "TestServlet3\$$origin" - "span.type" DDSpanTypes.WEB_SERVLET - "http.status_code" 200 - if (auth) { - "$DDTags.USER_NAME" "user" - } - defaultTags(distributedTracing) - } - } - } - } - - where: - path | expectedResponse | auth | origin | distributedTracing - "async" | "Hello Async" | false | "Async" | false - "sync" | "Hello Sync" | false | "Sync" | false - "auth/async" | "Hello Async" | true | "Async" | false - "auth/sync" | "Hello Sync" | true | "Sync" | false - "async" | "Hello Async" | false | "Async" | true - "sync" | "Hello Sync" | false | "Sync" | true - "auth/async" | "Hello Async" | true | "Async" | true - "auth/sync" | "Hello Sync" | true | "Sync" | true + @Override + String getContext() { + return "jetty-context" } - def "servlet instrumentation clears state after async request"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/async") - .get() - .build() - def numTraces = 5 - for (int i = 0; i < numTraces; ++i) { - client.newCall(request).execute() - } - - expect: - assertTraces(numTraces) { - for (int i = 0; i < numTraces; ++i) { - trace(i, 1) { - span(0) { - serviceName "unnamed-java-app" - operationName "servlet.request" - resourceName "GET /async" - } - } - } - } - } - - def "test #path error servlet call"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/$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 "unnamed-java-app" - operationName "servlet.request" - resourceName "GET /$path" - spanType DDSpanTypes.WEB_SERVLET - errored true - parent() - tags { - "http.url" "http://localhost:$port/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.type" DDSpanTypes.WEB_SERVLET - "span.origin.type" "TestServlet3\$Sync" - "http.status_code" 500 - errorTags(RuntimeException, "some $path error") - defaultTags() - } - } - } - } - - where: - path | expectedResponse - //"async" | "Hello Async" // FIXME: I can't seem get the async error handler to trigger - "sync" | "Hello Sync" - } - - def "test #path non-throwing-error servlet call"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/$path?non-throwing-error=true") - .get() - .build() - def response = client.newCall(request).execute() - - expect: - response.body().string().trim() != expectedResponse - - assertTraces(1) { - trace(0, 1) { - span(0) { - serviceName "unnamed-java-app" - operationName "servlet.request" - resourceName "GET /$path" - spanType DDSpanTypes.WEB_SERVLET - errored true - parent() - tags { - "http.url" "http://localhost:$port/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.type" DDSpanTypes.WEB_SERVLET - "span.origin.type" "TestServlet3\$Sync" - "http.status_code" 500 - "error" true - defaultTags() - } - } - } - } - - where: - path | expectedResponse - "sync" | "Hello Sync" + @Override + void addServlet(ServletContextHandler servletContext, String url, Class servlet) { + servletContext.addServlet(servlet, url) } - /** - * Setup simple authentication for tests - *

- * requests to {@code /auth/*} need login 'user' and password 'password' - *

- * For details @see http://www.eclipse.org/jetty/documentation/9.3.x/embedded-examples.html - * - * @param jettyServer server to attach login service - * @return SecurityHandler that can be assigned to servlet - */ - private ConstraintSecurityHandler setupAuthentication(Server jettyServer) { - ConstraintSecurityHandler security = new ConstraintSecurityHandler() + static setupAuthentication(Server jettyServer, ServletContextHandler servletContext) { + ConstraintSecurityHandler authConfig = new ConstraintSecurityHandler() Constraint constraint = new Constraint() constraint.setName("auth") @@ -250,14 +58,14 @@ class JettyServlet3Test extends AgentTestRunner { mapping.setPathSpec("/auth/*") mapping.setConstraint(constraint) - security.setConstraintMappings(mapping) - security.setAuthenticator(new BasicAuthenticator()) + authConfig.setConstraintMappings(mapping) + authConfig.setAuthenticator(new BasicAuthenticator()) LoginService loginService = new HashLoginService("TestRealm", "src/test/resources/realm.properties") - security.setLoginService(loginService) + authConfig.setLoginService(loginService) jettyServer.addBean(loginService) - security + servletContext.setSecurityHandler(authConfig) } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet3.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet3.groovy index 27b5ebb2ed5..d79060eb5f5 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet3.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet3.groovy @@ -3,6 +3,7 @@ import groovy.servlet.AbstractHttpServlet import javax.servlet.annotation.WebServlet import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +import java.util.concurrent.CountDownLatch class TestServlet3 { @@ -25,11 +26,76 @@ class TestServlet3 { static class Async extends AbstractHttpServlet { @Override void doGet(HttpServletRequest req, HttpServletResponse resp) { + def latch = new CountDownLatch(1) def context = req.startAsync() context.start { + latch.await() resp.writer.print("Hello Async") context.complete() } + latch.countDown() } } + + @WebServlet(asyncSupported = true) + static class BlockingAsync extends AbstractHttpServlet { + @Override + void doGet(HttpServletRequest req, HttpServletResponse resp) { + def latch = new CountDownLatch(1) + def context = req.startAsync() + context.start { + resp.writer.print("Hello BlockingAsync") + context.complete() + latch.countDown() + } + latch.await() + } + } + + @WebServlet(asyncSupported = true) + static class DispatchSync extends AbstractHttpServlet { + @Override + void doGet(HttpServletRequest req, HttpServletResponse resp) { + req.startAsync().dispatch("/sync") + } + } + + @WebServlet(asyncSupported = true) + static class DispatchAsync extends AbstractHttpServlet { + @Override + void doGet(HttpServletRequest req, HttpServletResponse resp) { + def context = req.startAsync() + context.start { + context.dispatch("/async") + } + } + } + + @WebServlet(asyncSupported = true) + static class DispatchRecursive extends AbstractHttpServlet { + @Override + void doGet(HttpServletRequest req, HttpServletResponse resp) { + if (req.servletPath.equals("/recursive")) { + resp.writer.print("Hello Recursive") + return + } + def depth = Integer.parseInt(req.getParameter("depth")) + if (depth > 0) { + req.startAsync().dispatch("/dispatch/recursive?depth=" + (depth - 1)) + } else { + req.startAsync().dispatch("/recursive") + } + } + } + + @WebServlet(asyncSupported = true) + static class FakeAsync extends AbstractHttpServlet { + @Override + void doGet(HttpServletRequest req, HttpServletResponse resp) { + def context = req.startAsync() + resp.writer.print("Hello FakeAsync") + context.complete() + } + } + } diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy index d823067a110..5ba55cb2249 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy @@ -1,56 +1,49 @@ import com.google.common.io.Files -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.agent.test.TestUtils -import datadog.trace.agent.test.utils.OkHttpUtils -import datadog.trace.api.DDSpanTypes -import okhttp3.OkHttpClient -import okhttp3.Request import org.apache.catalina.Context -import org.apache.catalina.core.ApplicationFilterChain +import org.apache.catalina.LifecycleState +import org.apache.catalina.realm.MemoryRealm +import org.apache.catalina.realm.MessageDigestCredentialHandler import org.apache.catalina.startup.Tomcat import org.apache.tomcat.JarScanFilter import org.apache.tomcat.JarScanType +import org.apache.tomcat.util.descriptor.web.LoginConfig +import org.apache.tomcat.util.descriptor.web.SecurityCollection +import org.apache.tomcat.util.descriptor.web.SecurityConstraint import spock.lang.Shared -class TomcatServlet3Test extends AgentTestRunner { +import javax.servlet.Servlet - OkHttpClient client = OkHttpUtils.client() +class TomcatServlet3Test extends AbstractServlet3Test { - @Shared - int port @Shared Tomcat tomcatServer @Shared - Context appContext + def baseDir = Files.createTempDir() def setupSpec() { - port = TestUtils.randomOpenPort() tomcatServer = new Tomcat() tomcatServer.setPort(port) tomcatServer.getConnector() - def baseDir = Files.createTempDir() baseDir.deleteOnExit() tomcatServer.setBaseDir(baseDir.getAbsolutePath()) final File applicationDir = new File(baseDir, "/webapps/ROOT") if (!applicationDir.exists()) { applicationDir.mkdirs() + applicationDir.deleteOnExit() } - appContext = tomcatServer.addWebapp("/my-context", applicationDir.getAbsolutePath()) + Context servletContext = tomcatServer.addWebapp("/$context", applicationDir.getAbsolutePath()) // Speed up startup by disabling jar scanning: - appContext.getJarScanner().setJarScanFilter(new JarScanFilter() { + servletContext.getJarScanner().setJarScanFilter(new JarScanFilter() { @Override boolean check(JarScanType jarScanType, String jarName) { return false } }) - Tomcat.addServlet(appContext, "syncServlet", new TestServlet3.Sync()) - appContext.addServletMappingDecoded("/sync", "syncServlet") - - Tomcat.addServlet(appContext, "asyncServlet", new TestServlet3.Async()) - appContext.addServletMappingDecoded("/async", "asyncServlet") + setupAuthentication(tomcatServer, servletContext) + setupServlets(servletContext) tomcatServer.start() System.out.println( @@ -62,137 +55,47 @@ class TomcatServlet3Test extends AgentTestRunner { tomcatServer.destroy() } - def "test #path servlet call (distributed tracing: #distributedTracing)"() { - setup: - def requestBuilder = new Request.Builder() - .url("http://localhost:$port/my-context/$path") - .get() - if (distributedTracing) { - requestBuilder.header("x-datadog-trace-id", "123") - requestBuilder.header("x-datadog-parent-id", "456") - } - 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 "my-context" - operationName "servlet.request" - resourceName "GET /my-context/$path" - spanType DDSpanTypes.WEB_SERVLET - errored false - tags { - "http.url" "http://localhost:$port/my-context/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.type" DDSpanTypes.WEB_SERVLET - "span.origin.type" ApplicationFilterChain.name - "servlet.context" "/my-context" - "http.status_code" 200 - defaultTags(distributedTracing) - } - } - } - } - - where: - path | expectedResponse | distributedTracing - "async" | "Hello Async" | false - "sync" | "Hello Sync" | false - "async" | "Hello Async" | true - "sync" | "Hello Sync" | true + @Override + String getContext() { + return "tomcat-context" } - def "test #path error servlet call"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/my-context/$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 "my-context" - operationName "servlet.request" - resourceName "GET /my-context/$path" - spanType DDSpanTypes.WEB_SERVLET - errored true - parent() - tags { - "http.url" "http://localhost:$port/my-context/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.type" DDSpanTypes.WEB_SERVLET - "span.origin.type" ApplicationFilterChain.name - "servlet.context" "/my-context" - "http.status_code" 500 - errorTags(RuntimeException, "some $path error") - defaultTags() - } - } - } - } - - where: - path | expectedResponse - //"async" | "Hello Async" // FIXME: I can't seem get the async error handler to trigger - "sync" | "Hello Sync" + @Override + void addServlet(Context servletContext, String url, Class servlet) { + String name = UUID.randomUUID() + Tomcat.addServlet(servletContext, name, servlet.newInstance()) + servletContext.addServletMappingDecoded(url, name) } - def "test #path error servlet call for non-throwing error"() { - setup: - def request = new Request.Builder() - .url("http://localhost:$port/my-context/$path?non-throwing-error=true") - .get() - .build() - def response = client.newCall(request).execute() - - expect: - response.body().string().trim() != expectedResponse - - assertTraces(1) { - trace(0, 1) { - span(0) { - serviceName "my-context" - operationName "servlet.request" - resourceName "GET /my-context/$path" - spanType DDSpanTypes.WEB_SERVLET - errored true - parent() - tags { - "http.url" "http://localhost:$port/my-context/$path" - "http.method" "GET" - "span.kind" "server" - "component" "java-web-servlet" - "span.type" DDSpanTypes.WEB_SERVLET - "span.origin.type" ApplicationFilterChain.name - "servlet.context" "/my-context" - "http.status_code" 500 - "error" true - defaultTags() - } - } + private setupAuthentication(Tomcat server, Context servletContext) { + // Login Config + LoginConfig authConfig = new LoginConfig() + authConfig.setAuthMethod("BASIC") + + // adding constraint with role "test" + SecurityConstraint constraint = new SecurityConstraint() + constraint.addAuthRole("role") + + // add constraint to a collection with pattern /second + SecurityCollection collection = new SecurityCollection() + collection.addPattern("/auth/*") + constraint.addCollection(collection) + + servletContext.setLoginConfig(authConfig) + // does the context need a auth role too? + servletContext.addSecurityRole("role") + servletContext.addConstraint(constraint) + + // add tomcat users to realm + MemoryRealm realm = new MemoryRealm() { + protected void startInternal() { + credentialHandler = new MessageDigestCredentialHandler() + setState(LifecycleState.STARTING) } } + realm.addUser(user, pass, "role") + server.getEngine().setRealm(realm) - where: - path | expectedResponse - "sync" | "Hello Sync" + servletContext.setLoginConfig(authConfig) } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/propagation/HTTPCodec.java b/dd-trace-ot/src/main/java/datadog/opentracing/propagation/HTTPCodec.java index 3d5158a85e0..7dd0676241d 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/propagation/HTTPCodec.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/propagation/HTTPCodec.java @@ -62,6 +62,10 @@ public SpanContext extract(final TextMap carrier) { final String key = entry.getKey().toLowerCase(); final String val = entry.getValue(); + if (val == null) { + continue; + } + if (TRACE_ID_KEY.equalsIgnoreCase(key)) { traceId = validateUInt64BitsID(val); } else if (SPAN_ID_KEY.equalsIgnoreCase(key)) {