Add instrumentation for all of ApacheHttpClient methods
MODIFY
dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java
dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java
@@ -2,12 +2,13 @@ package datadog.trace.instrumentation.apachehttpclient;
import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType;
import static datadog.trace.instrumentation.apachehttpclient.ApacheHttpClientDecorator.DECORATE;
-import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
+import static net.bytebuddy.matcher.ElementMatchers.isInterface;
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 static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
@@ -19,6 +20,7 @@ import io.opentracing.propagation.Format;
import io.opentracing.propagation.TextMap;
import io.opentracing.util.GlobalTracer;
import java.io.IOException;
+import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import net.bytebuddy.asm.Advice;
@@ -26,6 +28,7 @@ import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.implementation.bytecode.assign.Assigner;
import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.http.HttpHost;
import org.apache.http.HttpRequest;
import org.apache.http.HttpResponse;
import org.apache.http.client.ClientProtocolException;
@@ -42,53 +45,112 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
@Override
public ElementMatcher<TypeDescription> typeMatcher() {
- return safeHasSuperType(named("org.apache.http.client.HttpClient"));
+ return not(isInterface()).and(safeHasSuperType(named("org.apache.http.client.HttpClient")));
}
@Override
public String[] helperClassNames() {
return new String[] {
+ getClass().getName() + "$HelperMethods",
getClass().getName() + "$HttpHeadersInjectAdapter",
getClass().getName() + "$WrappingStatusSettingResponseHandler",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.HttpClientDecorator",
packageName + ".ApacheHttpClientDecorator",
+ packageName + ".HostAndRequestAsHttpUriRequest",
};
}
@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
- return singletonMap(
+ final Map<ElementMatcher<? super MethodDescription>, String> transformers = new HashMap<>();
+ // There are 8 execute(...) methods. Depending on the version, they may or may not delegate to
+ // eachother. Thus, all methods need to be instrumented. Because of argument position and type,
+ // some methods can share the same advice class. The call depth tracking ensures only 1 span is
+ // created
+
+ transformers.put(
isMethod()
- .and(not(isAbstract()))
.and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(1))
.and(takesArgument(0, named("org.apache.http.client.methods.HttpUriRequest"))),
- ClientAdvice.class.getName());
- }
+ UriRequestAdvice.class.getName());
- public static class ClientAdvice {
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(2))
+ .and(takesArgument(0, named("org.apache.http.client.methods.HttpUriRequest")))
+ .and(takesArgument(1, named("org.apache.http.protocol.HttpContext"))),
+ UriRequestAdvice.class.getName());
- @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;
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(2))
+ .and(takesArgument(0, named("org.apache.http.client.methods.HttpUriRequest")))
+ .and(takesArgument(1, named("org.apache.http.client.ResponseHandler"))),
+ UriRequestWithHandlerAdvice.class.getName());
+
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(3))
+ .and(takesArgument(0, named("org.apache.http.client.methods.HttpUriRequest")))
+ .and(takesArgument(1, named("org.apache.http.client.ResponseHandler")))
+ .and(takesArgument(2, named("org.apache.http.protocol.HttpContext"))),
+ UriRequestWithHandlerAdvice.class.getName());
+
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(2))
+ .and(takesArgument(0, named("org.apache.http.HttpHost")))
+ .and(takesArgument(1, named("org.apache.http.HttpRequest"))),
+ RequestAdvice.class.getName());
+
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(3))
+ .and(takesArgument(0, named("org.apache.http.HttpHost")))
+ .and(takesArgument(1, named("org.apache.http.HttpRequest")))
+ .and(takesArgument(2, named("org.apache.http.protocol.HttpContext"))),
+ RequestAdvice.class.getName());
+
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(3))
+ .and(takesArgument(0, named("org.apache.http.HttpHost")))
+ .and(takesArgument(1, named("org.apache.http.HttpRequest")))
+ .and(takesArgument(2, named("org.apache.http.client.ResponseHandler"))),
+ RequestWithHandlerAdvice.class.getName());
+
+ transformers.put(
+ isMethod()
+ .and(named("execute"))
+ .and(not(isAbstract()))
+ .and(takesArguments(4))
+ .and(takesArgument(0, named("org.apache.http.HttpHost")))
+ .and(takesArgument(1, named("org.apache.http.HttpRequest")))
+ .and(takesArgument(2, named("org.apache.http.client.ResponseHandler")))
+ .and(takesArgument(3, named("org.apache.http.protocol.HttpContext"))),
+ RequestWithHandlerAdvice.class.getName());
+
+ return transformers;
}
+
+ public static class HelperMethods {
+ public static Scope doMethodEnter(final HttpUriRequest request) {
final Tracer tracer = GlobalTracer.get();
final Scope scope = tracer.buildSpan("http.request").startActive(true);
final Span span = scope.span();
@@ -96,13 +158,6 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
DECORATE.afterStart(span);
DECORATE.onRequest(span, request);
- // 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) {
@@ -112,11 +167,8 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
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) {
+ public static void doMethodExit(
+ final Scope scope, final Object result, final Throwable throwable) {
if (scope != null) {
try {
final Span span = scope.span();
@@ -135,6 +187,130 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default {
}
}
+ public static class UriRequestAdvice {
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static Scope methodEnter(@Advice.Argument(0) final HttpUriRequest request) {
+ final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class);
+ if (callDepth > 0) {
+ return null;
+ }
+
+ return HelperMethods.doMethodEnter(request);
+ }
+
+ @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) {
+
+ HelperMethods.doMethodExit(scope, result, throwable);
+ }
+ }
+
+ public static class UriRequestWithHandlerAdvice {
+
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static Scope methodEnter(
+ @Advice.Argument(0) final HttpUriRequest request,
+ @Advice.Argument(
+ value = 1,
+ optional = true,
+ typing = Assigner.Typing.DYNAMIC,
+ readOnly = false)
+ Object handler) {
+ final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class);
+ if (callDepth > 0) {
+ return null;
+ }
+
+ final Scope scope = HelperMethods.doMethodEnter(request);
+
+ // Wrap the handler so we capture the status code
+ if (handler instanceof ResponseHandler) {
+ handler = new WrappingStatusSettingResponseHandler(scope.span(), (ResponseHandler) handler);
+ }
+ 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) {
+
+ HelperMethods.doMethodExit(scope, result, throwable);
+ }
+ }
+
+ public static class RequestAdvice {
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static Scope methodEnter(
+ @Advice.Argument(0) final HttpHost host, @Advice.Argument(1) final HttpRequest request) {
+ final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class);
+ if (callDepth > 0) {
+ return null;
+ }
+
+ if (request instanceof HttpUriRequest) {
+ return HelperMethods.doMethodEnter((HttpUriRequest) request);
+ } else {
+ return HelperMethods.doMethodEnter(new HostAndRequestAsHttpUriRequest(host, request));
+ }
+ }
+
+ @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) {
+
+ HelperMethods.doMethodExit(scope, result, throwable);
+ }
+ }
+
+ public static class RequestWithHandlerAdvice {
+
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static Scope methodEnter(
+ @Advice.Argument(0) final HttpHost host,
+ @Advice.Argument(1) final HttpRequest request,
+ @Advice.Argument(
+ value = 2,
+ optional = true,
+ typing = Assigner.Typing.DYNAMIC,
+ readOnly = false)
+ Object handler) {
+ final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class);
+ if (callDepth > 0) {
+ return null;
+ }
+
+ final Scope scope;
+
+ if (request instanceof HttpUriRequest) {
+ scope = HelperMethods.doMethodEnter((HttpUriRequest) request);
+ } else {
+ scope = HelperMethods.doMethodEnter(new HostAndRequestAsHttpUriRequest(host, request));
+ }
+
+ // Wrap the handler so we capture the status code
+ if (handler instanceof ResponseHandler) {
+ handler = new WrappingStatusSettingResponseHandler(scope.span(), (ResponseHandler) handler);
+ }
+ 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) {
+
+ HelperMethods.doMethodExit(scope, result, throwable);
+ }
+ }
+
public static class WrappingStatusSettingResponseHandler implements ResponseHandler {
final Span span;
final ResponseHandler handler;

ADD
dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/HostAndRequestAsHttpUriRequest.java
+package datadog.trace.instrumentation.apachehttpclient;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import lombok.Getter;
+import org.apache.http.HttpHost;
+import org.apache.http.HttpRequest;
+import org.apache.http.ProtocolVersion;
+import org.apache.http.RequestLine;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.apache.http.message.AbstractHttpMessage;
+
+/** Wraps HttpHost and HttpRequest into a HttpUriRequest for decorators and injectors */
+@Getter
+public class HostAndRequestAsHttpUriRequest extends AbstractHttpMessage implements HttpUriRequest {
+
+ private final String method;
+ private final RequestLine requestLine;
+ private final ProtocolVersion protocolVersion;
+ private final java.net.URI URI;
+
+ private final HttpRequest actualRequest;
+
+ public HostAndRequestAsHttpUriRequest(final HttpHost httpHost, final HttpRequest httpRequest) {
+
+ method = httpRequest.getRequestLine().getMethod();
+ requestLine = httpRequest.getRequestLine();
+ protocolVersion = requestLine.getProtocolVersion();
+
+ URI calculatedURI;
+ try {
+ calculatedURI = new URI(httpHost.toURI() + httpRequest.getRequestLine().getUri());
+ } catch (final URISyntaxException e) {
+ calculatedURI = null;
+ }
+ URI = calculatedURI;
+ actualRequest = httpRequest;
+ }
+
+ @Override
+ public void abort() throws UnsupportedOperationException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean isAborted() {
+ return false;
+ }
+
+ @Override
+ public void addHeader(final String name, final String value) {
+ actualRequest.addHeader(name, value);
+ }
+}

MODIFY
dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy
dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy
@@ -35,12 +35,30 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
abstract T createRequest(String method, URI uri)
abstract HttpResponse executeRequest(T request, URI uri)
+
+ static String fullPathFromURI(URI uri) {
+ StringBuilder builder = new StringBuilder()
+ if (uri.getPath() != null) {
+ builder.append(uri.getPath())
+ }
+
+ if (uri.getQuery() != null) {
+ builder.append('?')
+ builder.append(uri.getQuery())
+ }
+
+ if (uri.getFragment() != null) {
+ builder.append('#')
+ builder.append(uri.getFragment())
+ }
+ return builder.toString()
+ }
}
class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
@Override
BasicHttpRequest createRequest(String method, URI uri) {
- return new BasicHttpRequest(method, uri.getPath())
+ return new BasicHttpRequest(method, fullPathFromURI(uri))
}
@Override
@@ -52,7 +70,7 @@ class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
@Override
BasicHttpRequest createRequest(String method, URI uri) {
- return new BasicHttpRequest(method, uri.getPath())
+ return new BasicHttpRequest(method, fullPathFromURI(uri))
}
@Override
@@ -64,7 +82,7 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpReque
class ApacheClientHostRequestResponseHandler extends ApacheHttpClientTest<BasicHttpRequest> {
@Override
BasicHttpRequest createRequest(String method, URI uri) {
- return new BasicHttpRequest(method, uri.getPath())
+ return new BasicHttpRequest(method, fullPathFromURI(uri))
}
@Override
@@ -76,7 +94,7 @@ class ApacheClientHostRequestResponseHandler extends ApacheHttpClientTest<BasicH
class ApacheClientHostRequestResponseHandlerContext extends ApacheHttpClientTest<BasicHttpRequest> {
@Override
BasicHttpRequest createRequest(String method, URI uri) {
- return new BasicHttpRequest(method, uri.getPath())
+ return new BasicHttpRequest(method, fullPathFromURI(uri))
}
@Override

Add instrumentation for all of ApacheHttpClient methods