From 73122ea72c563c9ed4b5b0bd604251be766f250b Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 16 Mar 2020 16:55:31 -0400 Subject: [PATCH 01/15] Starting to get tests building --- .../instrumentation/play-2.3/.gitignore | 1 + .../instrumentation/play-2.3/play-2.3.gradle | 47 ++++++++++ .../play24/PlayInstrumentation.java | 52 +++++++++++ .../instrumentation/play24/PlayAdvice.java | 73 ++++++++++++++++ .../instrumentation/play24/PlayHeaders.java | 26 ++++++ .../play24/PlayHttpServerDecorator.java | 86 +++++++++++++++++++ .../play24/RequestCompleteCallback.java | 40 +++++++++ .../groovy/client/PlayWSClientTest.groovy | 54 ++++++++++++ .../NettyServerTestInstrumentation.java | 22 +++++ .../groovy/server/PlayAsyncServerTest.groovy | 46 ++++++++++ .../test/groovy/server/PlayServerTest.groovy | 79 +++++++++++++++++ .../src/test/scala/server/AsyncServer.scala | 5 ++ .../src/test/scala/server/SyncServer.scala | 15 ++++ settings.gradle | 1 + 14 files changed, 547 insertions(+) create mode 100644 dd-java-agent/instrumentation/play-2.3/.gitignore create mode 100644 dd-java-agent/instrumentation/play-2.3/play-2.3.gradle create mode 100644 dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala diff --git a/dd-java-agent/instrumentation/play-2.3/.gitignore b/dd-java-agent/instrumentation/play-2.3/.gitignore new file mode 100644 index 0000000000..5292519a25 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/.gitignore @@ -0,0 +1 @@ +logs/ \ No newline at end of file diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle new file mode 100644 index 0000000000..e77bad0dea --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -0,0 +1,47 @@ +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 + // Play doesn't work with Java 9+ until 2.6.12 + maxJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +muzzle { + pass { + group = 'com.typesafe.play' + module = 'play_2.11' + versions = '[2.3.0,2.4)' + assertInverse = true + } + fail { + group = 'com.typesafe.play' + module = 'play_2.12' + versions = '[,]' + } + fail { + group = 'com.typesafe.play' + module = 'play_2.13' + versions = '[,]' + } +} + +apply from: "${rootDir}/gradle/java.gradle" +apply from: "${rootDir}/gradle/test-with-scala.gradle" + +apply plugin: 'org.unbroken-dome.test-sets' + +testSets { + latestDepTest { + dirName = 'test' + } +} + +dependencies { + main_java8Compile group: 'com.typesafe.play', name: 'play_2.11', version: '2.3.9' + + testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.9' + testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.9' + testCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.9') + + latestDepTestCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.+' + latestDepTestCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.+' + latestDepTestCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.+') +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java new file mode 100644 index 0000000000..fa7d265158 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java @@ -0,0 +1,52 @@ +package datadog.trace.instrumentation.play24; + +import static datadog.trace.agent.tooling.ClassLoaderMatcher.hasClassesNamed; +import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.implementsInterface; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.returns; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class PlayInstrumentation extends Instrumenter.Default { + + public PlayInstrumentation() { + super("play"); + } + + @Override + public ElementMatcher classLoaderMatcher() { + // Optimization for expensive typeMatcher. + return hasClassesNamed("play.api.mvc.Action"); + } + + @Override + public ElementMatcher typeMatcher() { + return implementsInterface(named("play.api.mvc.Action")); + } + + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".PlayHttpServerDecorator", + packageName + ".RequestCompleteCallback", + packageName + ".PlayHeaders", + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + named("apply") + .and(takesArgument(0, named("play.api.mvc.Request"))) + .and(returns(named("scala.concurrent.Future"))), + packageName + ".PlayAdvice"); + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java new file mode 100644 index 0000000000..2e495f2e72 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java @@ -0,0 +1,73 @@ +package datadog.trace.instrumentation.play24; + +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.play24.PlayHeaders.GETTER; +import static datadog.trace.instrumentation.play24.PlayHttpServerDecorator.DECORATE; + +import datadog.trace.bootstrap.instrumentation.api.AgentScope; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan.Context; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import net.bytebuddy.asm.Advice; +import play.api.mvc.Action; +import play.api.mvc.Request; +import play.api.mvc.Result; +import scala.concurrent.Future; + +public class PlayAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope onEnter(@Advice.Argument(0) final Request req) { + final AgentSpan span; + if (activeSpan() == null) { + final Context extractedContext = propagate().extract(req.headers(), GETTER); + span = startSpan("play.request", extractedContext); + } else { + // An upstream framework (e.g. akka-http, netty) has already started the span. + // Do not extract the context. + span = startSpan("play.request"); + } + DECORATE.afterStart(span); + DECORATE.onConnection(span, req); + + final AgentScope scope = activateSpan(span, false); + scope.setAsyncPropagation(true); + return scope; + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopTraceOnResponse( + @Advice.Enter final AgentScope playControllerScope, + @Advice.This final Object thisAction, + @Advice.Thrown final Throwable throwable, + @Advice.Argument(0) final Request req, + @Advice.Return(readOnly = false) final Future responseFuture) { + final AgentSpan playControllerSpan = playControllerScope.span(); + + // Call onRequest on return after tags are populated. + DECORATE.onRequest(playControllerSpan, req); + + if (throwable == null) { + responseFuture.onComplete( + new RequestCompleteCallback(playControllerSpan), + ((Action) thisAction).executionContext()); + } else { + DECORATE.onError(playControllerSpan, throwable); + playControllerSpan.setTag(Tags.HTTP_STATUS, 500); + DECORATE.beforeFinish(playControllerSpan); + playControllerSpan.finish(); + } + playControllerScope.close(); + + final AgentSpan rootSpan = activeSpan(); + // set the resource name on the upstream akka/netty span + DECORATE.onRequest(rootSpan, req); + } + + // Unused method for muzzle to allow only 2.4-2.5 + public static void muzzleCheck() { + play.libs.Akka.system(); + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java new file mode 100644 index 0000000000..fd8c22ea83 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java @@ -0,0 +1,26 @@ +package datadog.trace.instrumentation.play24; + +import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; +import play.api.mvc.Headers; +import scala.Option; +import scala.collection.JavaConversions; + +public class PlayHeaders implements AgentPropagation.Getter { + + public static final PlayHeaders GETTER = new PlayHeaders(); + + @Override + public Iterable keys(final Headers headers) { + return JavaConversions.asJavaIterable(headers.keys()); + } + + @Override + public String get(final Headers headers, final String key) { + final Option option = headers.get(key); + if (option.isDefined()) { + return option.get(); + } else { + return null; + } + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java new file mode 100644 index 0000000000..6c12b2f301 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java @@ -0,0 +1,86 @@ +package datadog.trace.instrumentation.play24; + +import datadog.trace.api.DDTags; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.UndeclaredThrowableException; +import java.net.URI; +import java.net.URISyntaxException; +import lombok.extern.slf4j.Slf4j; +import play.api.mvc.Request; +import play.api.mvc.Result; +import scala.Option; + +@Slf4j +public class PlayHttpServerDecorator extends HttpServerDecorator { + public static final PlayHttpServerDecorator DECORATE = new PlayHttpServerDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[] {"play"}; + } + + @Override + protected String component() { + return "play-action"; + } + + @Override + protected String method(final Request httpRequest) { + return httpRequest.method(); + } + + @Override + protected URI url(final Request request) throws URISyntaxException { + return new URI((request.secure() ? "https://" : "http://") + request.host() + request.uri()); + } + + @Override + protected String peerHostIP(final Request request) { + return request.remoteAddress(); + } + + @Override + protected Integer peerPort(final Request request) { + return null; + } + + @Override + protected Integer status(final Result httpResponse) { + return httpResponse.header().status(); + } + + @Override + public AgentSpan onRequest(final AgentSpan span, final Request request) { + super.onRequest(span, request); + if (request != null) { + // more about routes here: + // https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release26/migration26/Migration26.md#router-tags-are-now-attributes + final Option pathOption = request.tags().get("ROUTE_PATTERN"); + if (!pathOption.isEmpty()) { + final String path = (String) pathOption.get(); + span.setTag(DDTags.RESOURCE_NAME, request.method() + " " + path); + } + } + return span; + } + + @Override + public AgentSpan onError(final AgentSpan span, Throwable throwable) { + span.setTag(Tags.HTTP_STATUS, 500); + if (throwable != null + // This can be moved to instanceof check when using Java 8. + && throwable.getClass().getName().equals("java.util.concurrent.CompletionException") + && throwable.getCause() != null) { + throwable = throwable.getCause(); + } + while ((throwable instanceof InvocationTargetException + || throwable instanceof UndeclaredThrowableException) + && throwable.getCause() != null) { + throwable = throwable.getCause(); + } + return super.onError(span, throwable); + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java new file mode 100644 index 0000000000..cb04d49441 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java @@ -0,0 +1,40 @@ +package datadog.trace.instrumentation.play24; + +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope; +import static datadog.trace.instrumentation.play24.PlayHttpServerDecorator.DECORATE; + +import datadog.trace.context.TraceScope; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import lombok.extern.slf4j.Slf4j; +import play.api.mvc.Result; +import scala.util.Try; + +@Slf4j +public class RequestCompleteCallback extends scala.runtime.AbstractFunction1, Object> { + private final AgentSpan span; + + public RequestCompleteCallback(final AgentSpan span) { + this.span = span; + } + + @Override + public Object apply(final Try result) { + try { + if (result.isFailure()) { + DECORATE.onError(span, result.failed().get()); + } else { + DECORATE.onResponse(span, result.get()); + } + DECORATE.beforeFinish(span); + final TraceScope scope = activeScope(); + if (scope != null) { + scope.setAsyncPropagation(false); + } + } catch (final Throwable t) { + log.debug("error in play instrumentation", t); + } finally { + span.finish(); + } + return null; + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy new file mode 100644 index 0000000000..f408a5cdc6 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -0,0 +1,54 @@ +package client + +import datadog.trace.agent.test.base.HttpClientTest +import play.libs.ws.WS +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Subject + +import java.util.concurrent.TimeUnit + +// Play 2.6+ uses a separately versioned client that shades the underlying dependency +// This means our built in instrumentation won't work. +class PlayWSClientTest extends HttpClientTest { + @Subject + @Shared + @AutoCleanup + def client = WS.client() + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + def request = client.url(uri.toString()) + headers.entrySet().each { + request.setHeader(it.key, it.value) + } + + def status = request.execute(method).map({ + callback?.call() + it + }).map({ + it.status + }) + return status.get(1, TimeUnit.SECONDS) + } + + @Override + String component() { + return "" // NettyHttpClientDecorator.DECORATE.component() + } + + @Override + String expectedOperationName() { + return "netty.client.request" + } + + @Override + boolean testRedirects() { + false + } + + @Override + boolean testConnectionFailure() { + false + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java new file mode 100644 index 0000000000..61cb150a54 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/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/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy new file mode 100644 index 0000000000..38953b437d --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy @@ -0,0 +1,46 @@ +package server + +class PlayAsyncServerTest { //extends PlayServerTest { +// @Override +// Server startServer(int port) { +// def router = +// new RoutingDsl() +// .GET(SUCCESS.getPath()).routeAsync({ +// CompletableFuture.supplyAsync({ +// controller(SUCCESS) { +// Results.status(SUCCESS.getStatus(), SUCCESS.getBody()) +// } +// }, HttpExecution.defaultContext()) +// } as Supplier) +// .GET(QUERY_PARAM.getPath()).routeAsync({ +// CompletableFuture.supplyAsync({ +// controller(QUERY_PARAM) { +// Results.status(QUERY_PARAM.getStatus(), QUERY_PARAM.getBody()) +// } +// }, HttpExecution.defaultContext()) +// } as Supplier) +// .GET(REDIRECT.getPath()).routeAsync({ +// CompletableFuture.supplyAsync({ +// controller(REDIRECT) { +// Results.found(REDIRECT.getBody()) +// } +// }, HttpExecution.defaultContext()) +// } as Supplier) +// .GET(ERROR.getPath()).routeAsync({ +// CompletableFuture.supplyAsync({ +// controller(ERROR) { +// Results.status(ERROR.getStatus(), ERROR.getBody()) +// } +// }, HttpExecution.defaultContext()) +// } as Supplier) +// .GET(EXCEPTION.getPath()).routeAsync({ +// CompletableFuture.supplyAsync({ +// controller(EXCEPTION) { +// throw new Exception(EXCEPTION.getBody()) +// } +// }, HttpExecution.defaultContext()) +// } as Supplier) +// +// return Server.forRouter(router.build(), port) +// } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy new file mode 100644 index 0000000000..95fa6a0027 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -0,0 +1,79 @@ +package server + +import datadog.opentracing.DDSpan +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import datadog.trace.bootstrap.instrumentation.api.Tags +import play.api.test.TestServer + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.* + +class PlayServerTest extends HttpServerTest { + @Override + TestServer startServer(int port) { + def server = SyncServer.server(port) + server.start() + return server + } + + @Override + void stopServer(TestServer server) { + server.stop() + } + + @Override + String component() { + return "" + } + + @Override + String expectedOperationName() { + return "netty.request" + } + + @Override + boolean hasHandlerSpan() { + true + } + + @Override + // Return the handler span's name + String reorderHandlerSpan() { + "play.request" + } + + boolean testExceptionBody() { + // I can't figure out how to set a proper exception handler to customize the response body. + false + } + + @Override + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + serviceName expectedServiceName() + operationName "play.request" + spanType DDSpanTypes.HTTP_SERVER + errored endpoint == ERROR || endpoint == EXCEPTION + childOf(parent as DDSpan) + tags { + "$Tags.COMPONENT" "" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional + "$Tags.HTTP_URL" String + "$Tags.HTTP_METHOD" String + "$Tags.HTTP_STATUS" Integer + if (endpoint == ERROR) { + "$Tags.ERROR" true + } else if (endpoint == EXCEPTION) { + errorTags(Exception, EXCEPTION.body) + } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } + defaultTags() + } + } + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala new file mode 100644 index 0000000000..228dcb95a3 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala @@ -0,0 +1,5 @@ +package server + +object AsyncServer { + +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala new file mode 100644 index 0000000000..1f36d66824 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala @@ -0,0 +1,15 @@ +package server + +import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ +import play.api.mvc.{Action, Handler, Results} +import play.api.test.{FakeApplication, TestServer} + +object SyncServer { + val routes: PartialFunction[(String, String), Handler] = { + case ("GET", "/success") => Action { request => Results.Status(SUCCESS.getStatus) } + } + + def server(port: Int): TestServer = { + TestServer(port, FakeApplication(withRoutes = routes)) + } +} diff --git a/settings.gradle b/settings.gradle index e184365078..9e75b55c12 100644 --- a/settings.gradle +++ b/settings.gradle @@ -116,6 +116,7 @@ include ':dd-java-agent:instrumentation:netty-3.8' include ':dd-java-agent:instrumentation:netty-4.0' include ':dd-java-agent:instrumentation:netty-4.1' include ':dd-java-agent:instrumentation:okhttp-3' +include ':dd-java-agent:instrumentation:play-2.3' include ':dd-java-agent:instrumentation:play-2.4' include ':dd-java-agent:instrumentation:play-2.6' include ':dd-java-agent:instrumentation:play-ws' From 5cfaf8b2e57403e40399d450f9363872c38727ea Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Tue, 17 Mar 2020 16:49:19 -0400 Subject: [PATCH 02/15] Test servers now both work --- .../groovy/client/PlayWSClientTest.groovy | 2 - .../groovy/server/PlayAsyncServerTest.groovy | 52 ++++--------------- .../test/groovy/server/PlayServerTest.groovy | 5 -- .../src/test/scala/server/AsyncServer.scala | 25 ++++++++- .../src/test/scala/server/Settings.scala | 12 +++++ .../src/test/scala/server/SyncServer.scala | 8 ++- 6 files changed, 51 insertions(+), 53 deletions(-) create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/scala/server/Settings.scala diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index f408a5cdc6..2e0b8bfce9 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -8,8 +8,6 @@ import spock.lang.Subject import java.util.concurrent.TimeUnit -// Play 2.6+ uses a separately versioned client that shades the underlying dependency -// This means our built in instrumentation won't work. class PlayWSClientTest extends HttpClientTest { @Subject @Shared diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy index 38953b437d..71dfc6a9c6 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayAsyncServerTest.groovy @@ -1,46 +1,12 @@ package server -class PlayAsyncServerTest { //extends PlayServerTest { -// @Override -// Server startServer(int port) { -// def router = -// new RoutingDsl() -// .GET(SUCCESS.getPath()).routeAsync({ -// CompletableFuture.supplyAsync({ -// controller(SUCCESS) { -// Results.status(SUCCESS.getStatus(), SUCCESS.getBody()) -// } -// }, HttpExecution.defaultContext()) -// } as Supplier) -// .GET(QUERY_PARAM.getPath()).routeAsync({ -// CompletableFuture.supplyAsync({ -// controller(QUERY_PARAM) { -// Results.status(QUERY_PARAM.getStatus(), QUERY_PARAM.getBody()) -// } -// }, HttpExecution.defaultContext()) -// } as Supplier) -// .GET(REDIRECT.getPath()).routeAsync({ -// CompletableFuture.supplyAsync({ -// controller(REDIRECT) { -// Results.found(REDIRECT.getBody()) -// } -// }, HttpExecution.defaultContext()) -// } as Supplier) -// .GET(ERROR.getPath()).routeAsync({ -// CompletableFuture.supplyAsync({ -// controller(ERROR) { -// Results.status(ERROR.getStatus(), ERROR.getBody()) -// } -// }, HttpExecution.defaultContext()) -// } as Supplier) -// .GET(EXCEPTION.getPath()).routeAsync({ -// CompletableFuture.supplyAsync({ -// controller(EXCEPTION) { -// throw new Exception(EXCEPTION.getBody()) -// } -// }, HttpExecution.defaultContext()) -// } as Supplier) -// -// return Server.forRouter(router.build(), port) -// } +import play.api.test.TestServer + +class PlayAsyncServerTest extends PlayServerTest { + @Override + TestServer startServer(int port) { + def server = AsyncServer.server(port) + server.start() + return server + } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy index 95fa6a0027..f6a733e58d 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -44,11 +44,6 @@ class PlayServerTest extends HttpServerTest { "play.request" } - boolean testExceptionBody() { - // I can't figure out how to set a proper exception handler to customize the response body. - false - } - @Override void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { trace.span(index) { diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala index 228dcb95a3..087f520a85 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala @@ -1,5 +1,28 @@ package server -object AsyncServer { +import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ +import play.api.libs.concurrent.Execution.{defaultContext => ec} +import play.api.mvc.{Action, Handler, Results} +import play.api.test.{FakeApplication, TestServer} +import scala.concurrent.Future + +object AsyncServer { + val routes: PartialFunction[(String, String), Handler] = { + case ("GET", "/success") => Action.async { request => Future.successful(Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody)) } + case ("GET", "/redirect") => Action.async { request => Future.successful(Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus)) } + case ("GET", "/query") => Action.async { result => Future.successful(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody)) } + case ("GET", "/error-status") => Action.async { result => Future { + throw new RuntimeException(ERROR.getBody) + }(ec) + } + case ("GET", "/exception") => Action.async { result => Future { + throw new RuntimeException(ERROR.getBody) + }(ec) + } + } + + def server(port: Int): TestServer = { + TestServer(port, FakeApplication(withGlobal = Some(new Settings()), withRoutes = routes)) + } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/Settings.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/Settings.scala new file mode 100644 index 0000000000..04add7c804 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/Settings.scala @@ -0,0 +1,12 @@ +package server + +import play.api.GlobalSettings +import play.api.mvc.{RequestHeader, Result, Results} + +import scala.concurrent.Future + +class Settings extends GlobalSettings { + override def onError(request: RequestHeader, ex: Throwable): Future[Result] = { + Future.successful(Results.InternalServerError(ex.getCause.getMessage)) + } +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala index 1f36d66824..699a900164 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala @@ -6,10 +6,14 @@ import play.api.test.{FakeApplication, TestServer} object SyncServer { val routes: PartialFunction[(String, String), Handler] = { - case ("GET", "/success") => Action { request => Results.Status(SUCCESS.getStatus) } + case ("GET", "/success") => Action { request => Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody) } + case ("GET", "/redirect") => Action { request => Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus) } + case ("GET", "/query") => Action { result => Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody) } + case ("GET", "/error-status") => Action { result => throw new RuntimeException(ERROR.getBody) } + case ("GET", "/exception") => Action { result => throw new RuntimeException(EXCEPTION.getBody) } } def server(port: Int): TestServer = { - TestServer(port, FakeApplication(withRoutes = routes)) + TestServer(port, FakeApplication(withGlobal = Some(new Settings()), withRoutes = routes)) } } From d1093a79b8e5d412dcdf51c4116874777dc39e1a Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Tue, 17 Mar 2020 16:52:50 -0400 Subject: [PATCH 03/15] rename package --- dd-java-agent/instrumentation/play-2.3/play-2.3.gradle | 2 +- .../{play24 => play23}/PlayInstrumentation.java | 2 +- .../instrumentation/{play24 => play23}/PlayAdvice.java | 7 +++---- .../instrumentation/{play24 => play23}/PlayHeaders.java | 2 +- .../{play24 => play23}/PlayHttpServerDecorator.java | 2 +- .../{play24 => play23}/RequestCompleteCallback.java | 6 +++--- 6 files changed, 10 insertions(+), 11 deletions(-) rename dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/{play24 => play23}/PlayInstrumentation.java (97%) rename dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/{play24 => play23}/PlayAdvice.java (93%) rename dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/{play24 => play23}/PlayHeaders.java (93%) rename dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/{play24 => play23}/PlayHttpServerDecorator.java (98%) rename dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/{play24 => play23}/RequestCompleteCallback.java (90%) diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index e77bad0dea..cbe2b9a76e 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -8,7 +8,7 @@ muzzle { pass { group = 'com.typesafe.play' module = 'play_2.11' - versions = '[2.3.0,2.4)' + versions = '[2.3.9,2.4)' assertInverse = true } fail { diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java similarity index 97% rename from dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java rename to dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java index fa7d265158..a8ac6a72d5 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play24/PlayInstrumentation.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java @@ -1,4 +1,4 @@ -package datadog.trace.instrumentation.play24; +package datadog.trace.instrumentation.play23; import static datadog.trace.agent.tooling.ClassLoaderMatcher.hasClassesNamed; import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.implementsInterface; diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java similarity index 93% rename from dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java rename to dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java index 2e495f2e72..924dbde45f 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayAdvice.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java @@ -1,11 +1,10 @@ -package datadog.trace.instrumentation.play24; +package datadog.trace.instrumentation.play23; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.propagate; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; -import static datadog.trace.instrumentation.play24.PlayHeaders.GETTER; -import static datadog.trace.instrumentation.play24.PlayHttpServerDecorator.DECORATE; +import static datadog.trace.instrumentation.play23.PlayHttpServerDecorator.DECORATE; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; @@ -22,7 +21,7 @@ public class PlayAdvice { public static AgentScope onEnter(@Advice.Argument(0) final Request req) { final AgentSpan span; if (activeSpan() == null) { - final Context extractedContext = propagate().extract(req.headers(), GETTER); + final Context extractedContext = propagate().extract(req.headers(), PlayHeaders.GETTER); span = startSpan("play.request", extractedContext); } else { // An upstream framework (e.g. akka-http, netty) has already started the span. diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHeaders.java similarity index 93% rename from dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java rename to dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHeaders.java index fd8c22ea83..a0c33050be 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHeaders.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHeaders.java @@ -1,4 +1,4 @@ -package datadog.trace.instrumentation.play24; +package datadog.trace.instrumentation.play23; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import play.api.mvc.Headers; diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHttpServerDecorator.java similarity index 98% rename from dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java rename to dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHttpServerDecorator.java index 6c12b2f301..09878f9ba0 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/PlayHttpServerDecorator.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayHttpServerDecorator.java @@ -1,4 +1,4 @@ -package datadog.trace.instrumentation.play24; +package datadog.trace.instrumentation.play23; import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/RequestCompleteCallback.java similarity index 90% rename from dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java rename to dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/RequestCompleteCallback.java index cb04d49441..e1c5ff9dc3 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play24/RequestCompleteCallback.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/RequestCompleteCallback.java @@ -1,10 +1,10 @@ -package datadog.trace.instrumentation.play24; +package datadog.trace.instrumentation.play23; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope; -import static datadog.trace.instrumentation.play24.PlayHttpServerDecorator.DECORATE; +import static datadog.trace.instrumentation.play23.PlayHttpServerDecorator.DECORATE; -import datadog.trace.context.TraceScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.context.TraceScope; import lombok.extern.slf4j.Slf4j; import play.api.mvc.Result; import scala.util.Try; From 89a0761ab4b0660d24b77abe8f5aaab640503c31 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Wed, 18 Mar 2020 10:26:55 -0400 Subject: [PATCH 04/15] Add typesafe repo and skip bad builds --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 15 ++++++++------- .../instrumentation/play-2.3/play-2.3.gradle | 10 +++++----- gradle/java.gradle | 3 +++ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index f8221de0d4..d8180344d8 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -14,16 +14,13 @@ import org.eclipse.aether.spi.connector.RepositoryConnectorFactory import org.eclipse.aether.spi.connector.transport.TransporterFactory import org.eclipse.aether.transport.http.HttpTransporterFactory import org.eclipse.aether.version.Version -import org.gradle.api.Action -import org.gradle.api.GradleException -import org.gradle.api.Plugin -import org.gradle.api.Project -import org.gradle.api.Task +import org.gradle.api.* import org.gradle.api.model.ObjectFactory import java.lang.reflect.Method import java.security.SecureClassLoader import java.util.concurrent.atomic.AtomicReference +import java.util.regex.Pattern /** * muzzle task plugin which runs muzzle validation against a range of dependencies. @@ -36,7 +33,8 @@ class MuzzlePlugin implements Plugin { private static final AtomicReference TOOLING_LOADER = new AtomicReference<>() static { RemoteRepository central = new RemoteRepository.Builder("central", "default", "https://repo1.maven.org/maven2/").build() - MUZZLE_REPOS = new ArrayList(Arrays.asList(central)) + RemoteRepository typesafe = new RemoteRepository.Builder("typesafe", "default", "https://repo.typesafe.com/typesafe/releases").build() + MUZZLE_REPOS = new ArrayList(Arrays.asList(central, typesafe)) } @Override @@ -343,6 +341,8 @@ class MuzzlePlugin implements Plugin { return session } + private static final Pattern GIT_SHA_PATTERN = Pattern.compile('^.*-[0-9a-f]{7,}$') + /** * Filter out snapshot-type builds from versions list. */ @@ -357,7 +357,8 @@ class MuzzlePlugin implements Plugin { version.contains(".m") || version.contains("-m") || version.contains("-dev") || - version.contains("public_draft") + version.contains("public_draft") || + version.matches(GIT_SHA_PATTERN) } return list } diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index cbe2b9a76e..7614f1d01b 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -8,7 +8,7 @@ muzzle { pass { group = 'com.typesafe.play' module = 'play_2.11' - versions = '[2.3.9,2.4)' + versions = '[2.3.0,2.4)' assertInverse = true } fail { @@ -35,11 +35,11 @@ testSets { } dependencies { - main_java8Compile group: 'com.typesafe.play', name: 'play_2.11', version: '2.3.9' + main_java8Compile group: 'com.typesafe.play', name: 'play_2.11', version: '2.3.0' - testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.9' - testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.9' - testCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.9') + testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.0' + testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.0' + testCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.0') latestDepTestCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.+' latestDepTestCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.+' diff --git a/gradle/java.gradle b/gradle/java.gradle index d2ab140903..5f3e53061f 100644 --- a/gradle/java.gradle +++ b/gradle/java.gradle @@ -131,6 +131,9 @@ repositories { maven { url "https://adoptopenjdk.jfrog.io/adoptopenjdk/jmc-libs-snapshots" } + maven { + url "https://repo.typesafe.com/typesafe/releases" + } } dependencies { From 485d9cb6c26874663e93171c28bcb72ce182a56b Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Thu, 19 Mar 2020 16:49:38 -0400 Subject: [PATCH 05/15] Work so far --- .../play23/PlayInstrumentation.java | 2 +- .../instrumentation/play23/PlayAdvice.java | 5 ---- .../NettyServerTestInstrumentation.java | 5 ++-- .../test/groovy/server/PlayServerTest.groovy | 8 +++--- .../server/ControllerClosureAdapter.scala | 18 +++++++++++++ .../src/test/scala/server/SyncServer.scala | 25 +++++++++++++++---- .../agent/test/base/HttpServerTest.groovy | 9 ++----- .../trace/common/writer/ListWriter.java | 7 ++++++ 8 files changed, 56 insertions(+), 23 deletions(-) create mode 100644 dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java index a8ac6a72d5..1a7ca983e9 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java @@ -18,7 +18,7 @@ import net.bytebuddy.matcher.ElementMatcher; public final class PlayInstrumentation extends Instrumenter.Default { public PlayInstrumentation() { - super("play"); + super("play", "play-action"); } @Override diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java index 924dbde45f..5c1de38c6b 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java @@ -64,9 +64,4 @@ public class PlayAdvice { // set the resource name on the upstream akka/netty span DECORATE.onRequest(rootSpan, req); } - - // Unused method for muzzle to allow only 2.4-2.5 - public static void muzzleCheck() { - play.libs.Akka.system(); - } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java index 61cb150a54..b94ea96264 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/NettyServerTestInstrumentation.java @@ -13,10 +13,11 @@ public class NettyServerTestInstrumentation implements Instrumenter { @Override public AgentBuilder instrument(final AgentBuilder agentBuilder) { return agentBuilder - .type(named("io.netty.handler.codec.ByteToMessageDecoder")) + .type(named("org.jboss.netty.handler.codec.http.HttpRequestDecoder")) .transform( new AgentBuilder.Transformer.ForAdvice() .advice( - named("channelRead"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + named("createMessage"), + HttpServerTestAdvice.ServerEntryAdvice.class.getName())); } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy index f6a733e58d..32aa2e83c9 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -6,6 +6,7 @@ import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import datadog.trace.bootstrap.instrumentation.api.Tags +import datadog.trace.instrumentation.play23.PlayHttpServerDecorator import play.api.test.TestServer import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.* @@ -25,14 +26,15 @@ class PlayServerTest extends HttpServerTest { @Override String component() { - return "" + return "netty" } @Override String expectedOperationName() { - return "netty.request" + return "play.server" } + // We don't have instrumentation for this version of netty yet @Override boolean hasHandlerSpan() { true @@ -53,7 +55,7 @@ class PlayServerTest extends HttpServerTest { errored endpoint == ERROR || endpoint == EXCEPTION childOf(parent as DDSpan) tags { - "$Tags.COMPONENT" "" + "$Tags.COMPONENT" PlayHttpServerDecorator.DECORATE.component() "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional "$Tags.HTTP_URL" String diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala new file mode 100644 index 0000000000..bf800b1c87 --- /dev/null +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala @@ -0,0 +1,18 @@ +package server + +import groovy.lang.Closure +import play.api.mvc.Result + +import scala.concurrent.Future + +class ControllerClosureAdapter(response: Result) extends Closure[Result] { + override def call(): Result = response +} + +class BlockClosureAdapter(block: () => Result) extends Closure[Result] { + override def call(): Result = block() +} + +class AsyncControllerClosureAdapter(response: Future[Result]) extends Closure[Future[Result]] { + override def call(): Future[Result] = response +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala index 699a900164..fad6181a13 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala @@ -1,16 +1,31 @@ package server +import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ import play.api.mvc.{Action, Handler, Results} import play.api.test.{FakeApplication, TestServer} object SyncServer { val routes: PartialFunction[(String, String), Handler] = { - case ("GET", "/success") => Action { request => Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody) } - case ("GET", "/redirect") => Action { request => Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus) } - case ("GET", "/query") => Action { result => Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody) } - case ("GET", "/error-status") => Action { result => throw new RuntimeException(ERROR.getBody) } - case ("GET", "/exception") => Action { result => throw new RuntimeException(EXCEPTION.getBody) } + case ("GET", "/success") => Action { request => + HttpServerTest.controller(SUCCESS, new ControllerClosureAdapter(Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody))) + } + case ("GET", "/redirect") => Action { request => + HttpServerTest.controller(REDIRECT, new ControllerClosureAdapter(Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus))) + } + case ("GET", "/query") => Action { request => + HttpServerTest.controller(QUERY_PARAM, new ControllerClosureAdapter(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody))) + } + case ("GET", "/error-status") => Action { request => + HttpServerTest.controller(ERROR, new BlockClosureAdapter(() => { + throw new RuntimeException(ERROR.getBody) + })) + } + case ("GET", "/exception") => Action { request => + HttpServerTest.controller(EXCEPTION, new BlockClosureAdapter(() => { + throw new RuntimeException(EXCEPTION.getBody) + })) + } } def server(port: Int): TestServer = { 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 16990954b1..3968ce78f6 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -24,12 +24,7 @@ import spock.lang.Unroll import java.util.concurrent.atomic.AtomicBoolean import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.* import static datadog.trace.agent.test.utils.ConfigUtils.withConfigOverride import static datadog.trace.agent.test.utils.TraceUtils.basicSpan import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace @@ -533,7 +528,7 @@ abstract class HttpServerTest extends AgentTestRunner { tags { "$Tags.COMPONENT" component "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER - "$Tags.PEER_PORT" Integer + "$Tags.PEER_PORT" { it == null || it instanceof Integer } // Optional "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional "$Tags.HTTP_URL" "${endpoint.resolve(address)}" "$Tags.HTTP_METHOD" method diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java index a92343350d..b1c8e06c68 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java @@ -39,6 +39,13 @@ public class ListWriter extends CopyOnWriteArrayList> implements Wr latches.add(latch); } if (!latch.await(20, TimeUnit.SECONDS)) { + System.out.println(this); + for (final List trace : this) { + System.out.println(trace.toString()); + for (final DDSpan span : trace) { + System.out.println(span.toString()); + } + } throw new TimeoutException( "Timeout waiting for " + number + " trace(s). ListWriter.size() == " + size()); } From 7f7b7d41cd5c4bdc1bce2f15cd2b63487518b300 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Fri, 20 Mar 2020 06:51:22 -0400 Subject: [PATCH 06/15] Depend on netty 3.9 instrumentation --- .../instrumentation/play-2.3/play-2.3.gradle | 6 ++++-- .../test/groovy/server/PlayServerTest.groovy | 5 +++-- .../src/test/scala/server/AsyncServer.scala | 20 +++++++++---------- .../server/ControllerClosureAdapter.scala | 4 ++++ .../src/test/scala/server/SyncServer.scala | 6 ++---- .../agent/test/base/HttpServerTest.groovy | 9 +++++++-- .../trace/common/writer/ListWriter.java | 7 ------- 7 files changed, 29 insertions(+), 28 deletions(-) diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index 7614f1d01b..1bf70fd0c2 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -37,11 +37,13 @@ testSets { dependencies { main_java8Compile group: 'com.typesafe.play', name: 'play_2.11', version: '2.3.0' + testCompile project(':dd-java-agent:instrumentation:netty-3.9') + testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.0' testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.0' - testCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.0') + testCompile group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.0' latestDepTestCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.+' latestDepTestCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.+' - latestDepTestCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.+') + latestDepTestCompile group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.+' } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy index 32aa2e83c9..9267d0b9c7 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -6,6 +6,7 @@ import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import datadog.trace.bootstrap.instrumentation.api.Tags +import datadog.trace.instrumentation.netty39.server.NettyHttpServerDecorator import datadog.trace.instrumentation.play23.PlayHttpServerDecorator import play.api.test.TestServer @@ -26,12 +27,12 @@ class PlayServerTest extends HttpServerTest { @Override String component() { - return "netty" + return NettyHttpServerDecorator.DECORATE.component() } @Override String expectedOperationName() { - return "play.server" + return "netty.request" } // We don't have instrumentation for this version of netty yet diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala index 087f520a85..a1789635a0 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/AsyncServer.scala @@ -1,7 +1,7 @@ package server +import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ -import play.api.libs.concurrent.Execution.{defaultContext => ec} import play.api.mvc.{Action, Handler, Results} import play.api.test.{FakeApplication, TestServer} @@ -9,16 +9,14 @@ import scala.concurrent.Future object AsyncServer { val routes: PartialFunction[(String, String), Handler] = { - case ("GET", "/success") => Action.async { request => Future.successful(Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody)) } - case ("GET", "/redirect") => Action.async { request => Future.successful(Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus)) } - case ("GET", "/query") => Action.async { result => Future.successful(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody)) } - case ("GET", "/error-status") => Action.async { result => Future { - throw new RuntimeException(ERROR.getBody) - }(ec) - } - case ("GET", "/exception") => Action.async { result => Future { - throw new RuntimeException(ERROR.getBody) - }(ec) + case ("GET", "/success") => Action.async { request => HttpServerTest.controller(SUCCESS, new AsyncControllerClosureAdapter(Future.successful(Results.Status(SUCCESS.getStatus).apply(SUCCESS.getBody)))) } + case ("GET", "/redirect") => Action.async { request => HttpServerTest.controller(REDIRECT, new AsyncControllerClosureAdapter(Future.successful(Results.Redirect(REDIRECT.getBody, REDIRECT.getStatus)))) } + case ("GET", "/query") => Action.async { result => HttpServerTest.controller(QUERY_PARAM, new AsyncControllerClosureAdapter(Future.successful(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody)))) } + case ("GET", "/error-status") => Action.async { result => HttpServerTest.controller(ERROR, new AsyncControllerClosureAdapter(Future.successful(Results.Status(ERROR.getStatus).apply(ERROR.getBody)))) } + case ("GET", "/exception") => Action.async { result => + HttpServerTest.controller(EXCEPTION, new AsyncBlockClosureAdapter(() => { + throw new Exception(EXCEPTION.getBody) + })) } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala index bf800b1c87..8cf3766917 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/ControllerClosureAdapter.scala @@ -16,3 +16,7 @@ class BlockClosureAdapter(block: () => Result) extends Closure[Result] { class AsyncControllerClosureAdapter(response: Future[Result]) extends Closure[Future[Result]] { override def call(): Future[Result] = response } + +class AsyncBlockClosureAdapter(block: () => Future[Result]) extends Closure[Future[Result]] { + override def call(): Future[Result] = block() +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala index fad6181a13..2565127ecb 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala @@ -17,13 +17,11 @@ object SyncServer { HttpServerTest.controller(QUERY_PARAM, new ControllerClosureAdapter(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody))) } case ("GET", "/error-status") => Action { request => - HttpServerTest.controller(ERROR, new BlockClosureAdapter(() => { - throw new RuntimeException(ERROR.getBody) - })) + HttpServerTest.controller(QUERY_PARAM, new ControllerClosureAdapter(Results.Status(ERROR.getStatus).apply(ERROR.getBody))) } case ("GET", "/exception") => Action { request => HttpServerTest.controller(EXCEPTION, new BlockClosureAdapter(() => { - throw new RuntimeException(EXCEPTION.getBody) + throw new Exception(EXCEPTION.getBody) })) } } 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 3968ce78f6..16990954b1 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -24,7 +24,12 @@ import spock.lang.Unroll import java.util.concurrent.atomic.AtomicBoolean import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.* +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS import static datadog.trace.agent.test.utils.ConfigUtils.withConfigOverride import static datadog.trace.agent.test.utils.TraceUtils.basicSpan import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace @@ -528,7 +533,7 @@ abstract class HttpServerTest extends AgentTestRunner { tags { "$Tags.COMPONENT" component "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER - "$Tags.PEER_PORT" { it == null || it instanceof Integer } // Optional + "$Tags.PEER_PORT" Integer "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional "$Tags.HTTP_URL" "${endpoint.resolve(address)}" "$Tags.HTTP_METHOD" method diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java index b1c8e06c68..a92343350d 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java @@ -39,13 +39,6 @@ public class ListWriter extends CopyOnWriteArrayList> implements Wr latches.add(latch); } if (!latch.await(20, TimeUnit.SECONDS)) { - System.out.println(this); - for (final List trace : this) { - System.out.println(trace.toString()); - for (final DDSpan span : trace) { - System.out.println(span.toString()); - } - } throw new TimeoutException( "Timeout waiting for " + number + " trace(s). ListWriter.size() == " + size()); } From 879d84a162d71b051aed4e64eda5ac3b39850d96 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Fri, 20 Mar 2020 07:58:55 -0400 Subject: [PATCH 07/15] Try to fix client test, not quite --- .../instrumentation/play-2.3/play-2.3.gradle | 8 ++++-- .../groovy/client/PlayWSClientTest.groovy | 26 ++++++++++++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index 1bf70fd0c2..1e5c01151b 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -41,9 +41,13 @@ dependencies { testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.0' testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.0' - testCompile group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.0' + testCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.0') { + exclude group: 'org.eclipse.jetty', module: 'jetty-websocket' + } latestDepTestCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.+' latestDepTestCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.+' - latestDepTestCompile group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.+' + latestDepTestCompile(group: 'com.typesafe.play', name: 'play-test_2.11', version: '2.3.+') { + exclude group: 'org.eclipse.jetty', module: 'jetty-websocket' + } } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index 2e0b8bfce9..e9165f086b 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -1,21 +1,35 @@ package client import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.netty39.client.NettyHttpClientDecorator +import play.GlobalSettings import play.libs.ws.WS -import spock.lang.AutoCleanup +import play.test.FakeApplication +import play.test.Helpers import spock.lang.Shared -import spock.lang.Subject import java.util.concurrent.TimeUnit class PlayWSClientTest extends HttpClientTest { - @Subject @Shared - @AutoCleanup - def client = WS.client() + def application = new FakeApplication(new File("."), FakeApplication.class.getClassLoader(), Collections.emptyMap(), Collections.emptyList(), new GlobalSettings()) + +// @Subject +// @Shared +// @AutoCleanup +// def client = WS.client() + + def setup() { + Helpers.start(application) + } + + def cleanup() { + Helpers.stop(application) + } @Override int doRequest(String method, URI uri, Map headers, Closure callback) { + def client = WS.client() def request = client.url(uri.toString()) headers.entrySet().each { request.setHeader(it.key, it.value) @@ -32,7 +46,7 @@ class PlayWSClientTest extends HttpClientTest { @Override String component() { - return "" // NettyHttpClientDecorator.DECORATE.component() + return NettyHttpClientDecorator.DECORATE.component() } @Override From 6fa94ed9ac32ef275a89688a22d0ba33366f0342 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Fri, 20 Mar 2020 15:14:34 -0400 Subject: [PATCH 08/15] Update to new module name --- dd-java-agent/instrumentation/play-2.3/play-2.3.gradle | 2 +- .../src/test/groovy/client/PlayWSClientTest.groovy | 7 +------ .../play-2.3/src/test/groovy/server/PlayServerTest.groovy | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index 1e5c01151b..fc88656192 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -37,7 +37,7 @@ testSets { dependencies { main_java8Compile group: 'com.typesafe.play', name: 'play_2.11', version: '2.3.0' - testCompile project(':dd-java-agent:instrumentation:netty-3.9') + testCompile project(':dd-java-agent:instrumentation:netty-3.8') testCompile group: 'com.typesafe.play', name: 'play-java_2.11', version: '2.3.0' testCompile group: 'com.typesafe.play', name: 'play-java-ws_2.11', version: '2.3.0' diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index e9165f086b..e254806bb4 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -1,7 +1,7 @@ package client import datadog.trace.agent.test.base.HttpClientTest -import datadog.trace.instrumentation.netty39.client.NettyHttpClientDecorator +import datadog.trace.instrumentation.netty38.client.NettyHttpClientDecorator import play.GlobalSettings import play.libs.ws.WS import play.test.FakeApplication @@ -14,11 +14,6 @@ class PlayWSClientTest extends HttpClientTest { @Shared def application = new FakeApplication(new File("."), FakeApplication.class.getClassLoader(), Collections.emptyMap(), Collections.emptyList(), new GlobalSettings()) -// @Subject -// @Shared -// @AutoCleanup -// def client = WS.client() - def setup() { Helpers.start(application) } diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy index 9267d0b9c7..c8ce2c32c5 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -6,7 +6,7 @@ import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import datadog.trace.bootstrap.instrumentation.api.Tags -import datadog.trace.instrumentation.netty39.server.NettyHttpServerDecorator +import datadog.trace.instrumentation.netty38.server.NettyHttpServerDecorator import datadog.trace.instrumentation.play23.PlayHttpServerDecorator import play.api.test.TestServer From 661a8f5a3ecae74f56ae768750f7af7fa31b2e1a Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 23 Mar 2020 14:01:57 -0400 Subject: [PATCH 09/15] Fix the latestdeptests --- dd-java-agent/instrumentation/play-2.3/play-2.3.gradle | 5 +++++ .../src/test/groovy/client/PlayWSClientTest.groovy | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle index fc88656192..4e74546bc0 100644 --- a/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle +++ b/dd-java-agent/instrumentation/play-2.3/play-2.3.gradle @@ -51,3 +51,8 @@ dependencies { exclude group: 'org.eclipse.jetty', module: 'jetty-websocket' } } + +compileLatestDepTestGroovy { + classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) + dependsOn compileLatestDepTestScala +} diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index e254806bb4..9d8c119f2c 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -12,7 +12,13 @@ import java.util.concurrent.TimeUnit class PlayWSClientTest extends HttpClientTest { @Shared - def application = new FakeApplication(new File("."), FakeApplication.class.getClassLoader(), Collections.emptyMap(), Collections.emptyList(), new GlobalSettings()) + def application = new FakeApplication( + new File("."), + FakeApplication.class.getClassLoader(), + Collections.emptyMap(), + Collections.emptyList(), + new GlobalSettings() + ) def setup() { Helpers.start(application) From 8dd5993a8fb74c890cc1442f74edd3feed0ff1a3 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 23 Mar 2020 15:08:04 -0400 Subject: [PATCH 10/15] Fix codenarc --- .../play-2.3/src/test/groovy/client/PlayWSClientTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index 9d8c119f2c..12ddf851b3 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -14,7 +14,7 @@ class PlayWSClientTest extends HttpClientTest { @Shared def application = new FakeApplication( new File("."), - FakeApplication.class.getClassLoader(), + FakeApplication.getClassLoader(), Collections.emptyMap(), Collections.emptyList(), new GlobalSettings() From d3def8ce2d892ff2a6a49a6e4767c8acfc619e94 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 23 Mar 2020 15:11:10 -0400 Subject: [PATCH 11/15] :( --- .../play-2.3/src/test/groovy/client/PlayWSClientTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index 12ddf851b3..fd7085437d 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -20,11 +20,11 @@ class PlayWSClientTest extends HttpClientTest { new GlobalSettings() ) - def setup() { + def setupSpec() { Helpers.start(application) } - def cleanup() { + def cleanupSpec() { Helpers.stop(application) } From acba3626dce035df543098252feabca06d8dce14 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 23 Mar 2020 15:59:06 -0400 Subject: [PATCH 12/15] Missed a static import --- .../java8/datadog/trace/instrumentation/play23/PlayAdvice.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java index 5c1de38c6b..7108037f96 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java +++ b/dd-java-agent/instrumentation/play-2.3/src/main/java8/datadog/trace/instrumentation/play23/PlayAdvice.java @@ -4,6 +4,7 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSp import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.propagate; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.play23.PlayHeaders.GETTER; import static datadog.trace.instrumentation.play23.PlayHttpServerDecorator.DECORATE; import datadog.trace.bootstrap.instrumentation.api.AgentScope; @@ -21,7 +22,7 @@ public class PlayAdvice { public static AgentScope onEnter(@Advice.Argument(0) final Request req) { final AgentSpan span; if (activeSpan() == null) { - final Context extractedContext = propagate().extract(req.headers(), PlayHeaders.GETTER); + final Context extractedContext = propagate().extract(req.headers(), GETTER); span = startSpan("play.request", extractedContext); } else { // An upstream framework (e.g. akka-http, netty) has already started the span. From 08e9f8c6e2a1b5d379ca07160b67b6960a783aae Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 23 Mar 2020 16:02:02 -0400 Subject: [PATCH 13/15] Undo star import --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index d8180344d8..71420ccb22 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -14,7 +14,11 @@ import org.eclipse.aether.spi.connector.RepositoryConnectorFactory import org.eclipse.aether.spi.connector.transport.TransporterFactory import org.eclipse.aether.transport.http.HttpTransporterFactory import org.eclipse.aether.version.Version -import org.gradle.api.* +import org.gradle.api.Action +import org.gradle.api.GradleException +import org.gradle.api.Plugin +import org.gradle.api.Project +import org.gradle.api.Task import org.gradle.api.model.ObjectFactory import java.lang.reflect.Method From 026072b866806b2acd207bda83f147e2f785b138 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Wed, 25 Mar 2020 12:57:00 -0400 Subject: [PATCH 14/15] Wrong endpoint --- .../play-2.3/src/test/scala/server/SyncServer.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala index 2565127ecb..602a14c70e 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala +++ b/dd-java-agent/instrumentation/play-2.3/src/test/scala/server/SyncServer.scala @@ -17,7 +17,7 @@ object SyncServer { HttpServerTest.controller(QUERY_PARAM, new ControllerClosureAdapter(Results.Status(QUERY_PARAM.getStatus).apply(QUERY_PARAM.getBody))) } case ("GET", "/error-status") => Action { request => - HttpServerTest.controller(QUERY_PARAM, new ControllerClosureAdapter(Results.Status(ERROR.getStatus).apply(ERROR.getBody))) + HttpServerTest.controller(ERROR, new ControllerClosureAdapter(Results.Status(ERROR.getStatus).apply(ERROR.getBody))) } case ("GET", "/exception") => Action { request => HttpServerTest.controller(EXCEPTION, new BlockClosureAdapter(() => { From fbb43840d39ee5385413505e623caf175656e684 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Thu, 26 Mar 2020 14:29:35 -0400 Subject: [PATCH 15/15] Fix new client tests --- .../groovy/client/PlayWSClientTest.groovy | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy index fd7085437d..a72b00c7b3 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/client/PlayWSClientTest.groovy @@ -15,13 +15,20 @@ class PlayWSClientTest extends HttpClientTest { def application = new FakeApplication( new File("."), FakeApplication.getClassLoader(), - Collections.emptyMap(), + [ + "ws.timeout.connection": CONNECT_TIMEOUT_MS, + "ws.timeout.request" : READ_TIMEOUT_MS + ], Collections.emptyList(), new GlobalSettings() ) + @Shared + def client + def setupSpec() { Helpers.start(application) + client = WS.client() } def cleanupSpec() { @@ -30,7 +37,6 @@ class PlayWSClientTest extends HttpClientTest { @Override int doRequest(String method, URI uri, Map headers, Closure callback) { - def client = WS.client() def request = client.url(uri.toString()) headers.entrySet().each { request.setHeader(it.key, it.value) @@ -64,4 +70,13 @@ class PlayWSClientTest extends HttpClientTest { boolean testConnectionFailure() { false } + + @Override + boolean testRemoteConnection() { + // On connection failures the operation and resource names end up different from expected. + // This would require a lot of changes to the base client test class to support + // span.operationName = "netty.connect" + // span.resourceName = "netty.connect" + false + } }