From 8c490a42a208f3f36eb5b7074201c62cd93e1db3 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 9 Aug 2019 16:22:32 -0700 Subject: [PATCH 1/3] Improve Ratpack context propagation and migrate tests --- .../HttpServerRequestTracingHandler.java | 3 + .../src/test/groovy/Netty40ServerTest.groovy | 1 + .../HttpServerRequestTracingHandler.java | 3 + .../src/test/groovy/Netty41ServerTest.groovy | 1 + .../ratpack-1.4/ratpack-1.4.gradle | 4 +- .../latestDepTest/groovy/RatpackTest.groovy | 695 ------------------ .../ratpack/ContinuationInstrumentation.java | 97 +++ .../DefaultExecutionInstrumentation.java | 107 +++ .../ratpack/ExecStreamInstrumentation.java | 108 --- .../ServerErrorHandlerInstrumentation.java | 24 +- .../ratpack/ErrorHandlerAdvice.java | 19 + .../ratpack/RatpackServerDecorator.java | 9 + .../src/test/groovy/RatpackOtherTest.groovy | 175 +++++ .../src/test/groovy/RatpackTest.groovy | 694 ----------------- .../client/RatpackForkedHttpClientTest.groovy | 26 + .../client/RatpackHttpClientTest.groovy | 58 ++ .../NettyServerTestInstrumentation.java | 22 + .../server/RatpackAsyncHttpServerTest.groovy | 75 ++ .../server/RatpackForkedHttpServerTest.groovy | 75 ++ .../server/RatpackHttpServerTest.groovy | 145 ++++ .../agent/test/base/HttpServerTest.groovy | 89 ++- 21 files changed, 898 insertions(+), 1532 deletions(-) delete mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/latestDepTest/groovy/RatpackTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java delete mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ExecStreamInstrumentation.java create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ErrorHandlerAdvice.java create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/RatpackOtherTest.groovy delete mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/RatpackTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackForkedHttpClientTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackHttpClientTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/NettyServerTestInstrumentation.java create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackAsyncHttpServerTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy diff --git a/dd-java-agent/instrumentation/netty-4.0/src/main/java/datadog/trace/instrumentation/netty40/server/HttpServerRequestTracingHandler.java b/dd-java-agent/instrumentation/netty-4.0/src/main/java/datadog/trace/instrumentation/netty40/server/HttpServerRequestTracingHandler.java index 72019f524a..6313cb1e6f 100644 --- a/dd-java-agent/instrumentation/netty-4.0/src/main/java/datadog/trace/instrumentation/netty40/server/HttpServerRequestTracingHandler.java +++ b/dd-java-agent/instrumentation/netty-4.0/src/main/java/datadog/trace/instrumentation/netty40/server/HttpServerRequestTracingHandler.java @@ -26,6 +26,9 @@ public class HttpServerRequestTracingHandler extends ChannelInboundHandlerAdapte ctx.fireChannelRead(msg); // superclass does not throw } else { try (final Scope scope = tracer.scopeManager().activate(span, false)) { + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(true); + } ctx.fireChannelRead(msg); // superclass does not throw } } 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 801cf2cbe5..601d07580f 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 @@ -103,6 +103,7 @@ class Netty40ServerTest extends HttpServerTest - // 1st internal http client call to nested - httpClient.get(HttpUrlBuilder.base(external.address).path("nested").build()) - .map { it.body.text } - .flatMap { t -> - // make a 2nd http request and concatenate the two bodies together - httpClient.get(HttpUrlBuilder.base(external.address).path("nested2").build()) map { t + it.body.text } - } - .then { - context.render(it) - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 200 - resp.body().string() == "success" - - // 3rd is the three traces, ratpack, http client 2 and http client 1 - // 2nd is nested2 from the external server (the result of the 2nd internal http client call) - // 1st is nested from the external server (the result of the 1st internal http client call) - assertTraces(3) { - distributedRequestTrace(it, 0, trace(2).get(3)) - distributedRequestTrace(it, 1, trace(2).get(2)) - - trace(2, 4) { - // main app span that processed the request from OKHTTP request - span(0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored false - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - span(1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored false - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - // Second http client call that receives the 'ess' of Success - span(2) { - resourceName "GET /?" - serviceName "unnamed-java-app" - operationName "netty.client.request" - spanType DDSpanTypes.HTTP_CLIENT - childOf(span(1)) - errored false - tags { - "$Tags.COMPONENT.key" "netty-client" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${external.address}/nested2" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - // First http client call that receives the 'Succ' of Success - span(3) { - resourceName "GET /nested" - serviceName "unnamed-java-app" - operationName "netty.client.request" - spanType DDSpanTypes.HTTP_CLIENT - childOf(span(1)) - errored false - tags { - "$Tags.COMPONENT.key" "netty-client" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${external.address}/nested" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - } - } - } - - def "test ratpack http client error handling"() { - setup: - def badAddress = new URI("http://localhost:$UNUSABLE_PORT") - - def app = GroovyEmbeddedApp.ratpack { - handlers { - get { HttpClient httpClient -> - httpClient.get(badAddress) - .map { it.body.text } - .then { - context.render(it) - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 500 - - assertTraces(1) { - trace(0, 3) { - span(0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored true - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - "$Tags.ERROR.key" true - defaultTags() - } - } - span(1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored true - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - "$Tags.ERROR.key" true - defaultTags() - } - } - span(2) { - operationName "netty.connect" - resourceName "netty.connect" - childOf(span(1)) - errored true - tags { - "$Tags.COMPONENT.key" "netty" - errorTags(AbstractChannel.AnnotatedConnectException, String) - defaultTags() - } - } - } - } - } - - def "test forked path call and start span in handler (#startSpanInHandler)"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - get { - TraceScope scope - if (startSpanInHandler) { - Span childSpan = GlobalTracer.get() - .buildSpan("ratpack.exec-test") - .withTag(DDTags.RESOURCE_NAME, "INSIDE-TEST") - .start() - scope = GlobalTracer.get().scopeManager().activate(childSpan, true) - } - def latch = new CountDownLatch(1) - try { - scope?.setAsyncPropagation(true) - GlobalTracer.get().activeSpan().setBaggageItem("test-baggage", "foo") - - context.render(testPromise(latch).fork()) - } finally { - scope?.close() - latch.countDown() - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 200 - resp.body().string() == "foo" - - assertTraces(1) { - trace(0, (startSpanInHandler ? 3 : 2)) { - if (startSpanInHandler) { - span(0) { - resourceName "INSIDE-TEST" - serviceName "unnamed-java-app" - operationName "ratpack.exec-test" - childOf(span(2)) - errored false - tags { - defaultTags() - } - } - } - span(startSpanInHandler ? 1 : 0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored false - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - span(startSpanInHandler ? 2 : 1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(startSpanInHandler ? 1 : 0)) - errored false - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - } - } - - where: - startSpanInHandler << [true, false] - } - - def "forked executions inherit parent scope"() { - when: - def result = ExecHarness.yieldSingle({}, { - final Scope scope = - GlobalTracer.get() - .buildSpan("ratpack.exec-test") - .withTag(DDTags.RESOURCE_NAME, "INSIDE-TEST") - .startActive(true) - - ((TraceScope) scope).setAsyncPropagation(true) - scope.span().setBaggageItem("test-baggage", "foo") - ParallelBatch.of(testPromise(), testPromise()) - .yield() - .map({ now -> - // close the scope now that we got the baggage inside the promises - scope.close() - return now - }) - }) - - then: - result.valueOrThrow == ["foo", "foo"] - assertTraces(1) { - trace(0, 1) { - span(0) { - resourceName "INSIDE-TEST" - serviceName "unnamed-java-app" - operationName "ratpack.exec-test" - parent() - errored false - tags { - defaultTags() - } - } - } - } - } - - // returns a promise that contains the active scope's "test-baggage" baggage - Promise testPromise(CountDownLatch latch = null) { - Promise.sync { - latch?.await() - Scope tracerScope = GlobalTracer.get().scopeManager().active() - return tracerScope?.span()?.getBaggageItem("test-baggage") - } - } -} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java new file mode 100644 index 0000000000..3d2fd7ecd2 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java @@ -0,0 +1,97 @@ +package datadog.trace.instrumentation.ratpack; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.context.TraceScope; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.util.GlobalTracer; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import ratpack.func.Block; +import ratpack.path.PathBinding; + +@AutoService(Instrumenter.class) +public final class ContinuationInstrumentation extends Instrumenter.Default { + + public ContinuationInstrumentation() { + super("ratpack"); + } + + @Override + public ElementMatcher typeMatcher() { + return safeHasSuperType(named("ratpack.exec.internal.Continuation")); + } + + @Override + public String[] helperClassNames() { + return new String[] { + getClass().getName() + "$ResumeAdvice$BlockWrapper", + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + named("resume").and(takesArgument(0, named("ratpack.func.Block"))), + ResumeAdvice.class.getName()); + } + + public static class ResumeAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void wrap(@Advice.Argument(value = 0, readOnly = false) Block block) { + final Span span = GlobalTracer.get().activeSpan(); + if (span != null) { + block = BlockWrapper.wrapIfNeeded(block, span); + } + } + + public void muzzleCheck(final PathBinding binding) { + // This was added in 1.4. Added here to ensure consistency with other instrumentation. + binding.getDescription(); + } + + @Slf4j + public static class BlockWrapper implements Block { + private final Block delegate; + private final Span span; + + private BlockWrapper(final Block delegate, final Span span) { + this.delegate = delegate; + this.span = span; + } + + @Override + public void execute() throws Exception { + if (span != null) { + try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(true); + } + delegate.execute(); + } + } else { + delegate.execute(); + } + } + + public static Block wrapIfNeeded(final Block delegate, final Span span) { + if (delegate instanceof BlockWrapper || span == null) { + return delegate; + } + log.debug("Wrapping action task {}", delegate); + return new BlockWrapper(delegate, span); + } + } + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java new file mode 100644 index 0000000000..908719a51e --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java @@ -0,0 +1,107 @@ +package datadog.trace.instrumentation.ratpack; + +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.context.TraceScope; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.util.GlobalTracer; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import ratpack.exec.internal.Continuation; +import ratpack.func.Action; +import ratpack.path.PathBinding; + +@AutoService(Instrumenter.class) +public final class DefaultExecutionInstrumentation extends Instrumenter.Default { + + public DefaultExecutionInstrumentation() { + super("ratpack"); + } + + @Override + public ElementMatcher typeMatcher() { + return named("ratpack.exec.internal.DefaultExecution"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + getClass().getName() + "$DelimitAdvice$ActionWrapper", + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + nameStartsWith("delimit") // include delimitStream + .and(takesArgument(0, named("ratpack.func.Action"))) + .and(takesArgument(1, named("ratpack.func.Action"))), + DelimitAdvice.class.getName()); + } + + public static class DelimitAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void wrap( + @Advice.Argument(value = 0, readOnly = false) Action onError, + @Advice.Argument(value = 1, readOnly = false) Action segment) { + final Span span = GlobalTracer.get().activeSpan(); + if (span != null) { + /** + * Here we pass along the span instead of a continuation because we aren't sure it won't be + * used for both callbacks. + */ + onError = ActionWrapper.wrapIfNeeded(onError, span); + segment = ActionWrapper.wrapIfNeeded(segment, span); + } + } + + public void muzzleCheck(final PathBinding binding) { + // This was added in 1.4. Added here to ensure consistency with other instrumentation. + binding.getDescription(); + } + + @Slf4j + public static class ActionWrapper implements Action { + private final Action delegate; + private final Span span; + + private ActionWrapper(final Action delegate, final Span span) { + this.delegate = delegate; + this.span = span; + } + + @Override + public void execute(final T t) throws Exception { + if (span != null) { + try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(true); + } + delegate.execute(t); + } + } else { + delegate.execute(t); + } + } + + public static Action wrapIfNeeded(final Action delegate, final Span span) { + if (delegate instanceof ActionWrapper || span == null) { + return delegate; + } + log.debug("Wrapping action task {}", delegate); + return new ActionWrapper(delegate, span); + } + } + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ExecStreamInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ExecStreamInstrumentation.java deleted file mode 100644 index 4ee441be6a..0000000000 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ExecStreamInstrumentation.java +++ /dev/null @@ -1,108 +0,0 @@ -package datadog.trace.instrumentation.ratpack; - -import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static net.bytebuddy.matcher.ElementMatchers.isInterface; -import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.not; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; - -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.context.TraceScope; -import io.opentracing.Scope; -import io.opentracing.util.GlobalTracer; -import java.util.Collections; -import java.util.Map; -import lombok.extern.slf4j.Slf4j; -import net.bytebuddy.asm.Advice; -import net.bytebuddy.description.method.MethodDescription; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; -import ratpack.exec.internal.Continuation; -import ratpack.func.Action; -import ratpack.path.PathBinding; - -@AutoService(Instrumenter.class) -public final class ExecStreamInstrumentation extends Instrumenter.Default { - - public ExecStreamInstrumentation() { - super("ratpack"); - } - - @Override - public ElementMatcher typeMatcher() { - return not(isInterface()) - .and(safeHasSuperType(named("ratpack.exec.internal.DefaultExecution"))); - } - - // ratpack.exec.internal.DefaultExecution.delimit - @Override - public String[] helperClassNames() { - return new String[] { - packageName + ".ExecStreamInstrumentation$ActionWrapper", - }; - } - - @Override - public Map, String> transformers() { - return Collections.singletonMap( - named("delimit") - .or(named("delimitStream")) - .and(takesArgument(0, named("ratpack.func.Action"))) - .and(takesArgument(1, named("ratpack.func.Action"))), - WrapActionAdvice.class.getName()); - } - - public static class WrapActionAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void wrap( - @Advice.Argument(value = 0, readOnly = false) Action onError, - @Advice.Argument(value = 1, readOnly = false) Action segment) { - final Scope scope = GlobalTracer.get().scopeManager().active(); - if (scope instanceof TraceScope) { - final TraceScope.Continuation continuation = ((TraceScope) scope).capture(); - onError = ActionWrapper.wrapIfNeeded(onError, continuation); - segment = ActionWrapper.wrapIfNeeded(segment, continuation); - } - } - - public void muzzleCheck(final PathBinding binding) { - // This was added in 1.4. Added here to ensure consistency with other instrumentation. - binding.getDescription(); - } - } - - @Slf4j - public static class ActionWrapper implements Action { - private final Action delegate; - private final TraceScope.Continuation traceContinuation; - - private ActionWrapper( - final Action delegate, final TraceScope.Continuation traceContinuation) { - this.delegate = delegate; - this.traceContinuation = traceContinuation; - } - - @Override - public void execute(final T subject) throws Exception { - if (traceContinuation != null) { - try (final TraceScope scope = traceContinuation.activate()) { - scope.setAsyncPropagation(true); - delegate.execute(subject); - } - } else { - delegate.execute(subject); - } - } - - public static Action wrapIfNeeded( - final Action delegate, final TraceScope.Continuation traceContinuation) { - if (delegate instanceof ActionWrapper) { - return delegate; - } - log.debug("Wrapping action task {}", delegate); - return new ActionWrapper<>(delegate, traceContinuation); - } - } -} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ServerErrorHandlerInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ServerErrorHandlerInstrumentation.java index 62e6f5dc74..b3908dd848 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ServerErrorHandlerInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ServerErrorHandlerInstrumentation.java @@ -1,8 +1,8 @@ package datadog.trace.instrumentation.ratpack; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static datadog.trace.instrumentation.ratpack.RatpackServerDecorator.DECORATE; import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isAbstract; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; @@ -10,10 +10,7 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; -import io.opentracing.Span; -import io.opentracing.util.GlobalTracer; import java.util.Map; -import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -27,8 +24,8 @@ public class ServerErrorHandlerInstrumentation extends Instrumenter.Default { @Override public ElementMatcher typeMatcher() { - return named("ratpack.exec.Execution") - .or(not(isInterface()).and(safeHasSuperType(named("ratpack.error.ServerErrorHandler")))); + return not(isInterface().or(isAbstract())) + .and(safeHasSuperType(named("ratpack.error.ServerErrorHandler"))); } @Override @@ -44,16 +41,9 @@ public class ServerErrorHandlerInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { return singletonMap( - named("error").and(takesArgument(1, Throwable.class)), ErrorHandlerAdvice.class.getName()); - } - - public static class ErrorHandlerAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void captureThrowable(@Advice.Argument(1) final Throwable throwable) { - final Span span = GlobalTracer.get().activeSpan(); - if (span != null) { - DECORATE.onError(span, throwable); - } - } + named("error") + .and(takesArgument(0, named("ratpack.handling.Context"))) + .and(takesArgument(1, Throwable.class)), + packageName + ".ErrorHandlerAdvice"); } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ErrorHandlerAdvice.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ErrorHandlerAdvice.java new file mode 100644 index 0000000000..55d13dbaf2 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ErrorHandlerAdvice.java @@ -0,0 +1,19 @@ +package datadog.trace.instrumentation.ratpack; + +import static datadog.trace.instrumentation.ratpack.RatpackServerDecorator.DECORATE; + +import io.opentracing.Span; +import java.util.Optional; +import net.bytebuddy.asm.Advice; +import ratpack.handling.Context; + +public class ErrorHandlerAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void captureThrowable( + @Advice.Argument(0) final Context ctx, @Advice.Argument(1) final Throwable throwable) { + final Optional span = ctx.maybeGet(Span.class); + if (span.isPresent()) { + DECORATE.onError(span.get(), throwable); + } + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java index da41f8f316..fb3153bed0 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java @@ -79,4 +79,13 @@ public class RatpackServerDecorator extends HttpServerDecorator + // close the scope now that we got the baggage inside the promises + scope.close() + return now + }) + }) + + then: + result.valueOrThrow == ["foo", "foo"] + assertTraces(1) { + trace(0, 1) { + span(0) { + resourceName "INSIDE-TEST" + serviceName "unnamed-java-app" + operationName "ratpack.exec-test" + parent() + errored false + tags { + defaultTags() + } + } + } + } + } + + // returns a promise that contains the active scope's "test-baggage" baggage + Promise testPromise(CountDownLatch latch = null) { + Promise.sync { + latch?.await() + Scope tracerScope = GlobalTracer.get().scopeManager().active() + return tracerScope?.span()?.getBaggageItem("test-baggage") + } + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/RatpackTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/RatpackTest.groovy deleted file mode 100644 index a27f60cff8..0000000000 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/RatpackTest.groovy +++ /dev/null @@ -1,694 +0,0 @@ -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.agent.test.utils.OkHttpUtils -import datadog.trace.api.DDSpanTypes -import datadog.trace.api.DDTags -import datadog.trace.context.TraceScope -import io.opentracing.Scope -import io.opentracing.Span -import io.opentracing.tag.Tags -import io.opentracing.util.GlobalTracer -import okhttp3.HttpUrl -import okhttp3.OkHttpClient -import okhttp3.Request -import ratpack.exec.Promise -import ratpack.exec.util.ParallelBatch -import ratpack.groovy.test.embed.GroovyEmbeddedApp -import ratpack.handling.internal.HandlerException -import ratpack.http.HttpUrlBuilder -import ratpack.http.client.HttpClient -import ratpack.path.PathBinding -import ratpack.test.exec.ExecHarness - -import java.util.concurrent.CountDownLatch -import java.util.regex.Pattern - -import static datadog.trace.agent.test.server.http.TestHttpServer.distributedRequestTrace -import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer -import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT - -class RatpackTest extends AgentTestRunner { - - OkHttpClient client = OkHttpUtils.client() - - def "test bindings for #path"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - prefix("a") { - all { - context.render(context.get(PathBinding).description) - } - } - prefix("b/::\\d+") { - all { - context.render(context.get(PathBinding).description) - } - } - prefix("c/:val?") { - all { - context.render(context.get(PathBinding).description) - } - } - prefix("d/:val") { - all { - context.render(context.get(PathBinding).description) - } - } - prefix("e/:val?:\\d+") { - all { - context.render(context.get(PathBinding).description) - } - } - prefix("f/:val:\\d+") { - all { - context.render(context.get(PathBinding).description) - } - } - } - } - def request = new Request.Builder() - .url(HttpUrl.get(app.address).newBuilder().addPathSegments(path).build()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 200 - resp.body.string() == route - - assertTraces(1) { - trace(0, 2) { - span(0) { - resourceName "GET /$route" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored false - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${app.address.resolve(path)}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - span(1) { - resourceName "GET /$route" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored false - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${app.address.resolve(path)}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - } - } - - where: - path | route - "a" | "a" - "b/123" | "b/::\\d+" - "c" | "c/:val?" - "c/123" | "c/:val?" - "c/foo" | "c/:val?" - "d/123" | "d/:val" - "d/foo" | "d/:val" - "e" | "e/:val?:\\d+" - "e/123" | "e/:val?:\\d+" - "e/foo" | "e/:val?:\\d+" - "f/123" | "f/:val:\\d+" - } - - def "test handler error response"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - prefix("handler-error") { - all { - 0 / 0 - } - } - } - } - def request = new Request.Builder() - .url(app.address.resolve("/handler-error?query=param").toURL()) - .get() - .build() - when: - def resp = client.newCall(request).execute() - then: - resp.code() == 500 - - assertTraces(1) { - trace(0, 2) { - span(0) { - resourceName "GET /handler-error" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored true - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "${app.address.resolve('handler-error')}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - errorTags(ArithmeticException, Pattern.compile("Division( is)? undefined")) - defaultTags() - } - } - span(1) { - resourceName "GET /handler-error" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored true - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "${app.address.resolve('handler-error')}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - errorTags(HandlerException, Pattern.compile("java.lang.ArithmeticException: Division( is)? undefined")) - defaultTags() - } - } - } - } - } - - def "test promise error response"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - get("promise-error") { - Promise.async { - 0 / 0 - }.then { - context.render(it) - } - } - } - } - def request = new Request.Builder() - .url(app.address.resolve("promise-error?query=param").toURL()) - .get() - .build() - when: - def resp = client.newCall(request).execute() - then: - resp.code() == 500 - - assertTraces(1) { - trace(0, 2) { - span(0) { - resourceName "GET /promise-error" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored true - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "${app.address.resolve('promise-error')}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - errorTags(ArithmeticException, Pattern.compile("Division( is)? undefined")) - defaultTags() - } - } - span(1) { - resourceName "GET /promise-error" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored true - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "${app.address.resolve('promise-error')}" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - "$Tags.ERROR.key" true - defaultTags() - } - } - } - } - } - - def "test render error response"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - all { - context.render(Promise.sync { - return "fail " + 0 / 0 - }) - } - } - } - def request = new Request.Builder() - .url(app.address.resolve("?query=param").toURL()) - .get() - .build() - when: - def resp = client.newCall(request).execute() - then: - resp.code() == 500 - - assertTraces(1) { - trace(0, 2) { - span(0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored true - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - errorTags(ArithmeticException, Pattern.compile("Division( is)? undefined")) - defaultTags() - } - } - span(1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored true - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - "$Tags.ERROR.key" true - defaultTags() - } - } - } - } - } - - def "test path call using ratpack http client"() { - setup: - - // Use jetty based server to avoid confusion. - def external = httpServer { - handlers { - get("nested") { - handleDistributedRequest() - response.send("succ") - } - get("nested2") { - handleDistributedRequest() - response.send("ess") - } - } - } - - def app = GroovyEmbeddedApp.ratpack { - handlers { - get { HttpClient httpClient -> - // 1st internal http client call to nested - httpClient.get(HttpUrlBuilder.base(external.address).path("nested").build()) - .map { it.body.text } - .flatMap { t -> - // make a 2nd http request and concatenate the two bodies together - httpClient.get(HttpUrlBuilder.base(external.address).path("nested2").build()) map { t + it.body.text } - } - .then { - context.render(it) - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 200 - resp.body().string() == "success" - - // 3rd is the three traces, ratpack, http client 2 and http client 1 - // 2nd is nested2 from the external server (the result of the 2nd internal http client call) - // 1st is nested from the external server (the result of the 1st internal http client call) - assertTraces(3) { - distributedRequestTrace(it, 0, trace(2).get(3)) - distributedRequestTrace(it, 1, trace(2).get(2)) - - trace(2, 4) { - // main app span that processed the request from OKHTTP request - span(0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored false - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - span(1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored false - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - // Second http client call that receives the 'ess' of Success - span(2) { - resourceName "GET /?" - serviceName "unnamed-java-app" - operationName "netty.client.request" - spanType DDSpanTypes.HTTP_CLIENT - childOf(span(1)) - errored false - tags { - "$Tags.COMPONENT.key" "netty-client" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${external.address}/nested2" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - // First http client call that receives the 'Succ' of Success - span(3) { - resourceName "GET /nested" - serviceName "unnamed-java-app" - operationName "netty.client.request" - spanType DDSpanTypes.HTTP_CLIENT - childOf(span(1)) - errored false - tags { - "$Tags.COMPONENT.key" "netty-client" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "${external.address}/nested" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - } - } - } - - def "test ratpack http client error handling"() { - setup: - def badAddress = new URI("http://localhost:$UNUSABLE_PORT") - - def app = GroovyEmbeddedApp.ratpack { - handlers { - get { HttpClient httpClient -> - httpClient.get(badAddress) - .map { it.body.text } - .then { - context.render(it) - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 500 - - assertTraces(1) { - trace(0, 3) { - span(0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored true - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - "$Tags.ERROR.key" true - defaultTags() - } - } - span(1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(0)) - errored true - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 500 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - errorTags(ConnectException, String) - defaultTags() - } - } - span(2) { - operationName "netty.connect" - resourceName "netty.connect" - childOf(span(1)) - errored true - tags { - "$Tags.COMPONENT.key" "netty" - errorTags(ConnectException, String) - defaultTags() - } - } - } - } - } - - def "test forked path call and start span in handler (#startSpanInHandler)"() { - setup: - def app = GroovyEmbeddedApp.ratpack { - handlers { - get { - TraceScope scope - if (startSpanInHandler) { - Span childSpan = GlobalTracer.get() - .buildSpan("ratpack.exec-test") - .withTag(DDTags.RESOURCE_NAME, "INSIDE-TEST") - .start() - scope = GlobalTracer.get().scopeManager().activate(childSpan, true) - } - def latch = new CountDownLatch(1) - try { - scope?.setAsyncPropagation(true) - GlobalTracer.get().activeSpan().setBaggageItem("test-baggage", "foo") - - context.render(testPromise(latch).fork()) - } finally { - scope?.close() - latch.countDown() - } - } - } - } - def request = new Request.Builder() - .url(app.address.toURL()) - .get() - .build() - - when: - def resp = client.newCall(request).execute() - - then: - resp.code() == 200 - resp.body().string() == "foo" - - assertTraces(1) { - trace(0, (startSpanInHandler ? 3 : 2)) { - if (startSpanInHandler) { - span(0) { - resourceName "INSIDE-TEST" - serviceName "unnamed-java-app" - operationName "ratpack.exec-test" - childOf(span(2)) - errored false - tags { - defaultTags() - } - } - } - span(startSpanInHandler ? 1 : 0) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "netty.request" - spanType DDSpanTypes.HTTP_SERVER - parent() - errored false - tags { - "$Tags.COMPONENT.key" "netty" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - span(startSpanInHandler ? 2 : 1) { - resourceName "GET /" - serviceName "unnamed-java-app" - operationName "ratpack.handler" - spanType DDSpanTypes.HTTP_SERVER - childOf(span(startSpanInHandler ? 1 : 0)) - errored false - tags { - "$Tags.COMPONENT.key" "ratpack" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_STATUS.key" 200 - "$Tags.HTTP_URL.key" "$app.address" - "$Tags.PEER_HOSTNAME.key" "$app.address.host" - "$Tags.PEER_PORT.key" Integer - defaultTags() - } - } - } - } - - where: - startSpanInHandler << [true, false] - } - - def "forked executions inherit parent scope"() { - when: - def result = ExecHarness.yieldSingle({}, { - final Scope scope = - GlobalTracer.get() - .buildSpan("ratpack.exec-test") - .withTag(DDTags.RESOURCE_NAME, "INSIDE-TEST") - .startActive(true) - - ((TraceScope) scope).setAsyncPropagation(true) - scope.span().setBaggageItem("test-baggage", "foo") - ParallelBatch.of(testPromise(), testPromise()) - .yield() - .map({ now -> - // close the scope now that we got the baggage inside the promises - scope.close() - return now - }) - }) - - then: - result.valueOrThrow == ["foo", "foo"] - assertTraces(1) { - trace(0, 1) { - span(0) { - resourceName "INSIDE-TEST" - serviceName "unnamed-java-app" - operationName "ratpack.exec-test" - parent() - errored false - tags { - defaultTags() - } - } - } - } - } - - // returns a promise that contains the active scope's "test-baggage" baggage - Promise testPromise(CountDownLatch latch = null) { - Promise.sync { - latch?.await() - Scope tracerScope = GlobalTracer.get().scopeManager().active() - return tracerScope?.span()?.getBaggageItem("test-baggage") - } - } -} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackForkedHttpClientTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackForkedHttpClientTest.groovy new file mode 100644 index 0000000000..abccfd6366 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackForkedHttpClientTest.groovy @@ -0,0 +1,26 @@ +package client + + +import ratpack.exec.ExecResult + +class RatpackForkedHttpClientTest extends RatpackHttpClientTest { + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + ExecResult result = exec.yield { + def resp = client.request(uri) { spec -> + spec.method(method) + spec.headers { headersSpec -> + headers.entrySet().each { + headersSpec.add(it.key, it.value) + } + } + } + return resp.fork().map { + callback?.call() + it.status.code + } + } + return result.value + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackHttpClientTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackHttpClientTest.groovy new file mode 100644 index 0000000000..61d0f81eb4 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackHttpClientTest.groovy @@ -0,0 +1,58 @@ +package client + +import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.netty41.client.NettyHttpClientDecorator +import ratpack.exec.ExecResult +import ratpack.http.client.HttpClient +import ratpack.test.exec.ExecHarness +import spock.lang.AutoCleanup +import spock.lang.Shared + +class RatpackHttpClientTest extends HttpClientTest { + + @AutoCleanup + @Shared + ExecHarness exec = ExecHarness.harness() + + @Shared + def client = HttpClient.of {} + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + ExecResult result = exec.yield { + def resp = client.request(uri) { spec -> + spec.method(method) + spec.headers { headersSpec -> + headers.entrySet().each { + headersSpec.add(it.key, it.value) + } + } + } + return resp.map { + callback?.call() + it.status.code + } + } + return result.value + } + + @Override + NettyHttpClientDecorator decorator() { + return NettyHttpClientDecorator.DECORATE + } + + @Override + String expectedOperationName() { + return "netty.client.request" + } + + @Override + boolean testRedirects() { + false + } + + @Override + boolean testConnectionFailure() { + false + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/NettyServerTestInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/NettyServerTestInstrumentation.java new file mode 100644 index 0000000000..61cb150a54 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/NettyServerTestInstrumentation.java @@ -0,0 +1,22 @@ +package server; + +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 NettyServerTestInstrumentation implements Instrumenter { + + @Override + public AgentBuilder instrument(final AgentBuilder agentBuilder) { + return agentBuilder + .type(named("io.netty.handler.codec.ByteToMessageDecoder")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("channelRead"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackAsyncHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackAsyncHttpServerTest.groovy new file mode 100644 index 0000000000..c106995923 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackAsyncHttpServerTest.groovy @@ -0,0 +1,75 @@ +package server + +import ratpack.exec.Promise +import ratpack.groovy.test.embed.GroovyEmbeddedApp +import ratpack.test.embed.EmbeddedApp + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +class RatpackAsyncHttpServerTest extends RatpackHttpServerTest { + + @Override + EmbeddedApp startServer(int bindPort) { + def ratpack = GroovyEmbeddedApp.ratpack { + serverConfig { + port bindPort + } + bindings { + bind TestErrorHandler + } + handlers { + prefix(SUCCESS.rawPath()) { + all { + Promise.sync { + SUCCESS + } then { ServerEndpoint endpoint -> + controller(endpoint) { + context.response.status(endpoint.status).send(endpoint.body) + } + } + } + } + prefix(REDIRECT.rawPath()) { + all { + Promise.sync { + REDIRECT + } then { ServerEndpoint endpoint -> + controller(endpoint) { + context.redirect(endpoint.body) + } + } + } + } + prefix(ERROR.rawPath()) { + all { + Promise.sync { + ERROR + } then { ServerEndpoint endpoint -> + controller(endpoint) { + context.response.status(endpoint.status).send(endpoint.body) + } + } + } + } + prefix(EXCEPTION.rawPath()) { + all { + Promise.sync { + EXCEPTION + } then { ServerEndpoint endpoint -> + controller(endpoint) { + throw new Exception(endpoint.body) + } + } + } + } + } + } + ratpack.server.start() + + assert ratpack.address.port == bindPort + return ratpack + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy new file mode 100644 index 0000000000..39b1547535 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy @@ -0,0 +1,75 @@ +package server + +import ratpack.exec.Promise +import ratpack.groovy.test.embed.GroovyEmbeddedApp +import ratpack.test.embed.EmbeddedApp + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +class RatpackForkedHttpServerTest extends RatpackHttpServerTest { + + @Override + EmbeddedApp startServer(int bindPort) { + def ratpack = GroovyEmbeddedApp.ratpack { + serverConfig { + port bindPort + } + bindings { + bind TestErrorHandler + } + handlers { + prefix(SUCCESS.rawPath()) { + all { + Promise.sync { + SUCCESS + }.fork().then { ServerEndpoint endpoint -> + controller(endpoint) { + context.response.status(endpoint.status).send(endpoint.body) + } + } + } + } + prefix(REDIRECT.rawPath()) { + all { + Promise.sync { + REDIRECT + }.fork().then { ServerEndpoint endpoint -> + controller(endpoint) { + context.redirect(endpoint.body) + } + } + } + } + prefix(ERROR.rawPath()) { + all { + Promise.sync { + ERROR + }.fork().then { ServerEndpoint endpoint -> + controller(endpoint) { + context.response.status(endpoint.status).send(endpoint.body) + } + } + } + } + prefix(EXCEPTION.rawPath()) { + all { + Promise.sync { + EXCEPTION + }.fork().then { ServerEndpoint endpoint -> + controller(endpoint) { + throw new Exception(endpoint.body) + } + } + } + } + } + } + ratpack.server.start() + + assert ratpack.address.port == bindPort + return ratpack + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy new file mode 100644 index 0000000000..33398003c1 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -0,0 +1,145 @@ +package server + +import datadog.opentracing.DDSpan +import datadog.trace.agent.test.asserts.ListWriterAssert +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.netty41.server.NettyHttpServerDecorator +import datadog.trace.instrumentation.ratpack.RatpackServerDecorator +import groovy.transform.stc.ClosureParams +import groovy.transform.stc.SimpleType +import io.opentracing.tag.Tags +import ratpack.error.ServerErrorHandler +import ratpack.groovy.test.embed.GroovyEmbeddedApp +import ratpack.handling.Context +import ratpack.test.embed.EmbeddedApp + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +class RatpackHttpServerTest extends HttpServerTest { + + @Override + EmbeddedApp startServer(int bindPort) { + def ratpack = GroovyEmbeddedApp.ratpack { + serverConfig { + port bindPort + } + bindings { + bind TestErrorHandler + } + handlers { + prefix(SUCCESS.rawPath()) { + all { + controller(SUCCESS) { + context.response.status(SUCCESS.status).send(SUCCESS.body) + } + } + } + prefix(REDIRECT.rawPath()) { + all { + controller(REDIRECT) { + context.redirect(REDIRECT.body) + } + } + } + prefix(ERROR.rawPath()) { + all { + controller(ERROR) { + context.response.status(ERROR.status).send(ERROR.body) + } + } + } + prefix(EXCEPTION.rawPath()) { + all { + controller(EXCEPTION) { + throw new Exception(EXCEPTION.body) + } + } + } + } + } + ratpack.server.start() + + assert ratpack.address.port == bindPort + return ratpack + } + + static class TestErrorHandler implements ServerErrorHandler { + @Override + void error(Context context, Throwable throwable) throws Exception { + context.response.status(500).send(throwable.message) + } + } + + @Override + void stopServer(EmbeddedApp server) { + server.close() + } + + @Override + NettyHttpServerDecorator decorator() { + return NettyHttpServerDecorator.DECORATE + } + + @Override + String expectedOperationName() { + "netty.request" + } + + @Override + boolean hasHandlerSpan() { + true + } + + 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 * 2) + + // Ratpack closes the handler span before the controller returns, so we need to manually reorder it. + TEST_WRITER.each { + def controllerSpan = it.find { + it.operationName == "controller" + } + if (controllerSpan) { + it.remove(controllerSpan) + it.add(controllerSpan) + } + } + super.cleanAndAssertTraces(size, spec) + } + + @Override + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + serviceName expectedServiceName() + operationName "ratpack.handler" + spanType DDSpanTypes.HTTP_SERVER + errored endpoint == ERROR || endpoint == EXCEPTION + childOf(parent as DDSpan) + tags { + "$Tags.COMPONENT.key" RatpackServerDecorator.DECORATE.component() + "$Tags.HTTP_STATUS.key" Integer + "$Tags.HTTP_URL.key" String + "$Tags.PEER_HOSTNAME.key" "localhost" + "$Tags.PEER_PORT.key" Integer + "$Tags.PEER_HOST_IPV4.key" { it == null || it == "127.0.0.1" } // Optional + "$Tags.HTTP_METHOD.key" String + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + defaultTags() + if (endpoint == ERROR) { + "$Tags.ERROR.key" true + } else if (endpoint == EXCEPTION) { + errorTags(Exception, EXCEPTION.body) + } + } + } + } +} 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 c07a502ce6..623e68eef4 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 @@ -72,6 +72,10 @@ abstract class HttpServerTest ext abstract String expectedOperationName() + boolean hasHandlerSpan() { + false + } + boolean testNotFound() { true } @@ -148,9 +152,17 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(count) { (1..count).eachWithIndex { val, i -> - trace(i, 2) { - serverSpan(it, 0) - controllerSpan(it, 1, span(0)) + if (hasHandlerSpan()) { + trace(i, 3) { + serverSpan(it, 0) + handlerSpan(it, 1, span(0)) + controllerSpan(it, 2, span(1)) + } + } else { + trace(i, 2) { + serverSpan(it, 0) + controllerSpan(it, 1, span(0)) + } } } } @@ -177,9 +189,17 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(1) { - trace(0, 2) { - serverSpan(it, 0, traceId, parentId) - controllerSpan(it, 1, span(0)) + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, traceId, parentId) + handlerSpan(it, 1, span(0)) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, traceId, parentId) + controllerSpan(it, 1, span(0)) + } } } @@ -201,9 +221,17 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(1) { - trace(0, 2) { - serverSpan(it, 0, null, null, method, REDIRECT) - controllerSpan(it, 1, span(0)) + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, null, null, method, REDIRECT) + handlerSpan(it, 1, span(0)) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, null, null, method, REDIRECT) + controllerSpan(it, 1, span(0)) + } } } @@ -223,9 +251,17 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(1) { - trace(0, 2) { - serverSpan(it, 0, null, null, method, ERROR) - controllerSpan(it, 1, span(0)) + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, null, null, method, ERROR) + handlerSpan(it, 1, span(0), ERROR) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, null, null, method, ERROR) + controllerSpan(it, 1, span(0)) + } } } @@ -247,9 +283,17 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(1) { - trace(0, 2) { - serverSpan(it, 0, null, null, method, EXCEPTION) - controllerSpan(it, 1, span(0), EXCEPTION.body) + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, null, null, method, EXCEPTION) + handlerSpan(it, 1, span(0), EXCEPTION) + controllerSpan(it, 2, span(1), EXCEPTION.body) + } + } else { + trace(0, 2) { + serverSpan(it, 0, null, null, method, EXCEPTION) + controllerSpan(it, 1, span(0), EXCEPTION.body) + } } } @@ -269,8 +313,15 @@ abstract class HttpServerTest ext and: cleanAndAssertTraces(1) { - trace(0, 1) { - serverSpan(it, 0, null, null, method, NOT_FOUND) + if (hasHandlerSpan()) { + trace(0, 2) { + serverSpan(it, 0, null, null, method, NOT_FOUND) + handlerSpan(it, 1, span(0), NOT_FOUND) + } + } else { + trace(0, 1) { + serverSpan(it, 0, null, null, method, NOT_FOUND) + } } } @@ -320,6 +371,10 @@ abstract class HttpServerTest ext } } + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + throw new UnsupportedOperationException("handlerSpan not implemented in " + getClass().name) + } + // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { From fc30b4c5bbea5d31cce89af4105ab0f35c08ad76 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 16 Aug 2019 09:22:44 -0700 Subject: [PATCH 2/3] CR fixes. --- .../src/test/groovy/GrizzlyAsyncTest.groovy | 29 ++++-------------- .../ratpack/ContinuationInstrumentation.java | 11 +++---- .../DefaultExecutionInstrumentation.java | 5 +--- .../server/RatpackHttpServerTest.groovy | 25 ++-------------- .../groovy/server/VertxHttpServerTest.groovy | 30 ++++--------------- .../agent/test/base/HttpServerTest.groovy | 17 +++++++++++ 6 files changed, 36 insertions(+), 81 deletions(-) diff --git a/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyAsyncTest.groovy b/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyAsyncTest.groovy index 97589cef3b..912dfa6d77 100644 --- a/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyAsyncTest.groovy +++ b/dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyAsyncTest.groovy @@ -1,6 +1,3 @@ -import datadog.trace.agent.test.asserts.ListWriterAssert -import groovy.transform.stc.ClosureParams -import groovy.transform.stc.SimpleType import javax.ws.rs.GET import javax.ws.rs.Path import javax.ws.rs.container.AsyncResponse @@ -28,6 +25,11 @@ class GrizzlyAsyncTest extends GrizzlyTest { GrizzlyHttpServerFactory.createHttpServer(new URI("http://localhost:$port"), rc) } + @Override + boolean reorderControllerSpan() { + true + } + @Path("/") static class AsyncServiceResource { private ExecutorService executor = Executors.newSingleThreadExecutor() @@ -76,25 +78,4 @@ class GrizzlyAsyncTest extends GrizzlyTest { } } } - - 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 * 2) - - // AsyncResponse.resume closes the handler span before the controller returns, so we need to manually reorder it. - TEST_WRITER.each { - def controllerSpan = it.find { - it.operationName == "controller" - } - if (controllerSpan) { - it.remove(controllerSpan) - it.add(controllerSpan) - } - } - super.cleanAndAssertTraces(size, spec) - } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java index 3d2fd7ecd2..78773f42a6 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java @@ -62,34 +62,31 @@ public final class ContinuationInstrumentation extends Instrumenter.Default { } @Slf4j - public static class BlockWrapper implements Block { + public static class BlockWrapper implements Block { private final Block delegate; private final Span span; private BlockWrapper(final Block delegate, final Span span) { + assert span != null; this.delegate = delegate; this.span = span; } @Override public void execute() throws Exception { - if (span != null) { try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(true); } delegate.execute(); } - } else { - delegate.execute(); - } } public static Block wrapIfNeeded(final Block delegate, final Span span) { - if (delegate instanceof BlockWrapper || span == null) { + if (delegate instanceof BlockWrapper) { return delegate; } - log.debug("Wrapping action task {}", delegate); + log.debug("Wrapping block {}", delegate); return new BlockWrapper(delegate, span); } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java index 908719a51e..8e130a1e20 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java @@ -77,22 +77,19 @@ public final class DefaultExecutionInstrumentation extends Instrumenter.Default private final Span span; private ActionWrapper(final Action delegate, final Span span) { + assert span != null; this.delegate = delegate; this.span = span; } @Override public void execute(final T t) throws Exception { - if (span != null) { try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { if (scope instanceof TraceScope) { ((TraceScope) scope).setAsyncPropagation(true); } delegate.execute(t); } - } else { - delegate.execute(t); - } } public static Action wrapIfNeeded(final Action delegate, final Span span) { diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy index 33398003c1..095edd7b16 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -1,14 +1,11 @@ package server import datadog.opentracing.DDSpan -import datadog.trace.agent.test.asserts.ListWriterAssert import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes import datadog.trace.instrumentation.netty41.server.NettyHttpServerDecorator import datadog.trace.instrumentation.ratpack.RatpackServerDecorator -import groovy.transform.stc.ClosureParams -import groovy.transform.stc.SimpleType import io.opentracing.tag.Tags import ratpack.error.ServerErrorHandler import ratpack.groovy.test.embed.GroovyEmbeddedApp @@ -95,25 +92,9 @@ class RatpackHttpServerTest extends HttpServerTest ext false } + boolean reorderControllerSpan() { + true + } + boolean testNotFound() { true } @@ -352,6 +356,19 @@ abstract class HttpServerTest ext assert toRemove.size() == size TEST_WRITER.removeAll(toRemove) + if(reorderControllerSpan()) { + // Some frameworks close the handler span before the controller returns, so we need to manually reorder it. + TEST_WRITER.each { + def controllerSpan = it.find { + it.operationName == "controller" + } + if (controllerSpan) { + it.remove(controllerSpan) + it.add(controllerSpan) + } + } + } + assertTraces(size, spec) } From 452a619b4b4b25e2d5a3697c4039ffa024f462d7 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 16 Aug 2019 10:10:09 -0700 Subject: [PATCH 3/3] =?UTF-8?q?Muzzle=20doesn=E2=80=99t=20seem=20to=20like?= =?UTF-8?q?=20those=20helper=20classes=20there?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Had to move them externally for muzzle to be happy. --- .../ratpack/ContinuationInstrumentation.java | 41 +--------------- .../DefaultExecutionInstrumentation.java | 49 +++---------------- .../ratpack/ActionWrapper.java | 38 ++++++++++++++ .../instrumentation/ratpack/BlockWrapper.java | 38 ++++++++++++++ 4 files changed, 85 insertions(+), 81 deletions(-) create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ActionWrapper.java create mode 100644 dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/BlockWrapper.java diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java index 78773f42a6..2600fe9f49 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/ContinuationInstrumentation.java @@ -7,12 +7,8 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.context.TraceScope; -import io.opentracing.Scope; -import io.opentracing.Span; import io.opentracing.util.GlobalTracer; import java.util.Map; -import lombok.extern.slf4j.Slf4j; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -35,7 +31,7 @@ public final class ContinuationInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - getClass().getName() + "$ResumeAdvice$BlockWrapper", + packageName + ".BlockWrapper", }; } @@ -50,45 +46,12 @@ public final class ContinuationInstrumentation extends Instrumenter.Default { @Advice.OnMethodEnter(suppress = Throwable.class) public static void wrap(@Advice.Argument(value = 0, readOnly = false) Block block) { - final Span span = GlobalTracer.get().activeSpan(); - if (span != null) { - block = BlockWrapper.wrapIfNeeded(block, span); - } + block = BlockWrapper.wrapIfNeeded(block, GlobalTracer.get().activeSpan()); } public void muzzleCheck(final PathBinding binding) { // This was added in 1.4. Added here to ensure consistency with other instrumentation. binding.getDescription(); } - - @Slf4j - public static class BlockWrapper implements Block { - private final Block delegate; - private final Span span; - - private BlockWrapper(final Block delegate, final Span span) { - assert span != null; - this.delegate = delegate; - this.span = span; - } - - @Override - public void execute() throws Exception { - try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { - if (scope instanceof TraceScope) { - ((TraceScope) scope).setAsyncPropagation(true); - } - delegate.execute(); - } - } - - public static Block wrapIfNeeded(final Block delegate, final Span span) { - if (delegate instanceof BlockWrapper) { - return delegate; - } - log.debug("Wrapping block {}", delegate); - return new BlockWrapper(delegate, span); - } - } } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java index 8e130a1e20..a2d59bc293 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/DefaultExecutionInstrumentation.java @@ -7,12 +7,9 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.context.TraceScope; -import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.util.GlobalTracer; import java.util.Map; -import lombok.extern.slf4j.Slf4j; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -36,7 +33,7 @@ public final class DefaultExecutionInstrumentation extends Instrumenter.Default @Override public String[] helperClassNames() { return new String[] { - getClass().getName() + "$DelimitAdvice$ActionWrapper", + packageName + ".ActionWrapper", }; } @@ -55,50 +52,18 @@ public final class DefaultExecutionInstrumentation extends Instrumenter.Default public static void wrap( @Advice.Argument(value = 0, readOnly = false) Action onError, @Advice.Argument(value = 1, readOnly = false) Action segment) { + /** + * Here we pass along the span instead of a continuation because we aren't sure the callback + * will actually be called. + */ final Span span = GlobalTracer.get().activeSpan(); - if (span != null) { - /** - * Here we pass along the span instead of a continuation because we aren't sure it won't be - * used for both callbacks. - */ - onError = ActionWrapper.wrapIfNeeded(onError, span); - segment = ActionWrapper.wrapIfNeeded(segment, span); - } + onError = ActionWrapper.wrapIfNeeded(onError, span); + segment = ActionWrapper.wrapIfNeeded(segment, span); } public void muzzleCheck(final PathBinding binding) { // This was added in 1.4. Added here to ensure consistency with other instrumentation. binding.getDescription(); } - - @Slf4j - public static class ActionWrapper implements Action { - private final Action delegate; - private final Span span; - - private ActionWrapper(final Action delegate, final Span span) { - assert span != null; - this.delegate = delegate; - this.span = span; - } - - @Override - public void execute(final T t) throws Exception { - try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { - if (scope instanceof TraceScope) { - ((TraceScope) scope).setAsyncPropagation(true); - } - delegate.execute(t); - } - } - - public static Action wrapIfNeeded(final Action delegate, final Span span) { - if (delegate instanceof ActionWrapper || span == null) { - return delegate; - } - log.debug("Wrapping action task {}", delegate); - return new ActionWrapper(delegate, span); - } - } } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ActionWrapper.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ActionWrapper.java new file mode 100644 index 0000000000..ffd62209fc --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/ActionWrapper.java @@ -0,0 +1,38 @@ +package datadog.trace.instrumentation.ratpack; + +import datadog.trace.context.TraceScope; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.util.GlobalTracer; +import lombok.extern.slf4j.Slf4j; +import ratpack.func.Action; + +@Slf4j +public class ActionWrapper implements Action { + private final Action delegate; + private final Span span; + + private ActionWrapper(final Action delegate, final Span span) { + assert span != null; + this.delegate = delegate; + this.span = span; + } + + @Override + public void execute(final T t) throws Exception { + try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(true); + } + delegate.execute(t); + } + } + + public static Action wrapIfNeeded(final Action delegate, final Span span) { + if (delegate instanceof ActionWrapper || span == null) { + return delegate; + } + log.debug("Wrapping action task {}", delegate); + return new ActionWrapper(delegate, span); + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/BlockWrapper.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/BlockWrapper.java new file mode 100644 index 0000000000..4803af1242 --- /dev/null +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/BlockWrapper.java @@ -0,0 +1,38 @@ +package datadog.trace.instrumentation.ratpack; + +import datadog.trace.context.TraceScope; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.util.GlobalTracer; +import lombok.extern.slf4j.Slf4j; +import ratpack.func.Block; + +@Slf4j +public class BlockWrapper implements Block { + private final Block delegate; + private final Span span; + + private BlockWrapper(final Block delegate, final Span span) { + assert span != null; + this.delegate = delegate; + this.span = span; + } + + @Override + public void execute() throws Exception { + try (final Scope scope = GlobalTracer.get().scopeManager().activate(span, false)) { + if (scope instanceof TraceScope) { + ((TraceScope) scope).setAsyncPropagation(true); + } + delegate.execute(); + } + } + + public static Block wrapIfNeeded(final Block delegate, final Span span) { + if (delegate instanceof BlockWrapper || span == null) { + return delegate; + } + log.debug("Wrapping block {}", delegate); + return new BlockWrapper(delegate, span); + } +}