Improve Apache HTTP client
Properly open and close outer span in multi-request cases
MODIFY
dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java
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.ClassLoaderMatcher.classLoaderHasClasses;
+import static io.opentracing.log.Fields.ERROR_OBJECT;
+import static net.bytebuddy.matcher.ElementMatchers.hasSuperType;
+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.client.DefaultRedirectStrategy;
import org.apache.http.impl.execchain.ClientExecChain;
@AutoService(Instrumenter.class)
@@ -23,8 +30,9 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
}
@Override
- public ElementMatcher<? super TypeDescription> typeMatcher() {
- return named("org.apache.http.impl.client.HttpClientBuilder");
+ public ElementMatcher typeMatcher() {
+ return named("org.apache.http.impl.client.HttpClientBuilder")
+ .or(hasSuperType(named("org.apache.http.impl.client.CloseableHttpClient")));
}
@Override
@@ -38,7 +46,9 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
"org.apache.http.client.methods.HttpRequestWrapper",
"org.apache.http.client.protocol.HttpClientContext",
"org.apache.http.conn.routing.HttpRoute",
- "org.apache.http.impl.execchain.ClientExecChain");
+ "org.apache.http.impl.execchain.ClientExecChain",
+ "org.apache.http.impl.client.CloseableHttpClient",
+ "org.apache.http.impl.client.InternalHttpClient");
}
@Override
@@ -51,19 +61,42 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
@Override
public Map<ElementMatcher, String> transformers() {
- Map<ElementMatcher, String> transformers = new HashMap<>();
+ final Map<ElementMatcher, String> transformers = new HashMap<>();
transformers.put(
- isMethod().and(named("decorateProtocolExec")), ApacheHttpClientAdvice.class.getName());
+ isMethod().and(not(isAbstract())).and(named("doExecute")), ClientAdvice.class.getName());
+ transformers.put(
+ isMethod().and(named("decorateProtocolExec")), ClientExecAdvice.class.getName());
return transformers;
}
- public static class ApacheHttpClientAdvice {
- /** Strategy: add our tracing exec to the apache exec chain. */
+ 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, DefaultRedirectStrategy.INSTANCE, false, GlobalTracer.get());
+ execChain = new DDTracingClientExec(execChain, GlobalTracer.get());
}
}
}

