Refactor Apache HttpClient Instrumentation.
Removes the need for DDTracingClientExec.
Reduces extra spans.
Does not include a span for each redirect.
Compatible with 4.0+ instead of just 4.3+.
DELETE
dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java
-package datadog.trace.instrumentation.apachehttpclient;
-
-import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType;
-import static io.opentracing.log.Fields.ERROR_OBJECT;
-import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
-import static net.bytebuddy.matcher.ElementMatchers.isMethod;
-import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.not;
-
-import com.google.auto.service.AutoService;
-import datadog.trace.agent.tooling.Instrumenter;
-import io.opentracing.Scope;
-import io.opentracing.Span;
-import io.opentracing.Tracer;
-import io.opentracing.tag.Tags;
-import io.opentracing.util.GlobalTracer;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-import net.bytebuddy.asm.Advice;
-import net.bytebuddy.description.type.TypeDescription;
-import net.bytebuddy.matcher.ElementMatcher;
-import org.apache.http.impl.execchain.ClientExecChain;
-
-@AutoService(Instrumenter.class)
-public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
-
- public ApacheHttpClientInstrumentation() {
- super("httpclient");
- }
-
- @Override
- public ElementMatcher<TypeDescription> typeMatcher() {
- return named("org.apache.http.impl.client.HttpClientBuilder")
- .or(safeHasSuperType(named("org.apache.http.impl.client.CloseableHttpClient")));
- }
-
- @Override
- public String[] helperClassNames() {
- return new String[] {
- "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec",
- "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec$HttpHeadersInjectAdapter"
- };
- }
-
- @Override
- public Map<ElementMatcher, String> transformers() {
- final Map<ElementMatcher, String> transformers = new HashMap<>();
- transformers.put(
- isMethod().and(not(isAbstract())).and(named("doExecute")), ClientAdvice.class.getName());
- transformers.put(
- isMethod().and(named("decorateProtocolExec")), ClientExecAdvice.class.getName());
- return transformers;
- }
-
- public static class ClientAdvice {
-
- @Advice.OnMethodEnter(suppress = Throwable.class)
- public static Scope methodEnter() {
- final Tracer.SpanBuilder spanBuilder =
- GlobalTracer.get()
- .buildSpan(DDTracingClientExec.OPERATION_NAME)
- .withTag(Tags.COMPONENT.getKey(), DDTracingClientExec.COMPONENT_NAME);
- return spanBuilder.startActive(true);
- }
-
- @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
- public static void methodExit(
- @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) {
- final Span span = scope.span();
- if (throwable != null) {
- Tags.ERROR.set(span, Boolean.TRUE);
- span.log(Collections.singletonMap(ERROR_OBJECT, throwable));
- span.finish();
- }
- scope.close();
- }
- }
-
- public static class ClientExecAdvice {
- @Advice.OnMethodExit(suppress = Throwable.class)
- public static void addTracingExec(@Advice.Return(readOnly = false) ClientExecChain execChain) {
- execChain = new DDTracingClientExec(execChain, GlobalTracer.get());
- }
- }
-}

