From a882a5439c1a7b770316697e086769956d29a568 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 25 Jun 2018 16:49:23 -0400 Subject: [PATCH 01/25] OkHttp instumentation: simplify error case --- .../okhttp3/OkHttpClientSpanDecorator.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java index 7e3ba79f76..6feb472cf9 100644 --- a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java +++ b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java @@ -4,12 +4,15 @@ import io.opentracing.Span; import io.opentracing.tag.Tags; import java.net.Inet4Address; import java.nio.ByteBuffer; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import okhttp3.Connection; import okhttp3.Request; import okhttp3.Response; +import static io.opentracing.log.Fields.ERROR_OBJECT; + /** * Span decorator to add tags, logs and operation name. * @@ -64,7 +67,7 @@ public interface OkHttpClientSpanDecorator { @Override public void onError(final Throwable throwable, final Span span) { Tags.ERROR.set(span, Boolean.TRUE); - span.log(errorLogs(throwable)); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); } @Override @@ -81,13 +84,5 @@ public interface OkHttpClientSpanDecorator { Tags.PEER_HOST_IPV6.set(span, connection.socket().getInetAddress().toString()); } } - - protected Map errorLogs(final Throwable throwable) { - final Map errorLogs = new HashMap<>(2); - errorLogs.put("event", Tags.ERROR.getKey()); - errorLogs.put("error.object", throwable); - - return errorLogs; - } }; } From d4d770fe4224687f97e5fbef94f0ab2442ef8154 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 10:25:13 -0400 Subject: [PATCH 02/25] Pull out ratpack helper into seprate class to avoid copy-paste --- .../test/groovy/ApacheHttpClientTest.groovy | 43 ++--------------- .../test/groovy/HttpUrlConnectionTest.groovy | 40 +--------------- .../trace/agent/test/RatpackUtils.groovy | 47 +++++++++++++++++++ dd-java-agent/testing/testing.gradle | 6 ++- 4 files changed, 56 insertions(+), 80 deletions(-) create mode 100644 dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy diff --git a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy index 791cbc39fe..96960d791f 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy +++ b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy @@ -1,18 +1,13 @@ import datadog.opentracing.DDSpan import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.RatpackUtils import datadog.trace.api.DDSpanTypes -import io.opentracing.Scope -import io.opentracing.SpanContext -import io.opentracing.propagation.Format -import io.opentracing.propagation.TextMap import io.opentracing.tag.Tags -import io.opentracing.util.GlobalTracer import org.apache.http.HttpResponse import org.apache.http.client.HttpClient import org.apache.http.client.methods.HttpGet import org.apache.http.impl.client.HttpClientBuilder import org.apache.http.message.BasicHeader -import ratpack.handling.Context import spock.lang.Shared import static datadog.trace.agent.test.TestUtils.runUnderTrace @@ -24,23 +19,9 @@ class ApacheHttpClientTest extends AgentTestRunner { def server = ratpack { handlers { get { - String msg = "

Hello test.

\n" - boolean isDDServer = true - if (context.request.getHeaders().contains("is-dd-server")) { - isDDServer = Boolean.parseBoolean(context.request.getHeaders().get("is-dd-server")) - } - if (isDDServer) { - final SpanContext extractedContext = - GlobalTracer.get() - .extract(Format.Builtin.HTTP_HEADERS, new RatpackResponseAdapter(context)) - Scope scope = - GlobalTracer.get() - .buildSpan("test-http-server") - .asChildOf(extractedContext) - .startActive(true) - scope.close() - } + RatpackUtils.handleDistributedRequest(context) + String msg = "

Hello test.

\n" response.status(200).send(msg) } } @@ -128,22 +109,4 @@ class ApacheHttpClientTest extends AgentTestRunner { clientSpan.getTags()[Tags.PEER_PORT.getKey()] == server.getAddress().port clientSpan.getTags()[Tags.SPAN_KIND.getKey()] == Tags.SPAN_KIND_CLIENT } - - private static class RatpackResponseAdapter implements TextMap { - final Context context - - RatpackResponseAdapter(Context context) { - this.context = context - } - - @Override - void put(String key, String value) { - context.response.set(key, value) - } - - @Override - Iterator> iterator() { - return context.request.getHeaders().asMultiValueMap().entrySet().iterator() - } - } } diff --git a/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy b/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy index ccd337a576..5ffc21300b 100644 --- a/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy +++ b/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy @@ -1,13 +1,9 @@ import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.RatpackUtils import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags -import io.opentracing.Scope -import io.opentracing.SpanContext -import io.opentracing.propagation.Format -import io.opentracing.propagation.TextMap import io.opentracing.tag.Tags import io.opentracing.util.GlobalTracer -import ratpack.handling.Context import spock.lang.Shared import static datadog.trace.agent.test.ListWriterAssert.assertTraces @@ -28,21 +24,7 @@ class HttpUrlConnectionTest extends AgentTestRunner { def server = ratpack { handlers { all { - boolean isDDServer = true - if (context.request.getHeaders().contains("is-dd-server")) { - isDDServer = Boolean.parseBoolean(context.request.getHeaders().get("is-dd-server")) - } - if (isDDServer) { - final SpanContext extractedContext = - GlobalTracer.get() - .extract(Format.Builtin.HTTP_HEADERS, new RatpackResponseAdapter(context)) - Scope scope = - GlobalTracer.get() - .buildSpan("test-http-server") - .asChildOf((SpanContext) extractedContext) - .startActive(true) - scope.close() - } + RatpackUtils.handleDistributedRequest(context) response.status(STATUS) // Ratpack seems to be sending body with HEAD requests - RFC specifically forbids this. @@ -442,22 +424,4 @@ class HttpUrlConnectionTest extends AgentTestRunner { } } } - - private static class RatpackResponseAdapter implements TextMap { - final Context context - - RatpackResponseAdapter(Context context) { - this.context = context - } - - @Override - void put(String key, String value) { - context.response.set(key, value) - } - - @Override - Iterator> iterator() { - return context.request.getHeaders().asMultiValueMap().entrySet().iterator() - } - } } diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy new file mode 100644 index 0000000000..71c0bdb953 --- /dev/null +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy @@ -0,0 +1,47 @@ +package datadog.trace.agent.test + +import io.opentracing.Scope +import io.opentracing.SpanContext +import io.opentracing.propagation.Format +import io.opentracing.propagation.TextMap +import io.opentracing.util.GlobalTracer +import ratpack.handling.Context + +class RatpackUtils { + + static handleDistributedRequest(Context context) { + boolean isDDServer = true + if (context.request.getHeaders().contains("is-dd-server")) { + isDDServer = Boolean.parseBoolean(context.request.getHeaders().get("is-dd-server")) + } + if (isDDServer) { + final SpanContext extractedContext = + GlobalTracer.get() + .extract(Format.Builtin.HTTP_HEADERS, new RatpackResponseAdapter(context)) + Scope scope = + GlobalTracer.get() + .buildSpan("test-http-server") + .asChildOf(extractedContext) + .startActive(true) + scope.close() + } + } + + private static class RatpackResponseAdapter implements TextMap { + final Context context + + RatpackResponseAdapter(Context context) { + this.context = context + } + + @Override + void put(String key, String value) { + context.response.set(key, value) + } + + @Override + Iterator> iterator() { + return context.request.getHeaders().asMultiValueMap().entrySet().iterator() + } + } +} diff --git a/dd-java-agent/testing/testing.gradle b/dd-java-agent/testing/testing.gradle index 9cdf90956f..ed8cdd1c28 100644 --- a/dd-java-agent/testing/testing.gradle +++ b/dd-java-agent/testing/testing.gradle @@ -5,8 +5,8 @@ minimumInstructionCoverage = 0.6 excludedClassesConverage += [ 'datadog.trace.agent.test.*Assert', 'datadog.trace.agent.test.AgentTestRunner.ErrorCountingListener', - 'datadog.trace.agent.test.TestUtils', - 'datadog.trace.agent.test.OkHttpUtils' + 'datadog.trace.agent.test.*Utils', + 'datadog.trace.agent.test.*Utils.*' ] dependencies { @@ -19,6 +19,8 @@ dependencies { compile deps.guava compile group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.10.0' + // Note: this should be the same version as in java.gradle + compileOnly group: 'io.ratpack', name: 'ratpack-groovy-test', version: '1.4.6' compile project(':dd-trace-ot') compile project(':dd-java-agent:agent-tooling') From ce7866d0183199def64f53db24035aca42f894cd Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 10:45:49 -0400 Subject: [PATCH 03/25] Use `io.opentracing.log.Fields.*` instead of hardcoded strings --- .../akkahttp/AkkaHttpServerInstrumentation.java | 3 ++- .../trace/instrumentation/aws/v0/SpanDecorator.java | 12 +++++++----- .../instrumentation/aws/v106/SpanDecorator.java | 12 +++++++----- .../datastax/cassandra/TracingSession.java | 12 +++++++----- .../instrumentation/jedis/JedisInstrumentation.java | 3 ++- .../lettuce/ConnectionFutureAdvice.java | 4 +++- .../lettuce/LettuceAsyncBiFunction.java | 4 +++- .../lettuce/LettuceAsyncCommandsAdvice.java | 4 +++- .../lettuce/rx/LettuceFluxTerminationRunnable.java | 4 +++- .../lettuce/rx/LettuceMonoDualConsumer.java | 4 +++- .../okhttp3/OkHttpClientSpanDecorator.java | 6 ++---- .../instrumentation/play/PlayInstrumentation.java | 3 ++- 12 files changed, 44 insertions(+), 27 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java index 4509644f5d..c9570b6d96 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.akkahttp; +import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.*; import akka.http.javadsl.model.HttpHeader; @@ -127,7 +128,7 @@ public final class AkkaHttpServerInstrumentation extends Instrumenter.Default { public static void finishSpan(Span span, Throwable t) { Tags.ERROR.set(span, true); - span.log(Collections.singletonMap("error.object", t)); + span.log(Collections.singletonMap(ERROR_OBJECT, t)); Tags.HTTP_STATUS.set(span, 500); if (GlobalTracer.get().scopeManager().active() instanceof TraceScope) { diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/SpanDecorator.java b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/SpanDecorator.java index b61316d47d..7f16cfef38 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/SpanDecorator.java +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/SpanDecorator.java @@ -13,6 +13,8 @@ */ package datadog.trace.instrumentation.aws.v0; +import static io.opentracing.log.Fields.*; + import com.amazonaws.AmazonWebServiceResponse; import com.amazonaws.Request; import com.amazonaws.Response; @@ -95,15 +97,15 @@ class SpanDecorator { private static Map errorLogs(final Throwable throwable) { final Map errorLogs = new HashMap<>(4); - errorLogs.put("event", Tags.ERROR.getKey()); - errorLogs.put("error.kind", throwable.getClass().getName()); - errorLogs.put("error.object", throwable); + errorLogs.put(EVENT, Tags.ERROR.getKey()); + errorLogs.put(ERROR_KIND, throwable.getClass().getName()); + errorLogs.put(ERROR_OBJECT, throwable); - errorLogs.put("message", throwable.getMessage()); + errorLogs.put(MESSAGE, throwable.getMessage()); final StringWriter sw = new StringWriter(); throwable.printStackTrace(new PrintWriter(sw)); - errorLogs.put("stack", sw.toString()); + errorLogs.put(STACK, sw.toString()); return errorLogs; } diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.106/src/main/java/datadog/trace/instrumentation/aws/v106/SpanDecorator.java b/dd-java-agent/instrumentation/aws-java-sdk-1.11.106/src/main/java/datadog/trace/instrumentation/aws/v106/SpanDecorator.java index 1074a96876..370b4f011f 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.106/src/main/java/datadog/trace/instrumentation/aws/v106/SpanDecorator.java +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.106/src/main/java/datadog/trace/instrumentation/aws/v106/SpanDecorator.java @@ -13,6 +13,8 @@ */ package datadog.trace.instrumentation.aws.v106; +import static io.opentracing.log.Fields.*; + import com.amazonaws.AmazonWebServiceResponse; import com.amazonaws.Request; import com.amazonaws.Response; @@ -95,15 +97,15 @@ class SpanDecorator { private static Map errorLogs(final Throwable throwable) { final Map errorLogs = new HashMap<>(4); - errorLogs.put("event", Tags.ERROR.getKey()); - errorLogs.put("error.kind", throwable.getClass().getName()); - errorLogs.put("error.object", throwable); + errorLogs.put(EVENT, Tags.ERROR.getKey()); + errorLogs.put(ERROR_KIND, throwable.getClass().getName()); + errorLogs.put(ERROR_OBJECT, throwable); - errorLogs.put("message", throwable.getMessage()); + errorLogs.put(MESSAGE, throwable.getMessage()); final StringWriter sw = new StringWriter(); throwable.printStackTrace(new PrintWriter(sw)); - errorLogs.put("stack", sw.toString()); + errorLogs.put(STACK, sw.toString()); return errorLogs; } diff --git a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/TracingSession.java b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/TracingSession.java index 630d3a9c09..51c0a89dbd 100644 --- a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/TracingSession.java +++ b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/TracingSession.java @@ -13,6 +13,8 @@ */ package datadog.trace.instrumentation.datastax.cassandra; +import static io.opentracing.log.Fields.*; + import com.datastax.driver.core.BoundStatement; import com.datastax.driver.core.CloseFuture; import com.datastax.driver.core.Cluster; @@ -295,15 +297,15 @@ class TracingSession implements Session { private static Map errorLogs(final Throwable throwable) { final Map errorLogs = new HashMap<>(4); - errorLogs.put("event", Tags.ERROR.getKey()); - errorLogs.put("error.kind", throwable.getClass().getName()); - errorLogs.put("error.object", throwable); + errorLogs.put(EVENT, Tags.ERROR.getKey()); + errorLogs.put(ERROR_KIND, throwable.getClass().getName()); + errorLogs.put(ERROR_OBJECT, throwable); - errorLogs.put("message", throwable.getMessage()); + errorLogs.put(MESSAGE, throwable.getMessage()); final StringWriter sw = new StringWriter(); throwable.printStackTrace(new PrintWriter(sw)); - errorLogs.put("stack", sw.toString()); + errorLogs.put(STACK, sw.toString()); return errorLogs; } diff --git a/dd-java-agent/instrumentation/jedis-1.4/src/main/java/datadog/trace/instrumentation/jedis/JedisInstrumentation.java b/dd-java-agent/instrumentation/jedis-1.4/src/main/java/datadog/trace/instrumentation/jedis/JedisInstrumentation.java index 7dca92ec21..c32a2a611b 100644 --- a/dd-java-agent/instrumentation/jedis-1.4/src/main/java/datadog/trace/instrumentation/jedis/JedisInstrumentation.java +++ b/dd-java-agent/instrumentation/jedis-1.4/src/main/java/datadog/trace/instrumentation/jedis/JedisInstrumentation.java @@ -1,6 +1,7 @@ package datadog.trace.instrumentation.jedis; import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses; +import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -82,7 +83,7 @@ public final class JedisInstrumentation extends Instrumenter.Default { if (throwable != null) { final Span span = scope.span(); Tags.ERROR.set(span, true); - span.log(Collections.singletonMap("error.object", throwable)); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); } scope.close(); } diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/ConnectionFutureAdvice.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/ConnectionFutureAdvice.java index dfec9275f0..999f58e436 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/ConnectionFutureAdvice.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/ConnectionFutureAdvice.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.lettuce; +import static io.opentracing.log.Fields.ERROR_OBJECT; + import datadog.trace.api.DDTags; import io.lettuce.core.ConnectionFuture; import io.lettuce.core.RedisURI; @@ -44,7 +46,7 @@ public class ConnectionFutureAdvice { if (throwable != null) { final Span span = scope.span(); Tags.ERROR.set(span, true); - span.log(Collections.singletonMap("error.object", throwable)); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); scope.close(); return; } diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncBiFunction.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncBiFunction.java index 013a075caa..a48868b240 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncBiFunction.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncBiFunction.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.lettuce; +import static io.opentracing.log.Fields.ERROR_OBJECT; + import io.opentracing.Span; import io.opentracing.tag.Tags; import java.util.Collections; @@ -31,7 +33,7 @@ public class LettuceAsyncBiFunction, Runnabl } if (throwable != null) { Tags.ERROR.set(this.span, true); - this.span.log(Collections.singletonMap("error.object", throwable)); + this.span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); } this.span.finish(); } else { diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java index aa9dcd4ae4..dd8c46b33b 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.lettuce.rx; +import static io.opentracing.log.Fields.ERROR_OBJECT; + import datadog.trace.api.DDTags; import datadog.trace.instrumentation.lettuce.LettuceInstrumentationUtil; import io.opentracing.Scope; @@ -29,7 +31,7 @@ public class LettuceMonoDualConsumer if (this.span != null) { if (throwable != null) { Tags.ERROR.set(this.span, true); - this.span.log(Collections.singletonMap("error.object", throwable)); + this.span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); } this.span.finish(); } else { diff --git a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java index 6feb472cf9..2e1ddd7616 100644 --- a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java +++ b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/OkHttpClientSpanDecorator.java @@ -1,18 +1,16 @@ package datadog.trace.instrumentation.okhttp3; +import static io.opentracing.log.Fields.ERROR_OBJECT; + import io.opentracing.Span; import io.opentracing.tag.Tags; import java.net.Inet4Address; import java.nio.ByteBuffer; import java.util.Collections; -import java.util.HashMap; -import java.util.Map; import okhttp3.Connection; import okhttp3.Request; import okhttp3.Response; -import static io.opentracing.log.Fields.ERROR_OBJECT; - /** * Span decorator to add tags, logs and operation name. * diff --git a/dd-java-agent/instrumentation/play-2.4/src/main/java/datadog/trace/instrumentation/play/PlayInstrumentation.java b/dd-java-agent/instrumentation/play-2.4/src/main/java/datadog/trace/instrumentation/play/PlayInstrumentation.java index cc71390786..35c6a35b43 100644 --- a/dd-java-agent/instrumentation/play-2.4/src/main/java/datadog/trace/instrumentation/play/PlayInstrumentation.java +++ b/dd-java-agent/instrumentation/play-2.4/src/main/java/datadog/trace/instrumentation/play/PlayInstrumentation.java @@ -2,6 +2,7 @@ package datadog.trace.instrumentation.play; import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClassWithMethod; import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses; +import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.*; import akka.japi.JavaPartialFunction; @@ -193,7 +194,7 @@ public final class PlayInstrumentation extends Instrumenter.Default { public static void onError(final Span span, final Throwable t) { Tags.ERROR.set(span, Boolean.TRUE); - span.log(Collections.singletonMap("error.object", t)); + span.log(Collections.singletonMap(ERROR_OBJECT, t)); Tags.HTTP_STATUS.set(span, 500); } } From c844501bc589dc1f0128684108718de98b6e248e Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 16:50:55 -0400 Subject: [PATCH 04/25] Fixes for some minor checkstyle complaints --- .../cassandra/CassandraClientInstrumentation.java | 3 +-- .../java/datadog/trace/agent/TracingAgent.java | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/CassandraClientInstrumentation.java b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/CassandraClientInstrumentation.java index ac24cc4472..2e2cb481ce 100644 --- a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/CassandraClientInstrumentation.java +++ b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/main/java/datadog/trace/instrumentation/datastax/cassandra/CassandraClientInstrumentation.java @@ -58,8 +58,7 @@ public class CassandraClientInstrumentation extends Instrumenter.Default { * com.datastax.driver.core.Cluster$Manager.newSession() method is called. The opentracing * contribution is a simple wrapper, so we just have to wrap the new session. * - * @param session The fresh session to patch - * @return A new tracing session + * @param session The fresh session to patch. This session is replaced with new session * @throws Exception */ @Advice.OnMethodExit(suppress = Throwable.class) diff --git a/dd-java-agent/src/main/java/datadog/trace/agent/TracingAgent.java b/dd-java-agent/src/main/java/datadog/trace/agent/TracingAgent.java index 20dd6a2775..a309c2eea2 100644 --- a/dd-java-agent/src/main/java/datadog/trace/agent/TracingAgent.java +++ b/dd-java-agent/src/main/java/datadog/trace/agent/TracingAgent.java @@ -155,12 +155,19 @@ public class TracingAgent { private static ClassLoader getPlatformClassLoader() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { - // must invoke ClassLoader.getPlatformClassLoader by reflection to remain compatible with java 7 - // + 8. + /* + Must invoke ClassLoader.getPlatformClassLoader by reflection to remain + compatible with java 7 + 8. + */ final Method method = ClassLoader.class.getDeclaredMethod("getPlatformClassLoader"); return (ClassLoader) method.invoke(null); } + /** + * Main entry point. + * + * @param args command line agruments + */ public static void main(final String... args) { try { System.out.println(getAgentVersion()); @@ -184,7 +191,9 @@ public class TracingAgent { new InputStreamReader( TracingAgent.class.getResourceAsStream("/dd-java-agent.version"), "UTF-8"); output = new BufferedReader(input); - for (int c = output.read(); c != -1; c = output.read()) sb.append((char) c); + for (int c = output.read(); c != -1; c = output.read()) { + sb.append((char) c); + } } finally { if (null != input) { input.close(); From e46fc30cf6eedb6661f80ae6454ab9c261328e13 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 27 Jun 2018 11:34:43 -0400 Subject: [PATCH 05/25] Remove confusing variable JAVA8_HOME from config.yml --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index c7b329b201..5806fafaf6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -115,7 +115,7 @@ jobs: test_8: <<: *default_test_job environment: - - JAVA8_HOME: /usr/lib/jvm/java-8-openjdk-amd64 + # We are building on Java8, this is our default JVM so no need to set more homes - TEST_TASK: test latestDepTest jacocoTestReport jacocoTestCoverageVerification test_9: From f9f135cbd62bd23ec2726858d4807259183c060f Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 27 Jun 2018 16:09:02 -0400 Subject: [PATCH 06/25] Do not run Spymemcached tests locally under Java7 since it requires external Memcached container running because testcontainers doesn't work on Java7. --- .../instrumentation/spymemcached/SpymemcachedTest.groovy | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy b/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy index 6d8ed6d5e0..6403bfda89 100644 --- a/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy +++ b/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy @@ -14,7 +14,7 @@ import net.spy.memcached.internal.CheckedOperationTimeoutException import net.spy.memcached.ops.Operation import net.spy.memcached.ops.OperationQueueFactory import org.testcontainers.containers.GenericContainer - +import spock.lang.Requires import spock.lang.Shared import java.util.concurrent.ArrayBlockingQueue @@ -30,6 +30,9 @@ import static CompletionListener.SPAN_TYPE import static datadog.trace.agent.test.TestUtils.runUnderTrace import static net.spy.memcached.ConnectionFactoryBuilder.Protocol.BINARY +// Do not run tests locally on Java7 since testcontainers are not compatibly with Java7 +// It is fine to run on CI because CI provides memcached externally, not through testcontainers +@Requires({ "true" == System.getenv("CI") || jvm.java8Compatible }) class SpymemcachedTest extends AgentTestRunner { static { From 6932d581ed3419e7a937bfe88d54c65138c3076a Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 29 Jun 2018 16:19:02 -0400 Subject: [PATCH 07/25] Add some helpful messages to AgentTestRunner assertions --- .../main/java/datadog/trace/agent/test/AgentTestRunner.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java index a24c4e8a5b..f65756df0e 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java @@ -139,13 +139,13 @@ public abstract class AgentTestRunner extends Specification { WRITER_PHASER.register(); INSTRUMENTATION_ERROR_COUNT.set(0); ERROR_LISTENER.activateTest(this); - assert getTestTracer().activeSpan() == null; + assert getTestTracer().activeSpan() == null : "Span is active before test has started"; } @After public void afterTest() { ERROR_LISTENER.deactivateTest(this); - assert INSTRUMENTATION_ERROR_COUNT.get() == 0; + assert INSTRUMENTATION_ERROR_COUNT.get() == 0 : "Instrumentation errors during test"; } @AfterClass From 2b25de966a9891f635b8c9d574901e6064a96136 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 26 Jun 2018 16:10:52 -0400 Subject: [PATCH 08/25] Add akka-http-client instrumentation This is a very-very first pass: instrument single request --- .../akka-http-10.0/akka-http-10.0.gradle | 3 + .../AkkaHttpClientInstrumentation.java | 140 ++++++++++++++++ .../AkkaHttpClientInstrumentationTest.groovy | 153 ++++++++++++++++++ 3 files changed, 296 insertions(+) create mode 100644 dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java create mode 100644 dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy diff --git a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle index e10a43321c..a4cc0777eb 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle +++ b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle @@ -41,3 +41,6 @@ dependencies { test.dependsOn lagomTest testJava8Minimum += '*Test*.class' + +// These classes use Ratpack which requires Java 8. (Currently also incompatible with Java 9.) +testJava8Only += '**/AkkaHttpClientInstrumentationTest.class' diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java new file mode 100644 index 0000000000..104c05ac5b --- /dev/null +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -0,0 +1,140 @@ +package datadog.trace.instrumentation.akkahttp; + +import static io.opentracing.log.Fields.ERROR_OBJECT; +import static net.bytebuddy.matcher.ElementMatchers.*; + +import akka.http.javadsl.model.headers.RawHeader; +import akka.http.scaladsl.HttpExt; +import akka.http.scaladsl.model.HttpRequest; +import akka.http.scaladsl.model.HttpResponse; +import akka.stream.*; +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.*; +import datadog.trace.api.DDSpanTypes; +import datadog.trace.api.DDTags; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.propagation.Format; +import io.opentracing.propagation.TextMap; +import io.opentracing.tag.Tags; +import io.opentracing.util.GlobalTracer; +import java.util.Collections; +import java.util.Iterator; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import net.bytebuddy.agent.builder.AgentBuilder; +import net.bytebuddy.asm.Advice; +import scala.concurrent.Future; +import scala.runtime.AbstractFunction1; +import scala.util.Try; + +@Slf4j +@AutoService(Instrumenter.class) +public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurable { + public AkkaHttpClientInstrumentation() { + super("akka-http", "akka-http-client"); + } + + @Override + protected boolean defaultEnabled() { + return false; + } + + private static final HelperInjector HELPER_INJECTOR = + new HelperInjector( + AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", + AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders"); + + @Override + public AgentBuilder apply(final AgentBuilder agentBuilder) { + return agentBuilder + .type(named("akka.http.scaladsl.HttpExt")) + .transform(DDTransformers.defaultTransformers()) + .transform(HELPER_INJECTOR) + .transform( + DDAdvice.create() + .advice( + named("singleRequest") + .and(takesArgument(0, named("akka.http.scaladsl.model.HttpRequest"))), + AkkaHttpClientAdvice.class.getName())) + .asDecorator(); + } + + public static class AkkaHttpClientAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static Scope methodEnter( + @Advice.Argument(value = 0, readOnly = false) HttpRequest request) { + Scope scope = + GlobalTracer.get() + .buildSpan("akka-http.request") + .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT) + .withTag(Tags.HTTP_METHOD.getKey(), request.method().value()) + .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT) + .withTag(Tags.COMPONENT.getKey(), "akka-http-client") + .withTag(Tags.HTTP_URL.getKey(), request.getUri().toString()) + .startActive(false); + + AkkaHttpHeaders headers = new AkkaHttpHeaders(request); + GlobalTracer.get().inject(scope.span().context(), Format.Builtin.HTTP_HEADERS, headers); + // Request is immutable, so we have to assign new value once we update headers + request = headers.getRequest(); + + return scope; + } + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void methodExit( + @Advice.Argument(value = 0) final HttpRequest request, + @Advice.This final HttpExt thiz, + @Advice.Return final Future responseFuture, + @Advice.Enter final Scope scope) { + responseFuture.onComplete(new OnCompleteHandler(scope), thiz.system().dispatcher()); + scope.close(); + } + } + + public static class OnCompleteHandler extends AbstractFunction1, Void> { + private final Scope scope; + + public OnCompleteHandler(Scope scope) { + this.scope = scope; + } + + @Override + public Void apply(Try result) { + Span span = scope.span(); + if (result.isSuccess()) { + Tags.HTTP_STATUS.set(span, result.get().status().intValue()); + } else { + Tags.ERROR.set(span, true); + span.log(Collections.singletonMap(ERROR_OBJECT, result.failed().get())); + } + span.finish(); + return null; + } + } + + public static class AkkaHttpHeaders implements TextMap { + private HttpRequest request; + + public AkkaHttpHeaders(HttpRequest request) { + this.request = request; + } + + @Override + public Iterator> iterator() { + throw new UnsupportedOperationException( + "This class should be used only with Tracer.inject()!"); + } + + @Override + public void put(final String name, final String value) { + // It looks like this cast is only needed in Java, Scala would have figured it out + request = (HttpRequest) request.addHeader(RawHeader.create(name, value)); + } + + public HttpRequest getRequest() { + return request; + } + } +} diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy new file mode 100644 index 0000000000..d249cf7059 --- /dev/null +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy @@ -0,0 +1,153 @@ +import akka.actor.ActorSystem +import akka.http.javadsl.Http +import akka.http.javadsl.model.HttpRequest +import akka.http.javadsl.model.HttpResponse +import akka.stream.ActorMaterializer +import akka.stream.StreamTcpException +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.RatpackUtils +import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import io.opentracing.tag.Tags +import spock.lang.Shared + +import java.util.concurrent.CompletionStage +import java.util.concurrent.ExecutionException + +import static datadog.trace.agent.test.ListWriterAssert.assertTraces +import static ratpack.groovy.test.embed.GroovyEmbeddedApp.ratpack + +class AkkaHttpClientInstrumentationTest extends AgentTestRunner { + static { + System.setProperty("dd.integration.akka-http-client.enabled", "true") + } + + private static final String MESSAGE = "an\nmultiline\nhttp\nresponse" + private static final long TIMEOUT = 10000L + + @Shared + def server = ratpack { + handlers { + prefix("success") { + all { + RatpackUtils.handleDistributedRequest(context) + + response.status(200).send(MESSAGE) + } + } + + prefix("error") { + all { + RatpackUtils.handleDistributedRequest(context) + + throw new RuntimeException("error") + } + } + } + } + + @Shared + ActorSystem system = ActorSystem.create() + @Shared + ActorMaterializer materializer = ActorMaterializer.create(system) + + def "#route request trace" () { + setup: + def url = server.address.resolve("/" + route).toURL() + + HttpRequest request = HttpRequest.create(url.toString()) + CompletionStage responseFuture = + Http.get(system) + .singleRequest(request, materializer) + HttpResponse response = responseFuture.toCompletableFuture().get() + String message = readMessage(response) + + expect: + response.status().intValue() == expectedStatus + if (expectedMessage != null) { + message == expectedMessage + } + + assertTraces(TEST_WRITER, 2) { + trace(0, 1) { + span(0) { + operationName "test-http-server" + childOf(TEST_WRITER[1][0]) + errored false + tags { + defaultTags() + } + } + } + trace(1, 1) { + span(0) { + parent() + serviceName "unnamed-java-app" + operationName "akka-http.request" + resourceName "GET /$route" + errored expectedError + tags { + defaultTags() + "$Tags.HTTP_STATUS.key" expectedStatus + "$Tags.HTTP_URL.key" "${server.address}$route" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT + "$Tags.COMPONENT.key" "akka-http-client" + if (expectedError) { + "$Tags.ERROR.key" true + } + } + } + } + } + + where: + route | expectedStatus | expectedError | expectedMessage + "success" | 200 | false | MESSAGE + "error" | 500 | true | null + } + + def "error request trace" () { + setup: + def url = new URL("http://localhost:${server.address.port + 1}/test") + + HttpRequest request = HttpRequest.create(url.toString()) + CompletionStage responseFuture = + Http.get(system) + .singleRequest(request, materializer) + try { + responseFuture.toCompletableFuture().get() + } catch (ExecutionException e) { + // This is expected to fail + } + + expect: + assertTraces(TEST_WRITER, 1) { + trace(0, 1) { + span(0) { + parent() + serviceName "unnamed-java-app" + operationName "akka-http.request" + resourceName "GET /test" + errored true + tags { + defaultTags() + "$Tags.HTTP_URL.key" url.toString() + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT + "$Tags.COMPONENT.key" "akka-http-client" + "$Tags.ERROR.key" true + errorTags(StreamTcpException, { it.contains("Tcp command") }) + } + } + } + } + } + + String readMessage(HttpResponse response) { + response.entity().toStrict(TIMEOUT, materializer).toCompletableFuture().get().getData().utf8String() + } + +} From ae37ca4b0229852f96aea97a7e00e9056e5b37e0 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 29 Jun 2018 16:20:32 -0400 Subject: [PATCH 09/25] Add akka-http-client instrumentation: superPool --- .../akka-http-10.0/akka-http-10.0.gradle | 3 + .../AkkaHttpClientInstrumentation.java | 25 +++- .../AkkaHttpClientTransformFlow.scala | 48 ++++++++ .../AkkaHttpClientInstrumentationTest.groovy | 116 +++++++++++++++++- 4 files changed, 182 insertions(+), 10 deletions(-) rename dd-java-agent/instrumentation/akka-http-10.0/src/main/{java => scala}/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java (85%) create mode 100644 dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientTransformFlow.scala diff --git a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle index a4cc0777eb..cf3358daeb 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle +++ b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle @@ -1,6 +1,9 @@ apply from: "${rootDir}/gradle/java.gradle" apply from: "${rootDir}/gradle/test-with-scala.gradle" +// We have actual Scala sources here +apply plugin: 'scala' + apply plugin: 'org.unbroken-dome.test-sets' testSets { lagomTest diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java similarity index 85% rename from dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java rename to dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index 104c05ac5b..adf42c0b00 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -3,11 +3,12 @@ package datadog.trace.instrumentation.akkahttp; import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.*; +import akka.NotUsed; import akka.http.javadsl.model.headers.RawHeader; import akka.http.scaladsl.HttpExt; import akka.http.scaladsl.model.HttpRequest; import akka.http.scaladsl.model.HttpResponse; -import akka.stream.*; +import akka.stream.scaladsl.Flow; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.*; import datadog.trace.api.DDSpanTypes; @@ -24,6 +25,7 @@ import java.util.Map; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; +import scala.Tuple2; import scala.concurrent.Future; import scala.runtime.AbstractFunction1; import scala.util.Try; @@ -43,7 +45,8 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab private static final HelperInjector HELPER_INJECTOR = new HelperInjector( AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", - AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders"); + AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders", + AkkaHttpClientTransformFlow.class.getName()); @Override public AgentBuilder apply(final AgentBuilder agentBuilder) { @@ -56,11 +59,16 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab .advice( named("singleRequest") .and(takesArgument(0, named("akka.http.scaladsl.model.HttpRequest"))), - AkkaHttpClientAdvice.class.getName())) + SingleRequesrAdvice.class.getName())) + .transform( + DDAdvice.create() + .advice( + named("superPool").and(returns(named("akka.stream.scaladsl.Flow"))), + SuperPoolAdvice.class.getName())) .asDecorator(); } - public static class AkkaHttpClientAdvice { + public static class SingleRequesrAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope methodEnter( @Advice.Argument(value = 0, readOnly = false) HttpRequest request) { @@ -93,6 +101,15 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab } } + public static class SuperPoolAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void methodExit( + @Advice.Return(readOnly = false) + Flow, Tuple2, T>, NotUsed> flow) { + flow = AkkaHttpClientTransformFlow.transform(flow); + } + } + public static class OnCompleteHandler extends AbstractFunction1, Void> { private final Scope scope; diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientTransformFlow.scala b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientTransformFlow.scala new file mode 100644 index 0000000000..447624cec1 --- /dev/null +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientTransformFlow.scala @@ -0,0 +1,48 @@ +package datadog.trace.instrumentation.akkahttp + +import java.util.Collections + +import akka.NotUsed +import akka.http.scaladsl.model.{HttpRequest, HttpResponse} +import akka.stream.Supervision +import akka.stream.scaladsl.Flow +import datadog.trace.api.{DDSpanTypes, DDTags} +import io.opentracing.log.Fields.ERROR_OBJECT +import io.opentracing.{Scope, Span} +import io.opentracing.propagation.Format +import io.opentracing.tag.Tags +import io.opentracing.util.GlobalTracer + +import scala.util.{Failure, Success, Try} + +object AkkaHttpClientTransformFlow { + def transform[T](flow: Flow[(HttpRequest, T), (Try[HttpResponse], T), NotUsed]): Flow[(HttpRequest, T), (Try[HttpResponse], T), NotUsed] = { + var span: Span = null + + Flow.fromFunction((input: (HttpRequest, T)) => { + val (request, data) = input + val scope = GlobalTracer.get + .buildSpan("akka-http.request") + .withTag(Tags.SPAN_KIND.getKey, Tags.SPAN_KIND_CLIENT) + .withTag(Tags.HTTP_METHOD.getKey, request.method.value) + .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT) + .withTag(Tags.COMPONENT.getKey, "akka-http-client") + .withTag(Tags.HTTP_URL.getKey, request.getUri.toString) + .startActive(false) + val headers = new AkkaHttpClientInstrumentation.AkkaHttpHeaders(request) + GlobalTracer.get.inject(scope.span.context, Format.Builtin.HTTP_HEADERS, headers) + span = scope.span + scope.close() + (headers.getRequest, data) + }).via(flow).map(output => { + output._1 match { + case Success(response) => Tags.HTTP_STATUS.set(span, response.status.intValue) + case Failure(e) => + Tags.ERROR.set(span, true) + span.log(Collections.singletonMap(ERROR_OBJECT, e)) + } + span.finish() + output + }) + } +} diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy index d249cf7059..dbb5950507 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy @@ -2,13 +2,17 @@ import akka.actor.ActorSystem import akka.http.javadsl.Http import akka.http.javadsl.model.HttpRequest import akka.http.javadsl.model.HttpResponse +import akka.japi.Pair import akka.stream.ActorMaterializer import akka.stream.StreamTcpException +import akka.stream.javadsl.Sink +import akka.stream.javadsl.Source import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.RatpackUtils import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import io.opentracing.tag.Tags +import scala.util.Try import spock.lang.Shared import java.util.concurrent.CompletionStage @@ -51,6 +55,8 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { @Shared ActorMaterializer materializer = ActorMaterializer.create(system) + def pool = Http.get(system).superPool(materializer) + def "#route request trace" () { setup: def url = server.address.resolve("/" + route).toURL() @@ -59,10 +65,12 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { CompletionStage responseFuture = Http.get(system) .singleRequest(request, materializer) + + when: HttpResponse response = responseFuture.toCompletableFuture().get() String message = readMessage(response) - expect: + then: response.status().intValue() == expectedStatus if (expectedMessage != null) { message == expectedMessage @@ -116,13 +124,109 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { CompletionStage responseFuture = Http.get(system) .singleRequest(request, materializer) - try { - responseFuture.toCompletableFuture().get() - } catch (ExecutionException e) { - // This is expected to fail + + when: + responseFuture.toCompletableFuture().get() + + then: + thrown ExecutionException + assertTraces(TEST_WRITER, 1) { + trace(0, 1) { + span(0) { + parent() + serviceName "unnamed-java-app" + operationName "akka-http.request" + resourceName "GET /test" + errored true + tags { + defaultTags() + "$Tags.HTTP_URL.key" url.toString() + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT + "$Tags.COMPONENT.key" "akka-http-client" + "$Tags.ERROR.key" true + errorTags(StreamTcpException, { it.contains("Tcp command") }) + } + } + } + } + } + + def "#route pool request trace" () { + setup: + def url = server.address.resolve("/" + route).toURL() + + CompletionStage, Integer>> sink = Source + .>single(new Pair(HttpRequest.create(url.toString()), 1)) + .via(pool) + .runWith(Sink., Integer>>head(), materializer) + + when: + HttpResponse response = sink.toCompletableFuture().get().first().get() + String message = readMessage(response) + + then: + response.status().intValue() == expectedStatus + if (expectedMessage != null) { + message == expectedMessage } - expect: + assertTraces(TEST_WRITER, 2) { + trace(0, 1) { + span(0) { + operationName "test-http-server" + childOf(TEST_WRITER[1][0]) + errored false + tags { + defaultTags() + } + } + } + trace(1, 1) { + span(0) { + parent() + serviceName "unnamed-java-app" + operationName "akka-http.request" + resourceName "GET /$route" + errored expectedError + tags { + defaultTags() + "$Tags.HTTP_STATUS.key" expectedStatus + "$Tags.HTTP_URL.key" "${server.address}$route" + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT + "$Tags.COMPONENT.key" "akka-http-client" + if (expectedError) { + "$Tags.ERROR.key" true + } + } + } + } + } + + where: + route | expectedStatus | expectedError | expectedMessage + "success" | 200 | false | MESSAGE + "error" | 500 | true | null + } + + def "error request pool trace" () { + setup: + def url = new URL("http://localhost:${server.address.port + 1}/test") + + CompletionStage, Integer>> sink = Source + .>single(new Pair(HttpRequest.create(url.toString()), 1)) + .via(pool) + .runWith(Sink., Integer>>head(), materializer) + def response = sink.toCompletableFuture().get().first() + + when: + response.get() + + then: + thrown StreamTcpException assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { From db895f2e34742371aea01fe34b0243dfbc0effc2 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 10:09:10 -0400 Subject: [PATCH 10/25] Remove redundant plugin from akka gradle file --- .../instrumentation/akka-http-10.0/akka-http-10.0.gradle | 3 --- 1 file changed, 3 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle index cf3358daeb..a4cc0777eb 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle +++ b/dd-java-agent/instrumentation/akka-http-10.0/akka-http-10.0.gradle @@ -1,9 +1,6 @@ apply from: "${rootDir}/gradle/java.gradle" apply from: "${rootDir}/gradle/test-with-scala.gradle" -// We have actual Scala sources here -apply plugin: 'scala' - apply plugin: 'org.unbroken-dome.test-sets' testSets { lagomTest From 277e7a1a3c7002c7864c6ce5bdd736f32760fbc4 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 10:13:00 -0400 Subject: [PATCH 11/25] Typo fix --- .../akkahttp/AkkaHttpClientInstrumentation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index adf42c0b00..452aac246a 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -59,7 +59,7 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab .advice( named("singleRequest") .and(takesArgument(0, named("akka.http.scaladsl.model.HttpRequest"))), - SingleRequesrAdvice.class.getName())) + SingleRequestAdvice.class.getName())) .transform( DDAdvice.create() .advice( @@ -68,7 +68,7 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab .asDecorator(); } - public static class SingleRequesrAdvice { + public static class SingleRequestAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope methodEnter( @Advice.Argument(value = 0, readOnly = false) HttpRequest request) { From 5e67e6e5ba6ee4a344c98821e53e5e1132f0de84 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 10:17:49 -0400 Subject: [PATCH 12/25] Migrate to new instrumentation API --- .../AkkaHttpClientInstrumentation.java | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index 452aac246a..9f557d4020 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -20,11 +20,13 @@ import io.opentracing.propagation.TextMap; import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; import java.util.Map; import lombok.extern.slf4j.Slf4j; -import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; import scala.Tuple2; import scala.concurrent.Future; import scala.runtime.AbstractFunction1; @@ -32,7 +34,7 @@ import scala.util.Try; @Slf4j @AutoService(Instrumenter.class) -public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurable { +public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { public AkkaHttpClientInstrumentation() { super("akka-http", "akka-http-client"); } @@ -42,30 +44,30 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Configurab return false; } - private static final HelperInjector HELPER_INJECTOR = - new HelperInjector( - AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", - AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders", - AkkaHttpClientTransformFlow.class.getName()); + @Override + public ElementMatcher typeMatcher() { + return named("akka.http.scaladsl.HttpExt"); + } @Override - public AgentBuilder apply(final AgentBuilder agentBuilder) { - return agentBuilder - .type(named("akka.http.scaladsl.HttpExt")) - .transform(DDTransformers.defaultTransformers()) - .transform(HELPER_INJECTOR) - .transform( - DDAdvice.create() - .advice( - named("singleRequest") - .and(takesArgument(0, named("akka.http.scaladsl.model.HttpRequest"))), - SingleRequestAdvice.class.getName())) - .transform( - DDAdvice.create() - .advice( - named("superPool").and(returns(named("akka.stream.scaladsl.Flow"))), - SuperPoolAdvice.class.getName())) - .asDecorator(); + public String[] helperClassNames() { + return new String[] { + AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", + AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders", + AkkaHttpClientTransformFlow.class.getName() + }; + } + + @Override + public Map transformers() { + final Map transformers = new HashMap<>(); + transformers.put( + named("singleRequest").and(takesArgument(0, named("akka.http.scaladsl.model.HttpRequest"))), + SingleRequestAdvice.class.getName()); + transformers.put( + named("superPool").and(returns(named("akka.stream.scaladsl.Flow"))), + SuperPoolAdvice.class.getName()); + return transformers; } public static class SingleRequestAdvice { From 161847752fe3665a9c517bf616325370bd6c9ddc Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 11:49:37 -0400 Subject: [PATCH 13/25] Pass Span instead of Scope --- .../akkahttp/AkkaHttpClientInstrumentation.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index 9f557d4020..bd1243117f 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -98,7 +98,7 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { @Advice.This final HttpExt thiz, @Advice.Return final Future responseFuture, @Advice.Enter final Scope scope) { - responseFuture.onComplete(new OnCompleteHandler(scope), thiz.system().dispatcher()); + responseFuture.onComplete(new OnCompleteHandler(scope.span()), thiz.system().dispatcher()); scope.close(); } } @@ -113,15 +113,14 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { } public static class OnCompleteHandler extends AbstractFunction1, Void> { - private final Scope scope; + private final Span span; - public OnCompleteHandler(Scope scope) { - this.scope = scope; + public OnCompleteHandler(Span span) { + this.span = span; } @Override public Void apply(Try result) { - Span span = scope.span(); if (result.isSuccess()) { Tags.HTTP_STATUS.set(span, result.get().status().intValue()); } else { From fd5d80ba862715c567b220b8b3290db8854ec9c9 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 14:27:31 -0400 Subject: [PATCH 14/25] Make muzzle plugin apply to Scala code --- dd-java-agent/instrumentation/instrumentation.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/instrumentation.gradle b/dd-java-agent/instrumentation/instrumentation.gradle index 2106cc51bd..68793d1547 100644 --- a/dd-java-agent/instrumentation/instrumentation.gradle +++ b/dd-java-agent/instrumentation/instrumentation.gradle @@ -19,7 +19,7 @@ subprojects { subProj -> subProj.byteBuddy { transformation { // Applying NoOp optimizes build by applying bytebuddy plugin to only compileJava task - tasks = ['compileJava'] + tasks = ['compileJava', 'compileScala'] plugin = 'datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin$NoOp' } } @@ -27,7 +27,7 @@ subprojects { subProj -> subProj.afterEvaluate { subProj.byteBuddy { transformation { - tasks = ['compileJava'] + tasks = ['compileJava', 'compileScala'] plugin = 'datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin' classPath = project(':dd-java-agent:agent-tooling').configurations.instrumentationMuzzle + subProj.configurations.compile + subProj.sourceSets.main.output } From 68ce09b461146ad26dfac4fec3943cc085ead311 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 16 Jul 2018 14:30:33 -0400 Subject: [PATCH 15/25] Add missing Scala classes to list in injection classes --- .../akkahttp/AkkaHttpClientInstrumentation.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index bd1243117f..99db94e49c 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -54,7 +54,10 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { return new String[] { AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders", - AkkaHttpClientTransformFlow.class.getName() + AkkaHttpClientTransformFlow.class.getName(), + AkkaHttpClientTransformFlow.class.getName() + "$", + AkkaHttpClientTransformFlow.class.getName() + "$$anonfun$transform$1", + AkkaHttpClientTransformFlow.class.getName() + "$$anonfun$transform$2", }; } From b4b99d991c09908007d50a3cfe01f394de1a334f Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 13:13:33 -0400 Subject: [PATCH 16/25] Move AkkaHttpServerInstrumentation into Scala compilation This is a hack to work around the problem of `auto-service` annotation processing creating files with same name in `java` and `scala` directories. This results in Jar having two files with the same name instead of one concatenated file. --- .../instrumentation/akkahttp/AkkaHttpServerInstrumentation.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename dd-java-agent/instrumentation/akka-http-10.0/src/main/{java => scala}/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java (100%) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java similarity index 100% rename from dd-java-agent/instrumentation/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java rename to dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java From bf0651e88b2dd84f4ba6754d621138d0b942e9e0 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 13:19:26 -0400 Subject: [PATCH 17/25] Make Jar build fail on duplicate files --- gradle/java.gradle | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/gradle/java.gradle b/gradle/java.gradle index 522a4fb34b..7d4c4e282a 100644 --- a/gradle/java.gradle +++ b/gradle/java.gradle @@ -30,6 +30,23 @@ if (configurations.find { it.name == 'jmh' }) { eclipse.classpath.plusConfigurations += [configurations.jmh] } +jar { + /* + Make Jar build fail on duplicate files + + By default Gradle Jar task can put multiple files with the same name + into a Jar. This may lead to confusion. For example if auto-service + annotation processing creates files with same name in `scala` and + `java` directory this would result in Jar having two files with the + same name in it. Which in turn would result in only one of those + files being actually considered when that Jar is used leading to very + confusing failures. + + Instead we should 'fail early' and avoid building such Jars. + */ + duplicatesStrategy = 'fail' +} + task packageSources(type: Jar) { classifier = 'sources' from sourceSets.main.allSource From a8af78ae1865da1f97ed4ea0b959adb091309b5e Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 13:37:28 -0400 Subject: [PATCH 18/25] Add task-tree gradle plugin that is useful in debugging sometimes --- dd-trace-java.gradle | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dd-trace-java.gradle b/dd-trace-java.gradle index 4e7a518753..14f1b8df42 100644 --- a/dd-trace-java.gradle +++ b/dd-trace-java.gradle @@ -7,13 +7,14 @@ buildscript { classpath "org.jfrog.buildinfo:build-info-extractor-gradle:4.7.3" classpath "com.jfrog.bintray.gradle:gradle-bintray-plugin:1.8.1" classpath "net.ltgt.gradle:gradle-errorprone-plugin:0.0.14" - classpath 'org.unbroken-dome.gradle-plugins:gradle-testsets-plugin:1.5.0' + classpath "org.unbroken-dome.gradle-plugins:gradle-testsets-plugin:1.5.0" } } plugins { id 'com.gradle.build-scan' version '1.14' id 'com.github.sherter.google-java-format' version '0.7.1' + id 'com.dorongold.task-tree' version '1.3' } def isCI = System.getenv("CI") != null From 7794eacc91767a9a4b42e7386ba4fcaa1e1e0b9d Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 15:30:21 -0400 Subject: [PATCH 19/25] Increase Cassandra request timeout in tests --- .../src/test/groovy/CassandraClientTest.groovy | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/test/groovy/CassandraClientTest.groovy b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/test/groovy/CassandraClientTest.groovy index 54a33fd29f..35b86a5d1a 100644 --- a/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/test/groovy/CassandraClientTest.groovy +++ b/dd-java-agent/instrumentation/datastax-cassandra-3.2/src/test/groovy/CassandraClientTest.groovy @@ -5,9 +5,13 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDTags import io.opentracing.tag.Tags import org.cassandraunit.utils.EmbeddedCassandraServerHelper +import spock.lang.Shared class CassandraClientTest extends AgentTestRunner { + @Shared + Cluster cluster + def setupSpec() { /* This timeout seems excessive but we've seen tests fail with timeout of 40s. @@ -16,6 +20,14 @@ class CassandraClientTest extends AgentTestRunner { tests would have to assume they run under shared Cassandra and act accordingly. */ EmbeddedCassandraServerHelper.startEmbeddedCassandra(120000L) + + cluster = EmbeddedCassandraServerHelper.getCluster() + + /* + Looks like sometimes our requests fail because Cassandra takes to long to respond, + Increase this timeout as well to try to cope with this. + */ + cluster.getConfiguration().getSocketOptions().setReadTimeoutMillis(120000) } def cleanupSpec() { @@ -24,7 +36,6 @@ class CassandraClientTest extends AgentTestRunner { def "sync traces"() { setup: - final Cluster cluster = EmbeddedCassandraServerHelper.getCluster() final Session session = cluster.newSession() session.execute("DROP KEYSPACE IF EXISTS sync_test") @@ -57,7 +68,6 @@ class CassandraClientTest extends AgentTestRunner { def "async traces"() { setup: - final Cluster cluster = EmbeddedCassandraServerHelper.getCluster() final Session session = cluster.connectAsync().get() session.executeAsync("DROP KEYSPACE IF EXISTS async_test").get() From 732bce82d01a0f2946bb1d72af39b8750d2857ff Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 18:02:08 -0400 Subject: [PATCH 20/25] Fix typo --- .../trace/instrumentation/spymemcached/SpymemcachedTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy b/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy index 6403bfda89..dfc50be1c5 100644 --- a/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy +++ b/dd-java-agent/instrumentation/spymemcached-2.12/src/test/groovy/datadog/trace/instrumentation/spymemcached/SpymemcachedTest.groovy @@ -30,7 +30,7 @@ import static CompletionListener.SPAN_TYPE import static datadog.trace.agent.test.TestUtils.runUnderTrace import static net.spy.memcached.ConnectionFactoryBuilder.Protocol.BINARY -// Do not run tests locally on Java7 since testcontainers are not compatibly with Java7 +// Do not run tests locally on Java7 since testcontainers are not compatible with Java7 // It is fine to run on CI because CI provides memcached externally, not through testcontainers @Requires({ "true" == System.getenv("CI") || jvm.java8Compatible }) class SpymemcachedTest extends AgentTestRunner { From 35b980ea89b00d4bc645975e35cb167566718f07 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 17 Jul 2018 18:02:16 -0400 Subject: [PATCH 21/25] Fix ElasticSearch config for 2.x client --- .../src/test/groovy/Elasticsearch2NodeClientTest.groovy | 2 +- .../src/test/groovy/Elasticsearch2SpringTemplateTest.groovy | 2 +- .../src/test/groovy/Elasticsearch2TransportClientTest.groovy | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2NodeClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2NodeClientTest.groovy index fb2b1406fb..27db9d3b18 100644 --- a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2NodeClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2NodeClientTest.groovy @@ -38,7 +38,7 @@ class Elasticsearch2NodeClientTest extends AgentTestRunner { def settings = Settings.builder() .put("path.home", esWorkingDir.path) // Since we use listeners to close spans this should make our span closing deterministic which is good for tests - .put("thread_pool.listener.size", 1) + .put("threadpool.listener.size", 1) .put("http.port", httpPort) .put("transport.tcp.port", tcpPort) .build() diff --git a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2SpringTemplateTest.groovy b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2SpringTemplateTest.groovy index bcec92ef99..050ab11305 100644 --- a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2SpringTemplateTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2SpringTemplateTest.groovy @@ -49,7 +49,7 @@ class Elasticsearch2SpringTemplateTest extends AgentTestRunner { def settings = Settings.builder() .put("path.home", esWorkingDir.path) // Since we use listeners to close spans this should make our span closing deterministic which is good for tests - .put("thread_pool.listener.size", 1) + .put("threadpool.listener.size", 1) .put("http.port", httpPort) .put("transport.tcp.port", tcpPort) .build() diff --git a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2TransportClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2TransportClientTest.groovy index 7b3d17fa99..c27c3a0840 100644 --- a/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2TransportClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch-transport-2/src/test/groovy/Elasticsearch2TransportClientTest.groovy @@ -50,7 +50,7 @@ class Elasticsearch2TransportClientTest extends AgentTestRunner { client = TransportClient.builder().settings( Settings.builder() // Since we use listeners to close spans this should make our span closing deterministic which is good for tests - .put("thread_pool.listener.size", 1) + .put("threadpool.listener.size", 1) .put("cluster.name", "test-cluster") .build() ).build() From 642b862c13ea76bd27ed7091bcefbf553db1565c Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Sat, 21 Jul 2018 23:55:45 -0400 Subject: [PATCH 22/25] Do not reference `AkkaHttpClientTransformFlow` in instrumentation code to avoid class loader problems --- .../akkahttp/AkkaHttpClientInstrumentation.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index 99db94e49c..72f2d4919e 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -54,10 +54,12 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { return new String[] { AkkaHttpClientInstrumentation.class.getName() + "$OnCompleteHandler", AkkaHttpClientInstrumentation.class.getName() + "$AkkaHttpHeaders", - AkkaHttpClientTransformFlow.class.getName(), - AkkaHttpClientTransformFlow.class.getName() + "$", - AkkaHttpClientTransformFlow.class.getName() + "$$anonfun$transform$1", - AkkaHttpClientTransformFlow.class.getName() + "$$anonfun$transform$2", + AkkaHttpClientInstrumentation.class.getPackage().getName() + ".AkkaHttpClientTransformFlow", + AkkaHttpClientInstrumentation.class.getPackage().getName() + ".AkkaHttpClientTransformFlow$", + AkkaHttpClientInstrumentation.class.getPackage().getName() + + ".AkkaHttpClientTransformFlow$$anonfun$transform$1", + AkkaHttpClientInstrumentation.class.getPackage().getName() + + ".AkkaHttpClientTransformFlow$$anonfun$transform$2", }; } From dcbf8d674a0d46ab57624eb5d30c624b074f4544 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 23 Jul 2018 10:44:57 -0400 Subject: [PATCH 23/25] Akka-http-client: handle `singleRquest` throwing an exception --- .../AkkaHttpClientInstrumentation.java | 39 +++++++++++++------ .../AkkaHttpClientInstrumentationTest.groovy | 30 ++++++++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java index 72f2d4919e..10ac93b274 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java @@ -15,6 +15,7 @@ import datadog.trace.api.DDSpanTypes; import datadog.trace.api.DDTags; import io.opentracing.Scope; import io.opentracing.Span; +import io.opentracing.Tracer; import io.opentracing.propagation.Format; import io.opentracing.propagation.TextMap; import io.opentracing.tag.Tags; @@ -79,31 +80,45 @@ public final class AkkaHttpClientInstrumentation extends Instrumenter.Default { @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope methodEnter( @Advice.Argument(value = 0, readOnly = false) HttpRequest request) { - Scope scope = + Tracer.SpanBuilder builder = GlobalTracer.get() .buildSpan("akka-http.request") .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT) - .withTag(Tags.HTTP_METHOD.getKey(), request.method().value()) .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT) - .withTag(Tags.COMPONENT.getKey(), "akka-http-client") - .withTag(Tags.HTTP_URL.getKey(), request.getUri().toString()) - .startActive(false); + .withTag(Tags.COMPONENT.getKey(), "akka-http-client"); + if (request != null) { + builder = + builder + .withTag(Tags.HTTP_METHOD.getKey(), request.method().value()) + .withTag(Tags.HTTP_URL.getKey(), request.getUri().toString()); + } + Scope scope = builder.startActive(false); - AkkaHttpHeaders headers = new AkkaHttpHeaders(request); - GlobalTracer.get().inject(scope.span().context(), Format.Builtin.HTTP_HEADERS, headers); - // Request is immutable, so we have to assign new value once we update headers - request = headers.getRequest(); + if (request != null) { + AkkaHttpHeaders headers = new AkkaHttpHeaders(request); + GlobalTracer.get().inject(scope.span().context(), Format.Builtin.HTTP_HEADERS, headers); + // Request is immutable, so we have to assign new value once we update headers + request = headers.getRequest(); + } return scope; } - @Advice.OnMethodExit(suppress = Throwable.class) + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void methodExit( @Advice.Argument(value = 0) final HttpRequest request, @Advice.This final HttpExt thiz, @Advice.Return final Future responseFuture, - @Advice.Enter final Scope scope) { - responseFuture.onComplete(new OnCompleteHandler(scope.span()), thiz.system().dispatcher()); + @Advice.Enter final Scope scope, + @Advice.Thrown final Throwable throwable) { + Span span = scope.span(); + if (throwable == null) { + responseFuture.onComplete(new OnCompleteHandler(span), thiz.system().dispatcher()); + } else { + Tags.ERROR.set(span, Boolean.TRUE); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); + span.finish(); + } scope.close(); } } diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy index dbb5950507..d63b329596 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy @@ -153,6 +153,36 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { } } + def "singleRequest exception trace" () { + when: + // Passing null causes NPE in singleRequest + Http.get(system).singleRequest(null, materializer) + + then: + thrown NullPointerException + assertTraces(TEST_WRITER, 1) { + trace(0, 1) { + span(0) { + parent() + serviceName "unnamed-java-app" + operationName "akka-http.request" + resourceName "akka-http.request" + errored true + tags { + defaultTags() + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT + "$Tags.COMPONENT.key" "akka-http-client" + "$Tags.ERROR.key" true + errorTags(NullPointerException) + } + } + } + } + + } + + def "#route pool request trace" () { setup: def url = server.address.resolve("/" + route).toURL() From 989091847a62a3515f04e49cb5e513d6094f258a Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 23 Jul 2018 12:33:41 -0400 Subject: [PATCH 24/25] Try to reduce gradle heap size in a hope to fix the build --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5806fafaf6..81cfb824f9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -29,7 +29,7 @@ jobs: - run: name: Build Project - command: GRADLE_OPTS="-Dorg.gradle.jvmargs=-Xmx2G -Xms512M" ./gradlew clean :dd-java-agent:shadowJar compileTestGroovy compileTestScala compileTestJava check -x test -x latestDepTest -x traceAgentTest --build-cache --parallel --stacktrace --no-daemon --max-workers=4 + command: GRADLE_OPTS="-Dorg.gradle.jvmargs=-Xmx1G -Xms64M" ./gradlew clean :dd-java-agent:shadowJar compileTestGroovy compileTestScala compileTestJava check -x test -x latestDepTest -x traceAgentTest --build-cache --parallel --stacktrace --no-daemon --max-workers=4 - run: name: Collect Libs From 3d8e76c2a4963803949526f8c56eb97d2286a00b Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 23 Jul 2018 13:35:27 -0400 Subject: [PATCH 25/25] Get rid of `WRITER_PHASER` We have alternative way of doing the same thing and `Phaser` seems to be not very correct way of doing this anyway. --- .../src/test/groovy/KafkaClientTest.groovy | 3 +-- .../src/test/groovy/KafkaStreamsTest.groovy | 5 ++--- .../main/java/datadog/trace/agent/test/AgentTestRunner.java | 5 ----- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/instrumentation/kafka-clients-0.11/src/test/groovy/KafkaClientTest.groovy b/dd-java-agent/instrumentation/kafka-clients-0.11/src/test/groovy/KafkaClientTest.groovy index ab14e82dbc..da2680dd7e 100644 --- a/dd-java-agent/instrumentation/kafka-clients-0.11/src/test/groovy/KafkaClientTest.groovy +++ b/dd-java-agent/instrumentation/kafka-clients-0.11/src/test/groovy/KafkaClientTest.groovy @@ -44,11 +44,10 @@ class KafkaClientTest extends AgentTestRunner { def records = new LinkedBlockingQueue>() // setup a Kafka message listener - WRITER_PHASER.register() container.setupMessageListener(new MessageListener() { @Override void onMessage(ConsumerRecord record) { - WRITER_PHASER.arriveAndAwaitAdvance() // ensure consistent ordering of traces + TEST_WRITER.waitForTraces(1) // ensure consistent ordering of traces records.add(record) } }) diff --git a/dd-java-agent/instrumentation/kafka-streams-0.11/src/test/groovy/KafkaStreamsTest.groovy b/dd-java-agent/instrumentation/kafka-streams-0.11/src/test/groovy/KafkaStreamsTest.groovy index eece550b7d..fe4f533e53 100644 --- a/dd-java-agent/instrumentation/kafka-streams-0.11/src/test/groovy/KafkaStreamsTest.groovy +++ b/dd-java-agent/instrumentation/kafka-streams-0.11/src/test/groovy/KafkaStreamsTest.groovy @@ -43,14 +43,13 @@ class KafkaStreamsTest extends AgentTestRunner { def consumerContainer = new KafkaMessageListenerContainer<>(consumerFactory, new ContainerProperties(STREAM_PROCESSED)) // create a thread safe queue to store the processed message - WRITER_PHASER.register() def records = new LinkedBlockingQueue>() // setup a Kafka message listener consumerContainer.setupMessageListener(new MessageListener() { @Override void onMessage(ConsumerRecord record) { - WRITER_PHASER.arriveAndAwaitAdvance() // ensure consistent ordering of traces + TEST_WRITER.waitForTraces(1) // ensure consistent ordering of traces getTestTracer().activeSpan().setTag("testing", 123) records.add(record) } @@ -69,7 +68,7 @@ class KafkaStreamsTest extends AgentTestRunner { .mapValues(new ValueMapper() { @Override String apply(String textLine) { - WRITER_PHASER.arriveAndAwaitAdvance() // ensure consistent ordering of traces + TEST_WRITER.waitForTraces(1) // ensure consistent ordering of traces getTestTracer().activeSpan().setTag("asdf", "testing") return textLine.toLowerCase() } diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java index f65756df0e..96d1e8f348 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java @@ -16,7 +16,6 @@ import java.util.List; import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.Phaser; import java.util.concurrent.atomic.AtomicInteger; import net.bytebuddy.agent.ByteBuddyAgent; import net.bytebuddy.agent.builder.AgentBuilder; @@ -69,8 +68,6 @@ public abstract class AgentTestRunner extends Specification { private static final Instrumentation instrumentation; private static volatile ClassFileTransformer activeTransformer = null; - protected static final Phaser WRITER_PHASER = new Phaser(); - static { instrumentation = ByteBuddyAgent.getInstrumentation(); @@ -82,7 +79,6 @@ public abstract class AgentTestRunner extends Specification { @Override public boolean add(final List trace) { final boolean result = super.add(trace); - WRITER_PHASER.arriveAndDeregister(); return result; } }; @@ -136,7 +132,6 @@ public abstract class AgentTestRunner extends Specification { @Before public void beforeTest() { TEST_WRITER.start(); - WRITER_PHASER.register(); INSTRUMENTATION_ERROR_COUNT.set(0); ERROR_LISTENER.activateTest(this); assert getTestTracer().activeSpan() == null : "Span is active before test has started";