From fc30b4c5bbea5d31cce89af4105ab0f35c08ad76 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 16 Aug 2019 09:22:44 -0700 Subject: [PATCH] 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) }