diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java b/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java index 398cf7a2aa..40bcf9e584 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java +++ b/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 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, String> transformers() { - return singletonMap( + final Map, 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()); + + 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()); + + 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 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; - } + 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; diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/HostAndRequestAsHttpUriRequest.java b/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/HostAndRequestAsHttpUriRequest.java new file mode 100644 index 0000000000..6fba492971 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpclient-4/src/main/java/datadog/trace/instrumentation/apachehttpclient/HostAndRequestAsHttpUriRequest.java @@ -0,0 +1,54 @@ +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); + } +} diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy b/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy index afcf3cb4dc..664c72a192 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy +++ b/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientTest.groovy @@ -35,12 +35,30 @@ abstract class ApacheHttpClientTest 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 { @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 { class ApacheClientHostRequestContext extends ApacheHttpClientTest { @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 { @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 { @Override BasicHttpRequest createRequest(String method, URI uri) { - return new BasicHttpRequest(method, uri.getPath()) + return new BasicHttpRequest(method, fullPathFromURI(uri)) } @Override