MODIFY
dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/DDTracingClientExec.java
dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/DDTracingClientExec.java
@@ -18,7 +18,6 @@ import java.util.Map;
import lombok.extern.slf4j.Slf4j;
import org.apache.http.HttpException;
import org.apache.http.HttpRequest;
-import org.apache.http.client.RedirectStrategy;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpExecutionAware;
import org.apache.http.client.methods.HttpRequestWrapper;
@@ -33,36 +32,15 @@ import org.apache.http.impl.execchain.ClientExecChain;
*/
@Slf4j
public class DDTracingClientExec implements ClientExecChain {
- private static final String COMPONENT_NAME = "apache-httpclient";
- private static final String OPERATION_NAME = "http.request";
- /**
- * Id of {@link HttpClientContext#setAttribute(String, Object)} representing span associated with
- * the current client processing. Referenced span is local span not a span representing HTTP
- * communication.
- */
- private static final String ACTIVE_SPAN = DDTracingClientExec.class.getName() + ".activeSpan";
- /**
- * Tracing {@link ClientExecChain} is executed after redirect exec, so on redirects it is called
- * multiple times. This is used as an id for {@link HttpClientContext#setAttribute(String,
- * Object)} to store number of redirects.
- */
- private static final String REDIRECT_COUNT =
- DDTracingClientExec.class.getName() + ".redirectCount";
+ static final String COMPONENT_NAME = "apache-httpclient";
+ static final String OPERATION_NAME = "http.request";
- private final RedirectStrategy redirectStrategy;
private final ClientExecChain requestExecutor;
- private final boolean redirectHandlingDisabled;
private final Tracer tracer;
- public DDTracingClientExec(
- final ClientExecChain clientExecChain,
- final RedirectStrategy redirectStrategy,
- final boolean redirectHandlingDisabled,
- final Tracer tracer) {
- this.requestExecutor = clientExecChain;
- this.redirectStrategy = redirectStrategy;
- this.redirectHandlingDisabled = redirectHandlingDisabled;
+ public DDTracingClientExec(final ClientExecChain clientExecChain, final Tracer tracer) {
+ requestExecutor = clientExecChain;
this.tracer = tracer;
}
@@ -73,91 +51,37 @@ public class DDTracingClientExec implements ClientExecChain {
final HttpClientContext clientContext,
final HttpExecutionAware execAware)
throws IOException, HttpException {
-
- Scope localScope = clientContext.getAttribute(ACTIVE_SPAN, Scope.class);
- CloseableHttpResponse response = null;
- try {
- if (localScope == null) {
- localScope = createLocalScope(request, clientContext);
- }
-
- return (response = createNetworkSpan(localScope, route, request, clientContext, execAware));
- } catch (final Exception e) {
- localScope.close();
- throw e;
- } finally {
- if (response != null) {
- /**
- * This exec runs after {@link org.apache.http.impl.execchain.RedirectExec} which loops
- * until there is no redirect or reaches max redirect count. {@link RedirectStrategy} is
- * used to decide whether localScope should be finished or not. If there is a redirect
- * localScope is not finished and redirect is logged.
- */
- Integer redirectCount = clientContext.getAttribute(REDIRECT_COUNT, Integer.class);
- if (!redirectHandlingDisabled
- && clientContext.getRequestConfig().isRedirectsEnabled()
- && redirectStrategy.isRedirected(request, response, clientContext)
- && ++redirectCount < clientContext.getRequestConfig().getMaxRedirects()) {
- clientContext.setAttribute(REDIRECT_COUNT, redirectCount);
- } else {
- localScope.close();
- }
- }
- }
- }
-
- private Scope createLocalScope(
- final HttpRequest httpRequest, final HttpClientContext clientContext) {
- final Tracer.SpanBuilder spanBuilder =
- tracer.buildSpan(OPERATION_NAME).withTag(Tags.COMPONENT.getKey(), COMPONENT_NAME);
-
- final Scope scope = spanBuilder.startActive(true);
- clientContext.setAttribute(ACTIVE_SPAN, scope);
- clientContext.setAttribute(REDIRECT_COUNT, 0);
- return scope;
- }
-
- private CloseableHttpResponse createNetworkSpan(
- final Scope parentScope,
- final HttpRoute route,
- final HttpRequestWrapper request,
- final HttpClientContext clientContext,
- final HttpExecutionAware execAware)
- throws IOException, HttpException {
- Scope networkScope = null;
- Span networkSpan = null;
+ 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 {
- networkScope =
+ scope =
tracer
.buildSpan(OPERATION_NAME)
.withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT)
.withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT)
- .asChildOf(parentScope.span())
.startActive(true);
- networkSpan = networkScope.span();
+ 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(
- networkSpan.context(),
- Format.Builtin.HTTP_HEADERS,
- new HttpHeadersInjectAdapter(request));
+ span.context(), Format.Builtin.HTTP_HEADERS, new HttpHeadersInjectAdapter(request));
}
// request tags
- Tags.HTTP_METHOD.set(networkSpan, request.getRequestLine().getMethod());
- Tags.HTTP_URL.set(networkSpan, request.getRequestLine().getUri());
+ 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(networkSpan, uri.getPort() == -1 ? 80 : uri.getPort());
- Tags.PEER_HOSTNAME.set(networkSpan, uri.getHost());
+ 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 network span", e);
+ log.debug("failed to create span", e);
}
final CloseableHttpResponse response =
@@ -165,23 +89,23 @@ public class DDTracingClientExec implements ClientExecChain {
try {
// response tags
- if (null != networkSpan) {
- Tags.HTTP_STATUS.set(networkSpan, response.getStatusLine().getStatusCode());
+ if (null != span) {
+ Tags.HTTP_STATUS.set(span, response.getStatusLine().getStatusCode());
}
} catch (final Exception e) {
- log.debug("failed to set network span status", e);
+ log.debug("failed to set span status", e);
}
return response;
- } catch (IOException | HttpException | RuntimeException e) {
+ } catch (final IOException | HttpException | RuntimeException e) {
// error tags
- Tags.ERROR.set(networkSpan, Boolean.TRUE);
- networkSpan.log(Collections.singletonMap(ERROR_OBJECT, e));
+ Tags.ERROR.set(span, Boolean.TRUE);
+ span.log(Collections.singletonMap(ERROR_OBJECT, e));
throw e;
} finally {
- if (null != networkScope) {
- networkScope.close();
+ if (null != scope) {
+ scope.close();
}
}
}

MODIFY
dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy
dd-java-agent/instrumentation/apache-httpclient-4.3/src/test/groovy/ApacheHttpClientTest.groovy
+import datadog.opentracing.DDSpan
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.agent.test.ListWriterAssert
import datadog.trace.agent.test.RatpackUtils
@@ -6,6 +7,7 @@ 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.ClientProtocolException
import org.apache.http.client.HttpClient
import org.apache.http.client.config.RequestConfig
import org.apache.http.client.methods.HttpGet
@@ -14,7 +16,6 @@ import org.apache.http.message.BasicHeader
import spock.lang.Shared
import static datadog.trace.agent.test.ListWriterAssert.assertTraces
-import static datadog.trace.agent.test.TestUtils.runUnderTrace
import static ratpack.groovy.test.embed.GroovyEmbeddedApp.ratpack
class ApacheHttpClientTest extends AgentTestRunner {
@@ -37,6 +38,13 @@ class ApacheHttpClientTest extends AgentTestRunner {
redirect(server.address.resolve("/success").toURL().toString())
}
}
+ prefix("another-redirect") {
+ get {
+ RatpackUtils.handleDistributedRequest(context)
+
+ redirect(server.address.resolve("/redirect").toURL().toString())
+ }
+ }
}
}
@Shared
@@ -45,25 +53,24 @@ class ApacheHttpClientTest extends AgentTestRunner {
def successUrl = server.address.resolve("/success")
@Shared
def redirectUrl = server.address.resolve("/redirect")
+ @Shared
+ def twoRedirectsUrl = server.address.resolve("/another-redirect")
final HttpClientBuilder builder = HttpClientBuilder.create()
final HttpClient client = builder.build()
def "trace request with propagation"() {
- setup:
- runUnderTrace("someTrace") {
+ when:
HttpResponse response = client.execute(new HttpGet(successUrl))
- assert response.getStatusLine().getStatusCode() == 200
- }
- expect:
+ then:
+ response.getStatusLine().getStatusCode() == 200
// one trace on the server, one trace on the client
assertTraces(TEST_WRITER, 2) {
- serverTrace(it, 0, TEST_WRITER[1][2])
- trace(1, 3) {
- outerSpan(it, 0)
- clientParentSpan(it, 1, span(0))
- successClientSpan(it, 2, span(1))
+ serverTrace(it, 0, TEST_WRITER[1][1])
+ trace(1, 2) {
+ clientParentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
}
}
}
@@ -72,23 +79,23 @@ class ApacheHttpClientTest extends AgentTestRunner {
setup:
final RequestConfig.Builder requestConfigBuilder = new RequestConfig.Builder()
requestConfigBuilder.setMaxRedirects(10)
- runUnderTrace("someTrace") {
+
HttpGet request = new HttpGet(redirectUrl)
request.setConfig(requestConfigBuilder.build())
+
+ when:
HttpResponse response = client.execute(request)
- assert response.getStatusLine().getStatusCode() == 200
- }
- expect:
+ then:
+ response.getStatusLine().getStatusCode() == 200
// two traces on the server, one trace on the client
assertTraces(TEST_WRITER, 3) {
- serverTrace(it, 0, TEST_WRITER[2][3])
- serverTrace(it, 1, TEST_WRITER[2][2])
- trace(2, 4) {
- outerSpan(it, 0)
- clientParentSpan(it, 1, span(0))
- successClientSpan(it, 2, span(1))
- redirectClientSpan(it, 3, span(1))
+ serverTrace(it, 0, TEST_WRITER[2][2])
+ serverTrace(it, 1, TEST_WRITER[2][1])
+ trace(2, 3) {
+ clientParentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
+ redirectClientSpan(it, 2, span(0))
}
}
}
@@ -97,49 +104,71 @@ class ApacheHttpClientTest extends AgentTestRunner {
setup:
final RequestConfig.Builder requestConfigBuilder = new RequestConfig.Builder()
requestConfigBuilder.setMaxRedirects(1)
- runUnderTrace("someTrace") {
HttpGet request = new HttpGet(redirectUrl)
request.setConfig(requestConfigBuilder.build())
+
+ when:
HttpResponse response = client.execute(request)
- assert response.getStatusLine().getStatusCode() == 200
+
+ then:
+ response.getStatusLine().getStatusCode() == 200
+ // two traces on the server, one trace on the client
+ assertTraces(TEST_WRITER, 3) {
+ serverTrace(it, 0, TEST_WRITER[2][2])
+ serverTrace(it, 1, TEST_WRITER[2][1])
+ trace(2, 3) {
+ clientParentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
+ redirectClientSpan(it, 2, span(0))
+ }
+ }
}
- expect:
- print(TEST_WRITER)
+ def "trace redirected request with propagation too many redirects"() {
+ setup:
+ final RequestConfig.Builder requestConfigBuilder = new RequestConfig.Builder()
+ requestConfigBuilder.setMaxRedirects(1)
+
+ HttpGet request = new HttpGet(twoRedirectsUrl)
+ request.setConfig(requestConfigBuilder.build())
+
+ when:
+ client.execute(request)
+
+ then:
+ def exception = thrown(ClientProtocolException)
// two traces on the server, one trace on the client
assertTraces(TEST_WRITER, 3) {
- serverTrace(it, 0, TEST_WRITER[2][3])
+ serverTrace(it, 0, TEST_WRITER[2][2])
serverTrace(it, 1, TEST_WRITER[2][1])
- trace(2, 4) {
- outerSpan(it, 0)
- // Note: this is kind of an weird order?
- successClientSpan(it, 1, span(2))
- clientParentSpan(it, 2, span(0))
- redirectClientSpan(it, 3, span(2))
+ trace(2, 3) {
+ clientParentSpan(it, 0, exception)
+ redirectClientSpan(it, 1, span(0))
+ redirectClientSpan(it, 2, span(0), "another-redirect")
}
}
}
def "trace request without propagation"() {
setup:
- runUnderTrace("someTrace") {
HttpGet request = new HttpGet(successUrl)
request.addHeader(new BasicHeader("is-dd-server", "false"))
+
+ when:
HttpResponse response = client.execute(request)
- assert response.getStatusLine().getStatusCode() == 200
- }
- expect:
+
+ then:
+ response.getStatusLine().getStatusCode() == 200
// only one trace (client).
assertTraces(TEST_WRITER, 1) {
- trace(0, 3) {
- outerSpan(it, 0)
- clientParentSpan(it, 1, span(0))
- successClientSpan(it, 2, span(1))
+ trace(0, 2) {
+ clientParentSpan(it, 0)
+ successClientSpan(it, 1, span(0))
}
}
}
- def serverTrace(ListWriterAssert writer, index, parent) {
+ def serverTrace(ListWriterAssert writer, int index, DDSpan parent) {
writer.trace(index, 1) {
span(0) {
childOf parent
@@ -154,34 +183,24 @@ class ApacheHttpClientTest extends AgentTestRunner {
}
}
- def outerSpan(TraceAssert trace, index) {
+ def clientParentSpan(TraceAssert trace, int index, Throwable exception = null) {
trace.span(index) {
parent()
serviceName "unnamed-java-app"
- operationName "someTrace"
- resourceName "someTrace"
- errored false
- tags {
- defaultTags()
- }
- }
- }
-
- def clientParentSpan(TraceAssert trace, index, parent) {
- trace.span(index) {
- childOf parent
- serviceName "unnamed-java-app"
operationName "apache.http"
resourceName "apache.http"
- errored false
+ errored exception != null
tags {
defaultTags()
"$Tags.COMPONENT.key" "apache-httpclient"
+ if (exception) {
+ errorTags(exception.class)
+ }
}
}
}
- def successClientSpan(TraceAssert trace, index, parent, status = 200, route = "success") {
+ def successClientSpan(TraceAssert trace, int index, DDSpan parent, status = 200, route = "success") {
trace.span(index) {
childOf parent
serviceName "unnamed-java-app"
@@ -201,7 +220,7 @@ class ApacheHttpClientTest extends AgentTestRunner {
}
}
- def redirectClientSpan(TraceAssert trace, index, parent) {
- successClientSpan(trace, index, parent, 302, "redirect")
+ def redirectClientSpan(TraceAssert trace, int index, DDSpan parent, route = "redirect") {
+ successClientSpan(trace, index, parent, 302, route)
}
}

Improve Apache HTTP client

Properly open and close outer span in multi-request cases