DELETE
dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/DDTracingClientExec.java
-package datadog.trace.instrumentation.apachehttpclient;
-
-import static io.opentracing.log.Fields.ERROR_OBJECT;
-
-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;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.Map;
-import lombok.extern.slf4j.Slf4j;
-import org.apache.http.HttpException;
-import org.apache.http.HttpRequest;
-import org.apache.http.client.methods.CloseableHttpResponse;
-import org.apache.http.client.methods.HttpExecutionAware;
-import org.apache.http.client.methods.HttpRequestWrapper;
-import org.apache.http.client.protocol.HttpClientContext;
-import org.apache.http.conn.routing.HttpRoute;
-import org.apache.http.impl.execchain.ClientExecChain;
-
-/**
- * Tracing is added before {@link org.apache.http.impl.execchain.ProtocolExec} which is invoked as
- * the next to last. Note that {@link org.apache.http.impl.execchain.RedirectExec} is invoked before
- * so this exec has to deal with redirects.
- */
-@Slf4j
-public class DDTracingClientExec implements ClientExecChain {
- static final String COMPONENT_NAME = "apache-httpclient";
- static final String OPERATION_NAME = "http.request";
-
- private final ClientExecChain requestExecutor;
-
- private final Tracer tracer;
-
- public DDTracingClientExec(final ClientExecChain clientExecChain, final Tracer tracer) {
- requestExecutor = clientExecChain;
- this.tracer = tracer;
- }
-
- @Override
- public CloseableHttpResponse execute(
- final HttpRoute route,
- final HttpRequestWrapper request,
- final HttpClientContext clientContext,
- final HttpExecutionAware execAware)
- throws IOException, HttpException {
- Scope scope = null;
- Span span = null;
- try {
- // This handlers runs untrapped in the client code
- // so we must ensure any unexpected agent errors are caught.
- try {
- scope =
- tracer
- .buildSpan(OPERATION_NAME)
- .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT)
- .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT)
- .startActive(true);
- span = scope.span();
-
- final boolean awsClientCall = request.getHeaders("amz-sdk-invocation-id").length > 0;
- // AWS calls are often signed, so we can't add headers without breaking the signature.
- if (!awsClientCall) {
- tracer.inject(
- span.context(), Format.Builtin.HTTP_HEADERS, new HttpHeadersInjectAdapter(request));
- }
- // request tags
- Tags.HTTP_METHOD.set(span, request.getRequestLine().getMethod());
- Tags.HTTP_URL.set(span, request.getRequestLine().getUri());
- final URI uri = request.getURI();
- // zuul users have encountered cases where getURI returns null
- if (null != uri) {
- Tags.PEER_PORT.set(span, uri.getPort() == -1 ? 80 : uri.getPort());
- Tags.PEER_HOSTNAME.set(span, uri.getHost());
- }
- } catch (final Exception e) {
- log.debug("failed to create span", e);
- }
-
- final CloseableHttpResponse response =
- requestExecutor.execute(route, request, clientContext, execAware);
-
- try {
- // response tags
- if (null != span) {
- Tags.HTTP_STATUS.set(span, response.getStatusLine().getStatusCode());
- }
- } catch (final Exception e) {
- log.debug("failed to set span status", e);
- }
-
- return response;
- } catch (final IOException | HttpException | RuntimeException e) {
- // error tags
- Tags.ERROR.set(span, Boolean.TRUE);
- span.log(Collections.singletonMap(ERROR_OBJECT, e));
-
- throw e;
- } finally {
- if (null != scope) {
- scope.close();
- }
- }
- }
-
- public static class HttpHeadersInjectAdapter implements TextMap {
-
- private final HttpRequest httpRequest;
-
- public HttpHeadersInjectAdapter(final HttpRequest httpRequest) {
- this.httpRequest = httpRequest;
- }
-
- @Override
- public void put(final String key, final String value) {
- httpRequest.addHeader(key, value);
- }
-
- @Override
- public Iterator<Map.Entry<String, String>> iterator() {
- throw new UnsupportedOperationException(
- "This class should be used only with tracer#inject()");
- }
- }
-}

