From 44b63ea5cadac242ed25440c6ab4844d393fa59f Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 15:41:59 -0400 Subject: [PATCH 1/2] Rename AkkaHttpInstrumentation to AkkaHttpServerInstrumentation To be able to put client instrumentation in more consistent way --- ...ava => AkkaHttpServerInstrumentation.java} | 85 ++++++++++--------- ... AkkaHttpServerInstrumentationTest.groovy} | 60 ++++++------- 2 files changed, 74 insertions(+), 71 deletions(-) rename dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/{AkkaHttpInstrumentation.java => AkkaHttpServerInstrumentation.java} (80%) rename dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/{AkkaHttpInstrumentationTest.groovy => AkkaHttpServerInstrumentationTest.groovy} (72%) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java similarity index 80% rename from dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpInstrumentation.java rename to dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java index 902168bd85..e9a7e655c9 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java @@ -6,7 +6,6 @@ import akka.http.javadsl.model.HttpHeader; import akka.http.scaladsl.model.HttpRequest; import akka.http.scaladsl.model.HttpResponse; import akka.stream.*; -import akka.stream.stage.*; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.*; import datadog.trace.api.DDSpanTypes; @@ -33,8 +32,8 @@ import scala.runtime.AbstractFunction1; @Slf4j @AutoService(Instrumenter.class) -public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { - public AkkaHttpInstrumentation() { +public final class AkkaHttpServerInstrumentation extends Instrumenter.Configurable { + public AkkaHttpServerInstrumentation() { super("akka-http", "akka-http-server"); } @@ -45,11 +44,12 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { private static final HelperInjector akkaHttpHelperInjector = new HelperInjector( - AkkaHttpInstrumentation.class.getName() + "$DatadogSyncWrapper", - AkkaHttpInstrumentation.class.getName() + "$DatadogAsyncWrapper", - AkkaHttpInstrumentation.class.getName() + "$DatadogAsyncWrapper$1", - AkkaHttpInstrumentation.class.getName() + "$DatadogAsyncWrapper$2", - AkkaHttpInstrumentation.class.getName() + "$AkkaHttpHeaders"); + AkkaHttpServerInstrumentation.class.getName() + "$DatadogWrapperHelper", + AkkaHttpServerInstrumentation.class.getName() + "$DatadogSyncWrapper", + AkkaHttpServerInstrumentation.class.getName() + "$DatadogAsyncWrapper", + AkkaHttpServerInstrumentation.class.getName() + "$DatadogAsyncWrapper$1", + AkkaHttpServerInstrumentation.class.getName() + "$DatadogAsyncWrapper$2", + AkkaHttpServerInstrumentation.class.getName() + "$AkkaHttpServerHeaders"); @Override public AgentBuilder apply(final AgentBuilder agentBuilder) { @@ -57,9 +57,9 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { .type(named("akka.http.scaladsl.HttpExt")) .transform(DDTransformers.defaultTransformers()) .transform(akkaHttpHelperInjector) - // Insturmenting akka-streams bindAndHandle api was previously attempted. + // Instrumenting akka-streams bindAndHandle api was previously attempted. // This proved difficult as there was no clean way to close the async scope - // in the graph logic after the user's requst handler completes. + // in the graph logic after the user's request handler completes. // // Instead, we're instrumenting the bindAndHandle function helpers by // wrapping the scala functions with our own handlers. @@ -95,31 +95,11 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { } } - public static class DatadogSyncWrapper extends AbstractFunction1 { - private final Function1 userHandler; - - public DatadogSyncWrapper(Function1 userHandler) { - this.userHandler = userHandler; - } - - @Override - public HttpResponse apply(HttpRequest request) { - final Scope scope = DatadogSyncWrapper.createSpan(request); - try { - final HttpResponse response = userHandler.apply(request); - scope.close(); - finishSpan(scope.span(), response); - return response; - } catch (Throwable t) { - scope.close(); - finishSpan(scope.span(), t); - throw t; - } - } - + public static class DatadogWrapperHelper { public static Scope createSpan(HttpRequest request) { final SpanContext extractedContext = - GlobalTracer.get().extract(Format.Builtin.HTTP_HEADERS, new AkkaHttpHeaders(request)); + GlobalTracer.get() + .extract(Format.Builtin.HTTP_HEADERS, new AkkaHttpServerHeaders(request)); final Scope scope = GlobalTracer.get() .buildSpan("akka-http.request") @@ -158,6 +138,29 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { } } + public static class DatadogSyncWrapper extends AbstractFunction1 { + private final Function1 userHandler; + + public DatadogSyncWrapper(Function1 userHandler) { + this.userHandler = userHandler; + } + + @Override + public HttpResponse apply(HttpRequest request) { + final Scope scope = DatadogWrapperHelper.createSpan(request); + try { + final HttpResponse response = userHandler.apply(request); + scope.close(); + DatadogWrapperHelper.finishSpan(scope.span(), response); + return response; + } catch (Throwable t) { + scope.close(); + DatadogWrapperHelper.finishSpan(scope.span(), t); + throw t; + } + } + } + public static class DatadogAsyncWrapper extends AbstractFunction1> { private final Function1> userHandler; @@ -172,13 +175,13 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { @Override public Future apply(HttpRequest request) { - final Scope scope = DatadogSyncWrapper.createSpan(request); + final Scope scope = DatadogWrapperHelper.createSpan(request); Future futureResponse = null; try { futureResponse = userHandler.apply(request); } catch (Throwable t) { scope.close(); - DatadogSyncWrapper.finishSpan(scope.span(), t); + DatadogWrapperHelper.finishSpan(scope.span(), t); throw t; } final Future wrapped = @@ -186,14 +189,14 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { new AbstractFunction1() { @Override public HttpResponse apply(HttpResponse response) { - DatadogSyncWrapper.finishSpan(scope.span(), response); + DatadogWrapperHelper.finishSpan(scope.span(), response); return response; } }, new AbstractFunction1() { @Override public Throwable apply(Throwable t) { - DatadogSyncWrapper.finishSpan(scope.span(), t); + DatadogWrapperHelper.finishSpan(scope.span(), t); return t; } }, @@ -203,10 +206,10 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { } } - public static class AkkaHttpHeaders implements TextMap { + public static class AkkaHttpServerHeaders implements TextMap { private final HttpRequest request; - public AkkaHttpHeaders(HttpRequest request) { + public AkkaHttpServerHeaders(HttpRequest request) { this.request = request; } @@ -222,8 +225,8 @@ public final class AkkaHttpInstrumentation extends Instrumenter.Configurable { } @Override - public void put(String s, String s1) { - throw new IllegalStateException("akka http headers can only be extracted"); + public void put(String name, String value) { + throw new IllegalStateException("akka http server headers can only be extracted"); } } } diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy similarity index 72% rename from dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpInstrumentationTest.groovy rename to dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy index 86a645a581..254bfdecf5 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy @@ -1,11 +1,14 @@ import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import io.opentracing.tag.Tags import okhttp3.OkHttpClient import okhttp3.Request import spock.lang.Shared import static datadog.trace.agent.test.ListWriterAssert.assertTraces -class AkkaHttpInstrumentationTest extends AgentTestRunner { +class AkkaHttpServerInstrumentationTest extends AgentTestRunner { static { System.setProperty("dd.integration.akka-http-server.enabled", "true") } @@ -52,12 +55,12 @@ class AkkaHttpInstrumentationTest extends AgentTestRunner { errored false tags { defaultTags() - "http.status_code" 200 - "http.url" "http://localhost:$port/test" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" + "$Tags.HTTP_STATUS.key" 200 + "$Tags.HTTP_URL.key" "http://localhost:$port/test" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" } } span(1) { @@ -94,16 +97,13 @@ class AkkaHttpInstrumentationTest extends AgentTestRunner { errored true tags { defaultTags() - "http.status_code" 500 - "http.url" "http://localhost:$port/$endpoint" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" - "error" true - "error.type" RuntimeException.name - "error.msg" errorMessage - "error.stack" String + "$Tags.HTTP_STATUS.key" 500 + "$Tags.HTTP_URL.key" "http://localhost:$port/$endpoint" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" + errorTags RuntimeException, errorMessage } } } @@ -137,13 +137,13 @@ class AkkaHttpInstrumentationTest extends AgentTestRunner { errored true tags { defaultTags() - "http.status_code" 500 - "http.url" "http://localhost:$port/server-error" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" - "error" true + "$Tags.HTTP_STATUS.key" 500 + "$Tags.HTTP_URL.key" "http://localhost:$port/server-error" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" + "$Tags.ERROR.key" true } } } @@ -176,12 +176,12 @@ class AkkaHttpInstrumentationTest extends AgentTestRunner { errored false tags { defaultTags() - "http.status_code" 404 - "http.url" "http://localhost:$port/not-found" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" + "$Tags.HTTP_STATUS.key" 404 + "$Tags.HTTP_URL.key" "http://localhost:$port/not-found" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" } } } From 8d6392fa94b9468cd8c3dda3c0760be19aea4c8a Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 12:01:42 -0400 Subject: [PATCH 2/2] Remove timeouts from lagom tests It looks like server is started lazily and on laptop it may take over 5 seconds in parallel build. This means in may take long time on CI as well. It is in fact unikely that server will never return so adding timeout introduces flakiness and doesn't really protect from any real-life problems. Instead of hardcoding timeouts just rely on build eventually giving up on its own one way or another. --- .../src/lagomTest/groovy/LagomTest.groovy | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy index 0dde2113bc..cb59cb8c1e 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy @@ -1,9 +1,11 @@ import akka.NotUsed import akka.stream.javadsl.Source import akka.stream.testkit.javadsl.TestSink +import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import io.opentracing.tag.Tags import net.bytebuddy.utility.JavaModule -import static java.util.concurrent.TimeUnit.SECONDS import datadog.trace.agent.test.AgentTestRunner import play.inject.guice.GuiceApplicationBuilder import spock.lang.Shared @@ -64,7 +66,7 @@ class LagomTest extends AgentTestRunner { Source.from(Arrays.asList("msg1", "msg2", "msg3")) .concat(Source.maybe()) Source output = service.echo().invoke(input) - .toCompletableFuture().get(5, SECONDS) + .toCompletableFuture().get() Probe probe = output.runWith(TestSink.probe(server.system()), server.materializer()) probe.request(10) probe.expectNext("msg1") @@ -82,12 +84,12 @@ class LagomTest extends AgentTestRunner { errored false tags { defaultTags() - "http.status_code" 101 - "http.url" "ws://localhost:${server.port()}/echo" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" + "$Tags.HTTP_STATUS.key" 101 + "$Tags.HTTP_URL.key" "ws://localhost:${server.port()}/echo" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" } } span(1) { @@ -106,7 +108,7 @@ class LagomTest extends AgentTestRunner { Source.from(Arrays.asList("msg1", "msg2", "msg3")) .concat(Source.maybe()) try { - service.error().invoke(input).toCompletableFuture().get(5, SECONDS) + service.error().invoke(input).toCompletableFuture().get() } catch (Exception e) { } @@ -120,13 +122,13 @@ class LagomTest extends AgentTestRunner { errored true tags { defaultTags() - "http.status_code" 500 - "http.url" "ws://localhost:${server.port()}/error" - "http.method" "GET" - "span.kind" "server" - "span.type" "web" - "component" "akka-http-server" - "error" true + "$Tags.HTTP_STATUS.key" 500 + "$Tags.HTTP_URL.key" "ws://localhost:${server.port()}/error" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_SERVER + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "akka-http-server" + "$Tags.ERROR.key" true } } }