From abffe1779b727d8ceeebc25b40a2c9e15421aadb Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Tue, 7 Aug 2018 14:37:41 -0400 Subject: [PATCH 1/2] Remove Util Logger and Refactor test OKHttp instrumentation will not be using the java util logger, also applies the string representaiton of ip address for IPV4/6 tags. Refactored test. --- .../okhttp3/OkHttpClientSpanDecorator.java | 13 ++- .../okhttp3/TracingInterceptor.java | 6 +- .../src/test/groovy/OkHttp3Test.groovy | 88 ++++++++----------- 3 files changed, 48 insertions(+), 59 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 2e1ddd7616..0d8f265ccc 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,8 +4,7 @@ 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.net.Inet6Address; import java.util.Collections; import okhttp3.Connection; import okhttp3.Request; @@ -75,12 +74,12 @@ public interface OkHttpClientSpanDecorator { Tags.PEER_HOSTNAME.set(span, connection.socket().getInetAddress().getHostName()); Tags.PEER_PORT.set(span, connection.socket().getPort()); - if (connection.socket().getInetAddress() instanceof Inet4Address) { - final byte[] address = connection.socket().getInetAddress().getAddress(); - Tags.PEER_HOST_IPV4.set(span, ByteBuffer.wrap(address).getInt()); - } else { - Tags.PEER_HOST_IPV6.set(span, connection.socket().getInetAddress().toString()); + final String address = connection.socket().getInetAddress().getHostAddress(); + String ipvKey = Tags.PEER_HOST_IPV4.getKey(); + if (connection.socket().getInetAddress() instanceof Inet6Address) { + ipvKey = Tags.PEER_HOST_IPV6.getKey(); } + span.setTag(ipvKey, address); } }; } diff --git a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java index 29a5b7fad5..c0775443d6 100644 --- a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java +++ b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java @@ -12,12 +12,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.Executors; -import java.util.logging.Logger; import okhttp3.Dispatcher; import okhttp3.Interceptor; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; +import org.slf4j.LoggerFactory; /** * OkHttp interceptor to trace client requests. Interceptor adds span context into outgoing @@ -34,7 +34,6 @@ import okhttp3.Response; * @author Pavol Loffay */ public class TracingInterceptor implements Interceptor { - private static final Logger log = Logger.getLogger(TracingInterceptor.class.getName()); private final Tracer tracer; private final List decorators; @@ -120,7 +119,8 @@ public class TracingInterceptor implements Interceptor { tracer, tagWrapper.getSpan().context(), decorators) .intercept(chain); } else { - log.severe("tag is null or not an instance of TagWrapper, skipping decorator onResponse()"); + LoggerFactory.getLogger(TracingInterceptor.class.getName()) + .error("tag is null or not an instance of TagWrapper, skipping decorator onResponse()"); } } diff --git a/dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy b/dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy index 6a24a5e357..5ebefa5130 100644 --- a/dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy +++ b/dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy @@ -1,6 +1,5 @@ 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 @@ -8,6 +7,7 @@ import ratpack.http.Headers import java.util.concurrent.atomic.AtomicReference +import static datadog.trace.agent.test.ListWriterAssert.assertTraces import static ratpack.groovy.test.embed.GroovyEmbeddedApp.ratpack class OkHttp3Test extends AgentTestRunner { @@ -32,55 +32,45 @@ class OkHttp3Test extends AgentTestRunner { expect: response.body.string() == "pong" - TEST_WRITER.size() == 1 + assertTraces(TEST_WRITER, 1) { + trace(0, 2) { + span(0) { + operationName "okhttp.http" + serviceName "okhttp" + resourceName "okhttp.http" + spanType DDSpanTypes.HTTP_CLIENT + errored false + parent() + tags { + "component" "okhttp" + "span.type" DDSpanTypes.HTTP_CLIENT + defaultTags() + } + } + span(1) { + operationName "okhttp.http" + serviceName "okhttp" + resourceName "GET /ping" + errored false + childOf(span(0)) + tags { + defaultTags() + "component" "okhttp" + "span.type" DDSpanTypes.HTTP_CLIENT + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.HTTP_STATUS.key" 200 + "$Tags.HTTP_URL.key" "http://localhost:$server.address.port/ping" + "$Tags.PEER_HOSTNAME.key" "localhost" + "$Tags.PEER_PORT.key" server.address.port + "$Tags.PEER_HOST_IPV4.key" "127.0.0.1" + } + } + } + } - def trace = TEST_WRITER.firstTrace() - trace.size() == 2 - - and: // span 0 - def span1 = trace[0] - - span1.context().operationName == "okhttp.http" - span1.serviceName == "okhttp" - span1.resourceName == "okhttp.http" - span1.type == DDSpanTypes.HTTP_CLIENT - !span1.context().getErrorFlag() - span1.context().parentId == "0" - - - def tags1 = span1.context().tags - tags1["component"] == "okhttp" - tags1["span.type"] == DDSpanTypes.HTTP_CLIENT - tags1["thread.name"] != null - tags1["thread.id"] != null - tags1.size() == 4 - - and: // span 1 - def span2 = trace[1] - - span2.context().operationName == "okhttp.http" - span2.serviceName == "okhttp" - span2.resourceName == "GET /ping" - span2.type == DDSpanTypes.HTTP_CLIENT - !span2.context().getErrorFlag() - span2.context().parentId == span1.spanId - - - def tags2 = span2.context().tags - tags2[Tags.COMPONENT.key] == "okhttp" - tags2[Tags.SPAN_KIND.key] == Tags.SPAN_KIND_CLIENT - tags2[DDTags.SPAN_TYPE] == DDSpanTypes.HTTP_CLIENT - tags2[Tags.HTTP_METHOD.key] == "GET" - tags2[Tags.HTTP_URL.key] == "http://localhost:$server.address.port/ping" - tags2[Tags.PEER_HOSTNAME.key] == "localhost" - tags2[Tags.PEER_PORT.key] == server.address.port - tags2[Tags.PEER_HOST_IPV4.key] != null - tags2[DDTags.THREAD_NAME] != null - tags2[DDTags.THREAD_ID] != null - tags2.size() == 11 - - receivedHeaders.get().get("x-datadog-trace-id") == "$span2.traceId" - receivedHeaders.get().get("x-datadog-parent-id") == "$span2.spanId" + receivedHeaders.get().get("x-datadog-trace-id") == TEST_WRITER[0][1].traceId + receivedHeaders.get().get("x-datadog-parent-id") == TEST_WRITER[0][1].spanId cleanup: server.close() From 2041d56de8a9111e778a6523e98dae7be2a9cc61 Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Tue, 7 Aug 2018 16:49:51 -0400 Subject: [PATCH 2/2] use Slf4j annotation. --- .../okhttp3/OkHttpClientSpanDecorator.java | 10 ++++++---- .../instrumentation/okhttp3/TracingInterceptor.java | 6 +++--- 2 files changed, 9 insertions(+), 7 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 0d8f265ccc..546e876cce 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 @@ -5,6 +5,7 @@ import static io.opentracing.log.Fields.ERROR_OBJECT; import io.opentracing.Span; import io.opentracing.tag.Tags; import java.net.Inet6Address; +import java.net.InetAddress; import java.util.Collections; import okhttp3.Connection; import okhttp3.Request; @@ -70,16 +71,17 @@ public interface OkHttpClientSpanDecorator { @Override public void onResponse( final Connection connection, final Response response, final Span span) { + final InetAddress inetAddress = connection.socket().getInetAddress(); + Tags.HTTP_STATUS.set(span, response.code()); - Tags.PEER_HOSTNAME.set(span, connection.socket().getInetAddress().getHostName()); + Tags.PEER_HOSTNAME.set(span, inetAddress.getHostName()); Tags.PEER_PORT.set(span, connection.socket().getPort()); - final String address = connection.socket().getInetAddress().getHostAddress(); String ipvKey = Tags.PEER_HOST_IPV4.getKey(); - if (connection.socket().getInetAddress() instanceof Inet6Address) { + if (inetAddress instanceof Inet6Address) { ipvKey = Tags.PEER_HOST_IPV6.getKey(); } - span.setTag(ipvKey, address); + span.setTag(ipvKey, inetAddress.getHostAddress()); } }; } diff --git a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java index c0775443d6..a305e6ff45 100644 --- a/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java +++ b/dd-java-agent/instrumentation/okhttp-3/src/main/java/datadog/trace/instrumentation/okhttp3/TracingInterceptor.java @@ -12,12 +12,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.Executors; +import lombok.extern.slf4j.Slf4j; import okhttp3.Dispatcher; import okhttp3.Interceptor; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; -import org.slf4j.LoggerFactory; /** * OkHttp interceptor to trace client requests. Interceptor adds span context into outgoing @@ -33,6 +33,7 @@ import org.slf4j.LoggerFactory; * * @author Pavol Loffay */ +@Slf4j public class TracingInterceptor implements Interceptor { private final Tracer tracer; @@ -119,8 +120,7 @@ public class TracingInterceptor implements Interceptor { tracer, tagWrapper.getSpan().context(), decorators) .intercept(chain); } else { - LoggerFactory.getLogger(TracingInterceptor.class.getName()) - .error("tag is null or not an instance of TagWrapper, skipping decorator onResponse()"); + log.error("tag is null or not an instance of TagWrapper, skipping decorator onResponse()"); } }