RENAME
dd-java-agent/instrumentation/apache-httpclient-4.3/apache-httpclient-4.3.gradle
dd-java-agent/instrumentation/apache-httpclient-4/apache-httpclient-4.gradle
@@ -4,12 +4,12 @@ muzzle {
fail {
group = "commons-httpclient"
module = "commons-httpclient"
- versions = "[,4.3)"
+ versions = "[,4.0)"
}
pass {
group = "org.apache.httpcomponents"
module = "httpclient"
- versions = "[4.3,)"
+ versions = "[4.0,)"
assertInverse = true
}
}
@@ -17,13 +17,11 @@ muzzle {
apply plugin: 'org.unbroken-dome.test-sets'
testSets {
- latestDepTest {
- dirName = 'test'
- }
+ latestDepTest
}
dependencies {
- compileOnly group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.3'
+ compileOnly group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.0'
compile project(':dd-java-agent:agent-tooling')
@@ -33,8 +31,7 @@ dependencies {
implementation deps.autoservice
testCompile project(':dd-java-agent:testing')
- testCompile group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.3'
-
+ testCompile group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.0'
latestDepTestCompile group: 'org.apache.httpcomponents', name: 'httpclient', version: '+'
}

RENAME
dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy
dd-java-agent/instrumentation/apache-httpclient-4/src/latestDepTest/groovy/ApacheHttpClientTest.groovy
@@ -9,11 +9,13 @@ import org.apache.http.client.ClientProtocolException
import org.apache.http.client.HttpClient
import org.apache.http.client.config.RequestConfig
import org.apache.http.client.methods.HttpGet
+import org.apache.http.impl.client.BasicResponseHandler
import org.apache.http.impl.client.HttpClientBuilder
import org.apache.http.message.BasicHeader
import spock.lang.AutoCleanup
import spock.lang.Shared
+import static datadog.trace.agent.test.TestUtils.runUnderTrace
import static datadog.trace.agent.test.asserts.ListWriterAssert.assertTraces
import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer
@@ -25,7 +27,7 @@ class ApacheHttpClientTest extends AgentTestRunner {
handlers {
prefix("success") {
handleDistributedRequest()
- String msg = "<html><body><h1>Hello test.</h1>\n"
+ String msg = "Hello."
response.status(200).send(msg)
}
prefix("redirect") {
@@ -46,24 +48,35 @@ class ApacheHttpClientTest extends AgentTestRunner {
def redirectUrl = server.address.resolve("/redirect")
@Shared
def twoRedirectsUrl = server.address.resolve("/another-redirect")
+ @Shared
+ def handler = new BasicResponseHandler()
final HttpClientBuilder builder = HttpClientBuilder.create()
final HttpClient client = builder.build()
def "trace request with propagation"() {
when:
- HttpResponse response = client.execute(new HttpGet(successUrl))
+ String response = runUnderTrace("parent") {
+ if (responseHandler) {
+ client.execute(new HttpGet(successUrl), responseHandler)
+ } else {
+ client.execute(new HttpGet(successUrl)).entity.content.text
+ }
+ }
then:
- response.getStatusLine().getStatusCode() == 200
+ response == "Hello."
// one trace on the server, one trace on the client
assertTraces(TEST_WRITER, 2) {
server.distributedRequestTrace(it, 0, TEST_WRITER[1][1])
trace(1, 2) {
- clientParentSpan(it, 0)
+ parentSpan(it, 0)
successClientSpan(it, 1, span(0))
}
}
+
+ where:
+ responseHandler << [null, handler]
}
def "trace redirected request with propagation many redirects allowed"() {
@@ -75,18 +88,19 @@ class ApacheHttpClientTest extends AgentTestRunner {
request.setConfig(requestConfigBuilder.build())
when:
- HttpResponse response = client.execute(request)
+ HttpResponse response = runUnderTrace("parent") {
+ client.execute(request)
+ }
then:
response.getStatusLine().getStatusCode() == 200
// two traces on the server, one trace on the client
assertTraces(TEST_WRITER, 3) {
- server.distributedRequestTrace(it, 0, TEST_WRITER[2][2])
+ server.distributedRequestTrace(it, 0, TEST_WRITER[2][1])
server.distributedRequestTrace(it, 1, TEST_WRITER[2][1])
- trace(2, 3) {
- clientParentSpan(it, 0)
- successClientSpan(it, 1, span(0))
- redirectClientSpan(it, 2, span(0))
+ trace(2, 2) {
+ parentSpan(it, 0)
+ successClientSpan(it, 1, span(0), 200, "redirect")
}
}
}
@@ -99,18 +113,19 @@ class ApacheHttpClientTest extends AgentTestRunner {
request.setConfig(requestConfigBuilder.build())
when:
- HttpResponse response = client.execute(request)
+ HttpResponse response = runUnderTrace("parent") {
+ client.execute(request)
+ }
then:
response.getStatusLine().getStatusCode() == 200
// two traces on the server, one trace on the client
assertTraces(TEST_WRITER, 3) {
- server.distributedRequestTrace(it, 0, TEST_WRITER[2][2])
+ server.distributedRequestTrace(it, 0, TEST_WRITER[2][1])
server.distributedRequestTrace(it, 1, TEST_WRITER[2][1])
- trace(2, 3) {
- clientParentSpan(it, 0)
- successClientSpan(it, 1, span(0))
- redirectClientSpan(it, 2, span(0))
+ trace(2, 2) {
+ parentSpan(it, 0)
+ successClientSpan(it, 1, span(0), 200, "redirect")
}
}
}
@@ -124,18 +139,19 @@ class ApacheHttpClientTest extends AgentTestRunner {
request.setConfig(requestConfigBuilder.build())
when:
+ runUnderTrace("parent") {
client.execute(request)
+ }
then:
def exception = thrown(ClientProtocolException)
// two traces on the server, one trace on the client
assertTraces(TEST_WRITER, 3) {
- server.distributedRequestTrace(it, 0, TEST_WRITER[2][2])
+ server.distributedRequestTrace(it, 0, TEST_WRITER[2][1])
server.distributedRequestTrace(it, 1, TEST_WRITER[2][1])
- trace(2, 3) {
- clientParentSpan(it, 0, exception)
- redirectClientSpan(it, 1, span(0))
- redirectClientSpan(it, 2, span(0), "another-redirect")
+ trace(2, 2) {
+ parentSpan(it, 0, exception)
+ successClientSpan(it, 1, span(0), null, "another-redirect", exception)
}
}
}
@@ -146,29 +162,30 @@ class ApacheHttpClientTest extends AgentTestRunner {
request.addHeader(new BasicHeader("is-dd-server", "false"))
when:
- HttpResponse response = client.execute(request)
+ HttpResponse response = runUnderTrace("parent") {
+ client.execute(request)
+ }
then:
response.getStatusLine().getStatusCode() == 200
// only one trace (client).
assertTraces(TEST_WRITER, 1) {
trace(0, 2) {
- clientParentSpan(it, 0)
+ parentSpan(it, 0)
successClientSpan(it, 1, span(0))
}
}
}
- def clientParentSpan(TraceAssert trace, int index, Throwable exception = null) {
+ def parentSpan(TraceAssert trace, int index, Throwable exception = null) {
trace.span(index) {
parent()
serviceName "unnamed-java-app"
- operationName "apache.http"
- resourceName "apache.http"
+ operationName "parent"
+ resourceName "parent"
errored exception != null
tags {
defaultTags()
- "$Tags.COMPONENT.key" "apache-httpclient"
if (exception) {
errorTags(exception.class)
}
@@ -176,15 +193,19 @@ class ApacheHttpClientTest extends AgentTestRunner {
}
}
- def successClientSpan(TraceAssert trace, int index, DDSpan parent, status = 200, route = "success") {
+ def successClientSpan(TraceAssert trace, int index, DDSpan parent, status = 200, route = "success", Throwable exception = null) {
trace.span(index) {
childOf parent
serviceName "unnamed-java-app"
- operationName "http.request"
+ operationName "apache.http.request"
resourceName "GET /$route"
- errored false
+ errored exception != null
tags {
defaultTags()
+ if (exception) {
+ errorTags(exception.class)
+ }
+ "$Tags.COMPONENT.key" "apache-httpclient"
"$Tags.HTTP_STATUS.key" status
"$Tags.HTTP_URL.key" "http://localhost:$port/$route"
"$Tags.PEER_HOSTNAME.key" "localhost"
@@ -195,8 +216,4 @@ class ApacheHttpClientTest extends AgentTestRunner {
}
}
}
-
- def redirectClientSpan(TraceAssert trace, int index, DDSpan parent, route = "redirect") {
- successClientSpan(trace, index, parent, 302, route)
- }
}

ADD
dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java
+package datadog.trace.instrumentation.apachehttpclient;
+
+import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType;
+import static io.opentracing.log.Fields.ERROR_OBJECT;
+import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.not;
+import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
+
+import com.google.auto.service.AutoService;
+import datadog.trace.agent.tooling.Instrumenter;
+import datadog.trace.api.DDSpanTypes;
+import datadog.trace.api.DDTags;
+import datadog.trace.bootstrap.CallDepthThreadLocalMap;
+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;
+import io.opentracing.util.GlobalTracer;
+import java.io.IOException;
+import java.net.URI;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.Map;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.bytecode.assign.Assigner;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.http.HttpRequest;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.ResponseHandler;
+import org.apache.http.client.methods.HttpUriRequest;
+
+@AutoService(Instrumenter.class)
+public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
+
+ public ApacheHttpClientInstrumentation() {
+ super("httpclient");
+ }
+
+ @Override
+ public ElementMatcher<TypeDescription> typeMatcher() {
+ return safeHasSuperType(named("org.apache.http.client.HttpClient"));
+ }
+
+ @Override
+ public String[] helperClassNames() {
+ return new String[] {
+ getClass().getName() + "$HttpHeadersInjectAdapter",
+ getClass().getName() + "$WrappingStatusSettingResponseHandler",
+ };
+ }
+
+ @Override
+ public Map<? extends ElementMatcher, String> transformers() {
+ return Collections.singletonMap(
+ isMethod()
+ .and(not(isAbstract()))
+ .and(named("execute"))
+ .and(takesArgument(0, named("org.apache.http.client.methods.HttpUriRequest"))),
+ ClientAdvice.class.getName());
+ }
+
+ public static class ClientAdvice {
+
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static Scope methodEnter(
+ @Advice.Argument(0) final HttpUriRequest request,
+ // ResponseHandler could be either slot, but not both.
+ @Advice.Argument(
+ value = 1,
+ optional = true,
+ typing = Assigner.Typing.DYNAMIC,
+ readOnly = false)
+ Object handler1,
+ @Advice.Argument(
+ value = 2,
+ optional = true,
+ typing = Assigner.Typing.DYNAMIC,
+ readOnly = false)
+ Object handler2) {
+ final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class);
+ if (callDepth > 0) {
+ return null;
+ }
+ final Tracer tracer = GlobalTracer.get();
+ final Scope scope =
+ tracer
+ .buildSpan("apache.http.request")
+ .withTag(Tags.COMPONENT.getKey(), "apache-httpclient")
+ .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT)
+ .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT)
+ .withTag(Tags.HTTP_METHOD.getKey(), request.getRequestLine().getMethod())
+ .withTag(Tags.HTTP_URL.getKey(), request.getRequestLine().getUri())
+ .startActive(true);
+
+ final Span span = scope.span();
+
+ // Wrap the handler so we capture the status code
+ if (handler1 instanceof ResponseHandler) {
+ handler1 = new WrappingStatusSettingResponseHandler(span, (ResponseHandler) handler1);
+ } else if (handler2 instanceof ResponseHandler) {
+ handler2 = new WrappingStatusSettingResponseHandler(span, (ResponseHandler) handler2);
+ }
+
+ final boolean awsClientCall = request.getHeaders("amz-sdk-invocation-id").length > 0;
+ // AWS calls are often signed, so we can't add headers without breaking the signature.
+ if (!awsClientCall) {
+ tracer.inject(
+ span.context(), Format.Builtin.HTTP_HEADERS, new HttpHeadersInjectAdapter(request));
+ }
+ final URI uri = request.getURI();
+ // zuul users have encountered cases where getURI returns null
+ if (null != uri) {
+ Tags.PEER_PORT.set(span, uri.getPort() == -1 ? 80 : uri.getPort());
+ Tags.PEER_HOSTNAME.set(span, uri.getHost());
+ }
+ return scope;
+ }
+
+ @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+ public static void methodExit(
+ @Advice.Enter final Scope scope,
+ @Advice.Return final Object result,
+ @Advice.Thrown final Throwable throwable) {
+ if (scope != null) {
+ final Span span = scope.span();
+
+ if (result instanceof HttpResponse) {
+ Tags.HTTP_STATUS.set(span, ((HttpResponse) result).getStatusLine().getStatusCode());
+ } // else they probably provided a ResponseHandler.
+
+ if (throwable != null) {
+ Tags.ERROR.set(span, Boolean.TRUE);
+ span.log(Collections.singletonMap(ERROR_OBJECT, throwable));
+ span.finish();
+ }
+ scope.close();
+ CallDepthThreadLocalMap.reset(HttpClient.class);
+ }
+ }
+ }
+
+ public static class WrappingStatusSettingResponseHandler implements ResponseHandler {
+ final Span span;
+ final ResponseHandler handler;
+
+ public WrappingStatusSettingResponseHandler(final Span span, final ResponseHandler handler) {
+ this.span = span;
+ this.handler = handler;
+ }
+
+ @Override
+ public Object handleResponse(final HttpResponse response)
+ throws ClientProtocolException, IOException {
+ if (null != span) {
+ Tags.HTTP_STATUS.set(span, response.getStatusLine().getStatusCode());
+ }
+ return handler.handleResponse(response);
+ }
+ }
+
+ public static class HttpHeadersInjectAdapter implements TextMap {
+
+ private final HttpRequest httpRequest;
+
+ public HttpHeadersInjectAdapter(final HttpRequest httpRequest) {
+ this.httpRequest = httpRequest;
+ }
+
+ @Override
+ public void put(final String key, final String value) {
+ httpRequest.addHeader(key, value);
+ }
+
+ @Override
+ public Iterator<Map.Entry<String, String>> iterator() {
+ throw new UnsupportedOperationException(
+ "This class should be used only with tracer#inject()");
+ }
+ }
+}

ADD
dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy
+import datadog.opentracing.DDSpan
+import datadog.trace.agent.test.AgentTestRunner
+import datadog.trace.agent.test.asserts.TraceAssert
+import datadog.trace.api.DDSpanTypes
+import datadog.trace.api.DDTags
+import io.opentracing.tag.Tags
+import org.apache.http.HttpResponse
+import org.apache.http.client.HttpClient
+import org.apache.http.client.methods.HttpGet
+import org.apache.http.impl.client.BasicResponseHandler
+import org.apache.http.impl.client.DefaultHttpClient
+import org.apache.http.message.BasicHeader
+import spock.lang.AutoCleanup
+import spock.lang.Shared
+
+import static datadog.trace.agent.test.TestUtils.runUnderTrace
+import static datadog.trace.agent.test.asserts.ListWriterAssert.assertTraces
+import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer
+
+class ApacheHttpClientTest extends AgentTestRunner {
+
+ @AutoCleanup
+ @Shared
+ def server = httpServer {
+ handlers {
+ prefix("success") {
+ handleDistributedRequest()
+ String msg = "Hello."
+ response.status(200).send(msg)
+ }
+ prefix("redirect") {
+ handleDistributedRequest()
+ redirect(server.address.resolve("/success").toURL().toString())
+ }
+ prefix("another-redirect") {
+ handleDistributedRequest()
+ redirect(server.address.resolve("/redirect").toURL().toString())
+ }
+ }
+ }
+ @Shared
+ int port = server.address.port
+ @Shared
+ def successUrl = server.address.resolve("/success")
+ @Shared
+ def redirectUrl = server.address.resolve("/redirect")
+ @Shared
+ def twoRedirectsUrl = server.address.resolve("/another-redirect")
+ @Shared
+ def handler = new BasicResponseHandler()
+
+ final HttpClient client = new DefaultHttpClient()
+
+ def "trace request with propagation"() {
+ when:
+ String response = runUnderTrace("parent") {
+ if (responseHandler) {
+ client.execute(new HttpGet(successUrl), responseHandler)
+ } else {
+ client.execute(new HttpGet(successUrl)).entity.content.text
+ }
+ }
+
+ then:
+ response == "Hello."
+ // one trace on the server, one trace on the client
+ assertTraces(TEST_WRITER, 2) {
+ server.distributedRequestTrace(it, 0, TEST_WRITER[1][1])
+ trace(1, 2) {
+ parentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
+ }
+ }
+
+ where:
+ responseHandler << [null, handler]
+ }
+
+ def "trace request without propagation"() {
+ setup:
+ HttpGet request = new HttpGet(successUrl)
+ request.addHeader(new BasicHeader("is-dd-server", "false"))
+
+ when:
+ HttpResponse response = runUnderTrace("parent") {
+ client.execute(request)
+ }
+
+ then:
+ response.getStatusLine().getStatusCode() == 200
+ // only one trace (client).
+ assertTraces(TEST_WRITER, 1) {
+ trace(0, 2) {
+ parentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
+ }
+ }
+ }
+
+ def parentSpan(TraceAssert trace, int index, Throwable exception = null) {
+ trace.span(index) {
+ parent()
+ serviceName "unnamed-java-app"
+ operationName "parent"
+ resourceName "parent"
+ errored exception != null
+ tags {
+ defaultTags()
+ if (exception) {
+ errorTags(exception.class)
+ }
+ }
+ }
+ }
+
+ def successClientSpan(TraceAssert trace, int index, DDSpan parent, status = 200, route = "success", Throwable exception = null) {
+ trace.span(index) {
+ childOf parent
+ serviceName "unnamed-java-app"
+ operationName "apache.http.request"
+ resourceName "GET /$route"
+ errored exception != null
+ tags {
+ defaultTags()
+ if (exception) {
+ errorTags(exception.class)
+ }
+ "$Tags.COMPONENT.key" "apache-httpclient"
+ "$Tags.HTTP_STATUS.key" status
+ "$Tags.HTTP_URL.key" "http://localhost:$port/$route"
+ "$Tags.PEER_HOSTNAME.key" "localhost"
+ "$Tags.PEER_PORT.key" server.getAddress().port
+ "$Tags.HTTP_METHOD.key" "GET"
+ "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
+ "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
+ }
+ }
+ }
+}

MODIFY
dd-java-agent/instrumentation/aws-java-sdk-1.11.0/aws-java-sdk-1.11.0.gradle
dd-java-agent/instrumentation/aws-java-sdk-1.11.0/aws-java-sdk-1.11.0.gradle
@@ -58,7 +58,7 @@ dependencies {
testCompile project(':dd-java-agent:testing')
// Include httpclient instrumentation for testing because it is a dependency for aws-sdk.
- testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4.3')
+ testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4')
testCompile group: 'com.amazonaws', name: 'aws-java-sdk', version: '1.11.0'
}

MODIFY
dd-java-agent/instrumentation/aws-java-sdk-1.11.106/aws-java-sdk-1.11.106.gradle
dd-java-agent/instrumentation/aws-java-sdk-1.11.106/aws-java-sdk-1.11.106.gradle
@@ -31,7 +31,7 @@ dependencies {
testCompile project(':dd-java-agent:testing')
// Include httpclient instrumentation for testing because it is a dependency for aws-sdk.
- testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4.3')
+ testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4')
testCompile group: 'com.amazonaws', name: 'aws-java-sdk', version: '1.11.106'
}

MODIFY
dd-java-agent/instrumentation/elasticsearch-rest-5/elasticsearch-rest-5.gradle
dd-java-agent/instrumentation/elasticsearch-rest-5/elasticsearch-rest-5.gradle
@@ -34,7 +34,7 @@ dependencies {
// Include httpclient instrumentation for testing because it is a dependency for elasticsearch-rest-client.
// It doesn't actually work though. They use HttpAsyncClient, which isn't currently instrumented.
// TODO: add HttpAsyncClient instrumentation when that is complete.
- testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4.3')
+ testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4')
// TODO: add netty instrumentation when that is complete.
testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0'

MODIFY
dd-java-agent/instrumentation/elasticsearch-transport-5/elasticsearch-transport-5.gradle
dd-java-agent/instrumentation/elasticsearch-transport-5/elasticsearch-transport-5.gradle
@@ -41,7 +41,7 @@ dependencies {
// Include httpclient instrumentation for testing because it is a dependency for elasticsearch-rest-client.
// It doesn't actually work though. They use HttpAsyncClient, which isn't currently instrumented.
// TODO: add HttpAsyncClient instrumentation when that is complete.
- testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4.3')
+ testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4')
// TODO: add netty instrumentation when that is complete.
testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0'

MODIFY
dd-java-agent/instrumentation/elasticsearch-transport-6/elasticsearch-transport-6.gradle
dd-java-agent/instrumentation/elasticsearch-transport-6/elasticsearch-transport-6.gradle
@@ -41,7 +41,7 @@ dependencies {
// Include httpclient instrumentation for testing because it is a dependency for elasticsearch-rest-client.
// It doesn't actually work though. They use HttpAsyncClient, which isn't currently instrumented.
// TODO: add HttpAsyncClient instrumentation when that is complete.
- testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4.3')
+ testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4')
// TODO: add netty instrumentation when that is complete.
testCompile group: 'org.elasticsearch.plugin', name: 'transport-netty4-client', version: '6.0.0'

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java
dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java
@@ -17,7 +17,6 @@ public class OperationDecorator extends AbstractDecorator {
static {
final Map<String, String> mappings = new HashMap<>();
// Component name <> Operation name
- mappings.put("apache-httpclient", "apache.http");
mappings.put("java-aws-sdk", "aws.http");
// FIXME: JMS ops card is low (jms-send or jms-receive), may be this mapping is useless
mappings.put("java-jms", "jms");
@@ -28,7 +27,7 @@ public class OperationDecorator extends AbstractDecorator {
public OperationDecorator() {
super();
- this.setMatchingTag(Tags.COMPONENT.getKey());
+ setMatchingTag(Tags.COMPONENT.getKey());
}
@Override

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java
dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java
@@ -17,8 +17,8 @@ public class URLAsResourceName extends AbstractDecorator {
public URLAsResourceName() {
super();
- this.setMatchingTag(Tags.HTTP_URL.getKey());
- this.setReplacementTag(DDTags.RESOURCE_NAME);
+ setMatchingTag(Tags.HTTP_URL.getKey());
+ setReplacementTag(DDTags.RESOURCE_NAME);
}
@Override
@@ -59,6 +59,10 @@ public class URLAsResourceName extends AbstractDecorator {
norm = QUERYSTRING.matcher(norm).replaceAll("");
norm = PATH_MIXED_ALPHANUMERICS.matcher(norm).replaceAll("?");
+ if (norm.trim().isEmpty()) {
+ norm = "/";
+ }
+
return norm;
}
}

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy
dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy
@@ -25,6 +25,9 @@ class URLAsResourceNameTest extends Specification {
where:
input | output
+ "" | "/"
+ " " | "/"
+ "\t" | "/"
"/" | "/"
"/?asdf" | "/"
"/search" | "/search"

MODIFY
settings.gradle
settings.gradle
@@ -10,7 +10,7 @@ include ':dd-trace-api'
// instrumentation:
include ':dd-java-agent:instrumentation:akka-http-10.0'
-include ':dd-java-agent:instrumentation:apache-httpclient-4.3'
+include ':dd-java-agent:instrumentation:apache-httpclient-4'
include ':dd-java-agent:instrumentation:aws-java-sdk-1.11.0'
include ':dd-java-agent:instrumentation:aws-java-sdk-1.11.106'
include ':dd-java-agent:instrumentation:couchbase-2.0'

Refactor Apache HttpClient Instrumentation.

Removes the need for DDTracingClientExec.
Reduces extra spans.
Does not include a span for each redirect.
Compatible with 4.0+ instead of just 4.3+.