From 9ad06a679125a528072cf5eeadd474e1423327b8 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 22 Apr 2019 17:28:56 -0700 Subject: [PATCH 1/3] Add instrumentation for Apache HttpAsyncClient Extract http client tests to shared class. --- .../apache-httpasyncclient-4.gradle | 38 +++ .../ApacheHttpAsyncClientDecorator.java | 73 ++++ .../ApacheHttpAsyncClientInstrumentation.java | 255 ++++++++++++++ ...acheHttpClientRedirectInstrumentation.java | 65 ++++ .../ApacheHttpAsyncClientCallbackTest.groovy | 76 +++++ .../groovy/ApacheHttpAsyncClientTest.groovy | 59 ++++ .../apache-httpclient-4.gradle | 7 +- .../groovy/ApacheHttpClientTest.groovy | 243 -------------- ...ApacheHttpClientResponseHandlerTest.groovy | 43 +++ .../test/groovy/ApacheHttpClientTest.groovy | 153 +-------- .../agent/test/base/HttpClientTest.groovy | 312 ++++++++++++++++++ dd-java-agent/testing/testing.gradle | 1 + settings.gradle | 1 + 13 files changed, 945 insertions(+), 381 deletions(-) create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/apache-httpasyncclient-4.gradle create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentation.java create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientCallbackTest.groovy create mode 100644 dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientTest.groovy delete mode 100644 dd-java-agent/instrumentation/apache-httpclient-4/src/latestDepTest/groovy/ApacheHttpClientTest.groovy create mode 100644 dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientResponseHandlerTest.groovy create mode 100644 dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpClientTest.groovy diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/apache-httpasyncclient-4.gradle b/dd-java-agent/instrumentation/apache-httpasyncclient-4/apache-httpasyncclient-4.gradle new file mode 100644 index 0000000000..efa75ff2d5 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/apache-httpasyncclient-4.gradle @@ -0,0 +1,38 @@ +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +muzzle { + pass { + group = "org.apache.httpcomponents" + module = "httpasyncclient" + versions = "[4.0,)" + assertInverse = true + } +} + +apply from: "${rootDir}/gradle/java.gradle" +apply plugin: 'org.unbroken-dome.test-sets' + +testSets { + latestDepTest { + dirName = 'test' + } +} + +dependencies { + compileOnly group: 'org.apache.httpcomponents', name: 'httpasyncclient', version: '4.0' + + + compile project(':dd-java-agent:agent-tooling') + + compile deps.bytebuddy + compile deps.opentracing + annotationProcessor deps.autoservice + implementation deps.autoservice + + testCompile project(':dd-java-agent:testing') + testCompile group: 'org.apache.httpcomponents', name: 'httpasyncclient', version: '4.0' + + latestDepTestCompile group: 'org.apache.httpcomponents', name: 'httpasyncclient', version: '+' +} diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java new file mode 100644 index 0000000000..c8e4a9d312 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java @@ -0,0 +1,73 @@ +package datadog.trace.instrumentation.apachehttpasyncclient; + +import datadog.trace.agent.decorator.HttpClientDecorator; +import java.net.URI; +import org.apache.http.HttpRequest; +import org.apache.http.HttpResponse; +import org.apache.http.RequestLine; +import org.apache.http.StatusLine; +import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpCoreContext; + +public class ApacheHttpAsyncClientDecorator extends HttpClientDecorator { + public static final ApacheHttpAsyncClientDecorator DECORATE = + new ApacheHttpAsyncClientDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[] {"httpasyncclient", "apache-httpasyncclient"}; + } + + @Override + protected String component() { + return "apache-httpasyncclient"; + } + + @Override + protected String method(final HttpRequest request) { + final RequestLine requestLine = request.getRequestLine(); + return requestLine == null ? null : requestLine.getMethod(); + } + + @Override + protected String url(final HttpRequest request) { + final RequestLine requestLine = request.getRequestLine(); + return requestLine == null ? null : requestLine.getUri(); + } + + @Override + protected String hostname(final HttpRequest request) { + final RequestLine requestLine = request.getRequestLine(); + if (requestLine != null) { + final URI uri = URI.create(requestLine.getUri()); + if (uri != null) { + return uri.getHost(); + } + } + return null; + } + + @Override + protected Integer port(final HttpRequest request) { + final RequestLine requestLine = request.getRequestLine(); + if (requestLine != null) { + final URI uri = URI.create(requestLine.getUri()); + if (uri != null) { + return uri.getPort(); + } + } + return null; + } + + @Override + protected Integer status(final HttpContext context) { + final Object responseObject = context.getAttribute(HttpCoreContext.HTTP_RESPONSE); + if (responseObject instanceof HttpResponse) { + final StatusLine statusLine = ((HttpResponse) responseObject).getStatusLine(); + if (statusLine != null) { + return statusLine.getStatusCode(); + } + } + return null; + } +} diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java new file mode 100644 index 0000000000..63b8ea5c65 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java @@ -0,0 +1,255 @@ +package datadog.trace.instrumentation.apachehttpasyncclient; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator.DECORATE; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +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; +import datadog.trace.context.TraceScope; +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.util.GlobalTracer; +import java.io.IOException; +import java.util.Iterator; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.apache.http.HttpException; +import org.apache.http.HttpHost; +import org.apache.http.HttpRequest; +import org.apache.http.concurrent.FutureCallback; +import org.apache.http.nio.ContentEncoder; +import org.apache.http.nio.IOControl; +import org.apache.http.nio.protocol.HttpAsyncRequestProducer; +import org.apache.http.protocol.HttpContext; + +@AutoService(Instrumenter.class) +public class ApacheHttpAsyncClientInstrumentation extends Instrumenter.Default { + + public ApacheHttpAsyncClientInstrumentation() { + super("httpclient", "apache-httpclient", "apache-http-client"); + } + + @Override + public ElementMatcher typeMatcher() { + return safeHasSuperType(named("org.apache.http.nio.client.HttpAsyncClient")); + } + + @Override + public String[] helperClassNames() { + return new String[] { + getClass().getName() + "$HttpHeadersInjectAdapter", + getClass().getName() + "$DelegatingRequestProducer", + getClass().getName() + "$TraceContinuedFutureCallback", + "datadog.trace.agent.decorator.BaseDecorator", + "datadog.trace.agent.decorator.ClientDecorator", + "datadog.trace.agent.decorator.HttpClientDecorator", + packageName + ".ApacheHttpAsyncClientDecorator", + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(named("execute")) + .and(takesArguments(4)) + .and(takesArgument(0, named("org.apache.http.nio.protocol.HttpAsyncRequestProducer"))) + .and(takesArgument(1, named("org.apache.http.nio.protocol.HttpAsyncResponseConsumer"))) + .and(takesArgument(2, named("org.apache.http.protocol.HttpContext"))) + .and(takesArgument(3, named("org.apache.http.concurrent.FutureCallback"))), + ClientAdvice.class.getName()); + } + + public static class ClientAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static Span methodEnter( + @Advice.Argument(value = 0, readOnly = false) HttpAsyncRequestProducer requestProducer, + @Advice.Argument(2) final HttpContext context, + @Advice.Argument(value = 3, readOnly = false) FutureCallback futureCallback) { + + final Tracer tracer = GlobalTracer.get(); + final Scope parentScope = tracer.scopeManager().active(); + final Span clientSpan = tracer.buildSpan("http.request").start(); + + DECORATE.afterStart(clientSpan); + requestProducer = new DelegatingRequestProducer(clientSpan, requestProducer); + futureCallback = + new TraceContinuedFutureCallback(parentScope, clientSpan, context, futureCallback); + + return clientSpan; + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void methodExit( + @Advice.Enter final Span span, + @Advice.Return final Object result, + @Advice.Thrown final Throwable throwable) { + if (throwable != null) { + DECORATE.onError(span, throwable); + DECORATE.beforeFinish(span); + span.finish(); + } + } + } + + public static class DelegatingRequestProducer implements HttpAsyncRequestProducer { + final Span span; + final HttpAsyncRequestProducer delegate; + + public DelegatingRequestProducer(final Span span, final HttpAsyncRequestProducer delegate) { + this.span = span; + this.delegate = delegate; + } + + @Override + public HttpHost getTarget() { + return delegate.getTarget(); + } + + @Override + public HttpRequest generateRequest() throws IOException, HttpException { + final HttpRequest request = delegate.generateRequest(); + DECORATE.onRequest(span, request); + + GlobalTracer.get() + .inject( + span.context(), Format.Builtin.HTTP_HEADERS, new HttpHeadersInjectAdapter(request)); + + return request; + } + + @Override + public void produceContent(final ContentEncoder encoder, final IOControl ioctrl) + throws IOException { + delegate.produceContent(encoder, ioctrl); + } + + @Override + public void requestCompleted(final HttpContext context) { + delegate.requestCompleted(context); + } + + @Override + public void failed(final Exception ex) { + delegate.failed(ex); + } + + @Override + public boolean isRepeatable() { + return delegate.isRepeatable(); + } + + @Override + public void resetRequest() throws IOException { + delegate.resetRequest(); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + } + + public static class TraceContinuedFutureCallback implements FutureCallback { + private final TraceScope.Continuation parentContinuation; + private final Span clientSpan; + private final HttpContext context; + private final FutureCallback delegate; + + public TraceContinuedFutureCallback( + final Scope parentScope, + final Span clientSpan, + final HttpContext context, + final FutureCallback delegate) { + if (parentScope instanceof TraceScope) { + parentContinuation = ((TraceScope) parentScope).capture(); + } else { + parentContinuation = null; + } + this.clientSpan = clientSpan; + this.context = context; + this.delegate = delegate; + } + + @Override + public void completed(final T result) { + DECORATE.onResponse(clientSpan, context); + DECORATE.beforeFinish(clientSpan); + clientSpan.finish(); // Finish span before calling delegate + + if (parentContinuation == null) { + delegate.completed(result); + } else { + try (final TraceScope scope = parentContinuation.activate()) { + scope.setAsyncPropagation(true); + delegate.completed(result); + } + } + } + + @Override + public void failed(final Exception ex) { + DECORATE.onResponse(clientSpan, context); + DECORATE.onError(clientSpan, ex); + DECORATE.beforeFinish(clientSpan); + clientSpan.finish(); // Finish span before calling delegate + + if (parentContinuation == null) { + delegate.failed(ex); + } else { + try (final TraceScope scope = parentContinuation.activate()) { + scope.setAsyncPropagation(true); + delegate.failed(ex); + } + } + } + + @Override + public void cancelled() { + DECORATE.onResponse(clientSpan, context); + DECORATE.beforeFinish(clientSpan); + clientSpan.finish(); // Finish span before calling delegate + + if (parentContinuation == null) { + delegate.cancelled(); + } else { + try (final TraceScope scope = parentContinuation.activate()) { + scope.setAsyncPropagation(true); + delegate.cancelled(); + } + } + } + } + + 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> iterator() { + throw new UnsupportedOperationException( + "This class should be used only with tracer#inject()"); + } + } +} diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentation.java b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentation.java new file mode 100644 index 0000000000..93438a092b --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentation.java @@ -0,0 +1,65 @@ +package datadog.trace.instrumentation.apachehttpasyncclient; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.asm.Advice; +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.Header; +import org.apache.http.HttpRequest; + +/** + * Early versions don't copy headers over on redirect. This instrumentation copies our headers over + * manually. Inspired by + * https://github.com/elastic/apm-agent-java/blob/master/apm-agent-plugins/apm-apache-httpclient-plugin/src/main/java/co/elastic/apm/agent/httpclient/ApacheHttpAsyncClientRedirectInstrumentation.java + */ +@AutoService(Instrumenter.class) +public class ApacheHttpClientRedirectInstrumentation extends Instrumenter.Default { + + public ApacheHttpClientRedirectInstrumentation() { + super("httpasyncclient", "apache-httpasyncclient"); + } + + @Override + public ElementMatcher typeMatcher() { + return safeHasSuperType(named("org.apache.http.client.RedirectStrategy")); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(named("getRedirect")) + .and(takesArgument(0, named("org.apache.http.HttpRequest"))), + ClientRedirectAdvice.class.getName()); + } + + public static class ClientRedirectAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + private static void onAfterExecute( + @Advice.Argument(value = 0) final HttpRequest original, + @Advice.Return(typing = Assigner.Typing.DYNAMIC) final HttpRequest redirect) { + if (redirect == null) { + return; + } + + for (final Header header : original.getAllHeaders()) { + final String name = header.getName().toLowerCase(); + if (name.startsWith("x-datadog-") || name.startsWith("x-b3-")) { + if (!redirect.containsHeader(header.getName())) { + redirect.setHeader(header.getName(), header.getValue()); + } + } + } + } + } +} diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientCallbackTest.groovy b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientCallbackTest.groovy new file mode 100644 index 0000000000..ff5adcd29e --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientCallbackTest.groovy @@ -0,0 +1,76 @@ +import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator +import io.opentracing.util.GlobalTracer +import org.apache.http.HttpResponse +import org.apache.http.client.methods.HttpGet +import org.apache.http.concurrent.FutureCallback +import org.apache.http.impl.nio.client.HttpAsyncClients +import org.apache.http.message.BasicHeader +import spock.lang.AutoCleanup +import spock.lang.Shared + +import java.util.concurrent.CompletableFuture + +class ApacheHttpAsyncClientCallbackTest extends HttpClientTest { + + @AutoCleanup + @Shared + def client = HttpAsyncClients.createDefault() + + def setupSpec() { + client.start() + } + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + assert method == "GET" + + def hasParent = GlobalTracer.get().activeSpan() != null + + HttpGet request = new HttpGet(uri) + headers.entrySet().each { + request.addHeader(new BasicHeader(it.key, it.value)) + } + + def responseFuture = new CompletableFuture<>() + + client.execute(request, new FutureCallback() { + + @Override + void completed(HttpResponse result) { + if (hasParent && GlobalTracer.get().activeSpan() == null) { + responseFuture.completeExceptionally(new Exception("Missing span in scope")) + } else { + responseFuture.complete(result.statusLine.statusCode) + } + callback?.call() + } + + @Override + void failed(Exception ex) { + if (hasParent && GlobalTracer.get().activeSpan() == null) { + responseFuture.completeExceptionally(new Exception("Missing span in scope")) + } else { + responseFuture.completeExceptionally(ex) + } + } + + @Override + void cancelled() { + responseFuture.cancel(true) + } + }) + + return responseFuture.get() + } + + @Override + ApacheHttpAsyncClientDecorator decorator() { + return ApacheHttpAsyncClientDecorator.DECORATE + } + + @Override + Integer statusOnRedirectError() { + return 302 + } +} diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientTest.groovy b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientTest.groovy new file mode 100644 index 0000000000..1254e86316 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientTest.groovy @@ -0,0 +1,59 @@ +import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator +import org.apache.http.HttpResponse +import org.apache.http.client.methods.HttpGet +import org.apache.http.concurrent.FutureCallback +import org.apache.http.impl.nio.client.HttpAsyncClients +import org.apache.http.message.BasicHeader +import spock.lang.AutoCleanup +import spock.lang.Shared + +class ApacheHttpAsyncClientTest extends HttpClientTest { + + @AutoCleanup + @Shared + def client = HttpAsyncClients.createDefault() + + def setupSpec() { + client.start() + } + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + assert method == "GET" + HttpGet request = new HttpGet(uri) + headers.entrySet().each { + request.addHeader(new BasicHeader(it.key, it.value)) + } + + def handler = callback == null ? null : new FutureCallback() { + + @Override + void completed(HttpResponse result) { + callback() + } + + @Override + void failed(Exception ex) { + } + + @Override + void cancelled() { + } + } + + def response = client.execute(request, handler).get() + response.entity.getContent().close() // Make sure the connection is closed. + response.statusLine.statusCode + } + + @Override + ApacheHttpAsyncClientDecorator decorator() { + return ApacheHttpAsyncClientDecorator.DECORATE + } + + @Override + Integer statusOnRedirectError() { + return 302 + } +} diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/apache-httpclient-4.gradle b/dd-java-agent/instrumentation/apache-httpclient-4/apache-httpclient-4.gradle index 52f98d361e..678ed2afaa 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4/apache-httpclient-4.gradle +++ b/dd-java-agent/instrumentation/apache-httpclient-4/apache-httpclient-4.gradle @@ -1,5 +1,3 @@ -apply from: "${rootDir}/gradle/java.gradle" - muzzle { fail { group = "commons-httpclient" @@ -21,10 +19,13 @@ muzzle { } } +apply from: "${rootDir}/gradle/java.gradle" apply plugin: 'org.unbroken-dome.test-sets' testSets { - latestDepTest + latestDepTest { + dirName = 'test' + } } dependencies { diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/src/latestDepTest/groovy/ApacheHttpClientTest.groovy b/dd-java-agent/instrumentation/apache-httpclient-4/src/latestDepTest/groovy/ApacheHttpClientTest.groovy deleted file mode 100644 index 950d4898ca..0000000000 --- a/dd-java-agent/instrumentation/apache-httpclient-4/src/latestDepTest/groovy/ApacheHttpClientTest.groovy +++ /dev/null @@ -1,243 +0,0 @@ -import datadog.opentracing.DDSpan -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.agent.test.asserts.TraceAssert -import datadog.trace.api.Config -import datadog.trace.api.DDSpanTypes -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 -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.server.http.TestHttpServer.httpServer -import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace -import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride - -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 HttpClientBuilder builder = HttpClientBuilder.create() - final HttpClient client = builder.build() - - def "trace request with propagation"() { - when: - - String response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - 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(2) { - server.distributedRequestTrace(it, 0, TEST_WRITER[1][1]) - trace(1, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService) - } - } - - where: - responseHandler << [null, handler] - renameService << [false, true] - } - - def "trace redirected request with propagation many redirects allowed"() { - setup: - final RequestConfig.Builder requestConfigBuilder = new RequestConfig.Builder() - requestConfigBuilder.setMaxRedirects(10) - - HttpGet request = new HttpGet(redirectUrl) - request.setConfig(requestConfigBuilder.build()) - - when: - HttpResponse response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - client.execute(request) - } - } - - then: - response.getStatusLine().getStatusCode() == 200 - // two traces on the server, one trace on the client - assertTraces(3) { - server.distributedRequestTrace(it, 0, TEST_WRITER[2][1]) - server.distributedRequestTrace(it, 1, TEST_WRITER[2][1]) - trace(2, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService, 200, "redirect") - } - } - - where: - renameService << [false, true] - } - - def "trace redirected request with propagation 1 redirect allowed"() { - setup: - final RequestConfig.Builder requestConfigBuilder = new RequestConfig.Builder() - requestConfigBuilder.setMaxRedirects(1) - HttpGet request = new HttpGet(redirectUrl) - request.setConfig(requestConfigBuilder.build()) - - when: - HttpResponse response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - client.execute(request) - } - } - - then: - response.getStatusLine().getStatusCode() == 200 - // two traces on the server, one trace on the client - assertTraces(3) { - server.distributedRequestTrace(it, 0, TEST_WRITER[2][1]) - server.distributedRequestTrace(it, 1, TEST_WRITER[2][1]) - trace(2, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService, 200, "redirect") - } - } - - where: - renameService << [false, true] - } - - 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: - withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - client.execute(request) - } - } - - then: - def exception = thrown(ClientProtocolException) - // two traces on the server, one trace on the client - assertTraces(3) { - server.distributedRequestTrace(it, 0, TEST_WRITER[2][1]) - server.distributedRequestTrace(it, 1, TEST_WRITER[2][1]) - trace(2, 2) { - parentSpan(it, 0, exception) - successClientSpan(it, 1, span(0), renameService, null, "another-redirect", exception) - } - } - - where: - renameService << [false, true] - } - - def "trace request without propagation"() { - setup: - HttpGet request = new HttpGet(successUrl) - request.addHeader(new BasicHeader("is-dd-server", "false")) - - when: - HttpResponse response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - client.execute(request) - } - } - - then: - response.getStatusLine().getStatusCode() == 200 - // only one trace (client). - assertTraces(1) { - trace(0, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService) - } - } - - where: - renameService << [false, true] - } - - 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, boolean renameService, status = 200, route = "success", Throwable exception = null) { - trace.span(index) { - childOf parent - serviceName renameService ? "localhost" : "unnamed-java-app" - operationName "http.request" - resourceName "GET /$route" - spanType DDSpanTypes.HTTP_CLIENT - 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.address.port - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - } - } - } -} diff --git a/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientResponseHandlerTest.groovy b/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientResponseHandlerTest.groovy new file mode 100644 index 0000000000..146fc3fdb1 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientResponseHandlerTest.groovy @@ -0,0 +1,43 @@ +import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.apachehttpclient.ApacheHttpClientDecorator +import org.apache.http.HttpResponse +import org.apache.http.client.ResponseHandler +import org.apache.http.client.methods.HttpGet +import org.apache.http.impl.client.DefaultHttpClient +import org.apache.http.message.BasicHeader +import spock.lang.Shared + +class ApacheHttpClientResponseHandlerTest extends HttpClientTest { + + @Shared + def client = new DefaultHttpClient() + + @Shared + def handler = new ResponseHandler() { + @Override + Integer handleResponse(HttpResponse response) { + return response.statusLine.statusCode + } + } + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + assert method == "GET" + HttpGet request = new HttpGet(uri) + headers.entrySet().each { + request.addHeader(new BasicHeader(it.key, it.value)) + } + + def status = client.execute(request, handler) + + // handler execution is included within the client span, so we can't call the callback there. + callback?.call() + + return status + } + + @Override + ApacheHttpClientDecorator decorator() { + return ApacheHttpClientDecorator.DECORATE + } +} 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 4fe9cfecb5..1865b41ef8 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 @@ -1,148 +1,31 @@ -import datadog.opentracing.DDSpan -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.agent.test.asserts.TraceAssert -import datadog.trace.api.Config -import datadog.trace.api.DDSpanTypes -import io.opentracing.tag.Tags -import org.apache.http.HttpResponse -import org.apache.http.client.HttpClient +import datadog.trace.agent.test.base.HttpClientTest +import datadog.trace.instrumentation.apachehttpclient.ApacheHttpClientDecorator 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.server.http.TestHttpServer.httpServer -import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace -import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride +class ApacheHttpClientTest extends HttpClientTest { -class ApacheHttpClientTest extends AgentTestRunner { + @Shared + def client = new DefaultHttpClient() - @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 = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - if (responseHandler) { - client.execute(new HttpGet(successUrl), responseHandler) - } else { - client.execute(new HttpGet(successUrl)).entity.content.text - } - } + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + assert method == "GET" + HttpGet request = new HttpGet(uri) + headers.entrySet().each { + request.addHeader(new BasicHeader(it.key, it.value)) } - then: - response == "Hello." - // one trace on the server, one trace on the client - assertTraces(2) { - server.distributedRequestTrace(it, 0, TEST_WRITER[1][1]) - trace(1, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService) - } - } - - where: - responseHandler << [null, handler] - renameService << [false, true] + def response = client.execute(request) + callback?.call() + response.entity.getContent().close() // Make sure the connection is closed. + response.statusLine.statusCode } - def "trace request without propagation"() { - setup: - HttpGet request = new HttpGet(successUrl) - request.addHeader(new BasicHeader("is-dd-server", "false")) - - when: - HttpResponse response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { - runUnderTrace("parent") { - client.execute(request) - } - } - - then: - response.getStatusLine().getStatusCode() == 200 - // only one trace (client). - assertTraces(1) { - trace(0, 2) { - parentSpan(it, 0) - successClientSpan(it, 1, span(0), renameService) - } - } - - where: - renameService << [false, true] - } - - 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, boolean renameService, status = 200, route = "success", Throwable exception = null) { - trace.span(index) { - childOf parent - serviceName renameService ? "localhost" : "unnamed-java-app" - operationName "http.request" - resourceName "GET /$route" - spanType DDSpanTypes.HTTP_CLIENT - 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.address.port - "$Tags.HTTP_METHOD.key" "GET" - "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT - } - } + @Override + ApacheHttpClientDecorator decorator() { + return ApacheHttpClientDecorator.DECORATE } } diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpClientTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpClientTest.groovy new file mode 100644 index 0000000000..9d81071f79 --- /dev/null +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpClientTest.groovy @@ -0,0 +1,312 @@ +package datadog.trace.agent.test.base + +import datadog.opentracing.DDSpan +import datadog.trace.agent.decorator.HttpClientDecorator +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.api.Config +import datadog.trace.api.DDSpanTypes +import io.opentracing.tag.Tags +import spock.lang.AutoCleanup +import spock.lang.Shared + +import java.util.concurrent.ExecutionException + +import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer +import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace +import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride + +abstract class HttpClientTest extends AgentTestRunner { + + @AutoCleanup + @Shared + def server = httpServer { + handlers { + prefix("success") { + handleDistributedRequest() + String msg = "Hello." + response.status(200).send(msg) + } + prefix("error") { + handleDistributedRequest() + String msg = "Sorry." + response.status(500).send(msg) + } + prefix("redirect") { + handleDistributedRequest() + redirect(server.address.resolve("/success").toURL().toString()) + } + prefix("another-redirect") { + handleDistributedRequest() + redirect(server.address.resolve("/redirect").toURL().toString()) + } + prefix("circular-redirect") { + handleDistributedRequest() + redirect(server.address.resolve("/circular-redirect").toURL().toString()) + } + } + } + + @Shared + T decorator = decorator() + + /** + * Make the request and return the status code response + * @param method + * @return + */ + abstract int doRequest(String method, URI uri, Map headers = [:], Closure callback = null) + + abstract T decorator() + + Integer statusOnRedirectError() { + return null + } + + def "basic #method request"() { + when: + def status = doRequest(method, server.address.resolve("/success")) + + then: + status == 200 + assertTraces(2) { + server.distributedRequestTrace(it, 0, trace(1).get(0)) + trace(1, 1) { + clientSpan(it, 0, null, false) + } + } + + where: + method = "GET" + } + + def "basic #method request with parent"() { + when: + def status = runUnderTrace("parent") { + doRequest(method, server.address.resolve("/success")) + } + + then: + status == 200 + assertTraces(2) { + server.distributedRequestTrace(it, 0, trace(1).get(1)) + trace(1, 2) { + parentSpan(it, 0) + clientSpan(it, 1, span(0), false) + } + } + + where: + method = "GET" + } + + def "basic #method request with split-by-domain"() { + when: + def status = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "true") { + doRequest(method, server.address.resolve("/success")) + } + + then: + status == 200 + assertTraces(2) { + server.distributedRequestTrace(it, 0, trace(1).get(0)) + trace(1, 1) { + clientSpan(it, 0, null, true) + } + } + + where: + method = "GET" + } + + def "trace request without propagation"() { + when: + def status = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { + runUnderTrace("parent") { + doRequest(method, server.address.resolve("/success"), ["is-dd-server": "false"]) + } + } + + then: + status == 200 + // only one trace (client). + assertTraces(1) { + trace(0, 2) { + parentSpan(it, 0) + clientSpan(it, 1, span(0), renameService) + } + } + + where: + method = "GET" + renameService << [false, true] + } + + def "trace request with callback and parent"() { + when: + def status = runUnderTrace("parent") { + doRequest(method, server.address.resolve("/success"), ["is-dd-server": "false"]) { + runUnderTrace("child") {} + } + } + + then: + status == 200 + // only one trace (client). + assertTraces(1) { + trace(0, 3) { + parentSpan(it, 0) + span(1) { + operationName "child" + childOf span(0) + } + clientSpan(it, 2, span(0), false) + } + } + + where: + method = "GET" + } + + def "trace request with callback and no parent"() { + when: + def status = doRequest(method, server.address.resolve("/success"), ["is-dd-server": "false"]) { + runUnderTrace("child") { + // Ensure consistent ordering of traces for assertion. + TEST_WRITER.waitForTraces(1) + } + } + + then: + status == 200 + // only one trace (client). + assertTraces(2) { + trace(0, 1) { + clientSpan(it, 0, null, false) + } + trace(1, 1) { + span(0) { + operationName "child" + parent() + } + } + } + + where: + method = "GET" + } + + def "basic #method request with 1 redirect"() { + setup: + def uri = server.address.resolve("/redirect") + + when: + def status = doRequest(method, uri) + + then: + status == 200 + assertTraces(3) { + server.distributedRequestTrace(it, 0, trace(2).get(0)) + server.distributedRequestTrace(it, 1, trace(2).get(0)) + trace(2, 1) { + clientSpan(it, 0, null, false, uri) + } + } + + where: + method = "GET" + } + + def "basic #method request with 2 redirects"() { + setup: + def uri = server.address.resolve("/another-redirect") + + when: + def status = doRequest(method, uri) + + then: + status == 200 + assertTraces(4) { + server.distributedRequestTrace(it, 0, trace(3).get(0)) + server.distributedRequestTrace(it, 1, trace(3).get(0)) + server.distributedRequestTrace(it, 2, trace(3).get(0)) + trace(3, 1) { + clientSpan(it, 0, null, false, uri) + } + } + + where: + method = "GET" + } + + def "basic #method request with circular redirects"() { + setup: + def uri = server.address.resolve("/circular-redirect") + + when: + doRequest(method, uri)//, ["is-dd-server": "false"]) + + then: + def ex = thrown(Exception) + def thrownException = ex instanceof ExecutionException ? ex.cause : ex + + and: + assertTraces(3) { + server.distributedRequestTrace(it, 0, trace(2).get(0)) + server.distributedRequestTrace(it, 1, trace(2).get(0)) + trace(2, 1) { + clientSpan(it, 0, null, false, uri, statusOnRedirectError(), thrownException) + } + } + + where: + method = "GET" + } + + void 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) + } + } + } + } + + // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) + void clientSpan(TraceAssert trace, int index, Object parentSpan, boolean renameService, URI uri = server.address.resolve("/success"), Integer status = 200, Throwable exception = null) { + trace.span(index) { + if (parentSpan == null) { + parent() + } else { + childOf((DDSpan) parentSpan) + } + serviceName renameService ? "localhost" : "unnamed-java-app" + operationName "http.request" + resourceName "GET $uri.path" + spanType DDSpanTypes.HTTP_CLIENT + errored exception != null + tags { + defaultTags() + if (exception) { + errorTags(exception.class, exception.message) + } + "$Tags.COMPONENT.key" decorator.component() + if (status) { + "$Tags.HTTP_STATUS.key" status + } + "$Tags.HTTP_URL.key" "$uri" + "$Tags.PEER_HOSTNAME.key" "localhost" + "$Tags.PEER_PORT.key" server.address.port + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + } + } + } +} diff --git a/dd-java-agent/testing/testing.gradle b/dd-java-agent/testing/testing.gradle index cfdc48b881..da8acefcac 100644 --- a/dd-java-agent/testing/testing.gradle +++ b/dd-java-agent/testing/testing.gradle @@ -4,6 +4,7 @@ minimumBranchCoverage = 0.5 minimumInstructionCoverage = 0.5 excludedClassesConverage += [ 'datadog.trace.agent.test.asserts.*Assert', + 'datadog.trace.agent.test.base.*', 'datadog.trace.agent.test.AgentTestRunner.ErrorCountingListener', 'datadog.trace.agent.test.utils.*', // Avoid applying jacoco instrumentation to classes instrumented by tested agent diff --git a/settings.gradle b/settings.gradle index 88d16f3492..9963224de7 100644 --- a/settings.gradle +++ b/settings.gradle @@ -25,6 +25,7 @@ include ':dd-smoke-tests:wildfly' // instrumentation: include ':dd-java-agent:instrumentation:akka-http-10.0' +include ':dd-java-agent:instrumentation:apache-httpasyncclient-4' 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-2.2' From 20df3aa18e3f13bbc15cb2b1b160c5d61dd228de Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 25 Apr 2019 16:14:51 -0700 Subject: [PATCH 2/3] Add HttpAsyncClient to Elasticsearch for verification. --- .../agent/decorator/HttpClientDecorator.java | 3 ++- .../elasticsearch/rest-5/rest-5.gradle | 4 +--- .../groovy/Elasticsearch6RestClientTest.groovy | 17 ++++++++++++++++- .../groovy/Elasticsearch5RestClientTest.groovy | 17 ++++++++++++++++- .../elasticsearch/rest-6.4/rest-6.4.gradle | 6 +++--- .../groovy/Elasticsearch6RestClientTest.groovy | 17 ++++++++++++++++- .../groovy/Elasticsearch6RestClientTest.groovy | 17 ++++++++++++++++- .../transport-2/transport-2.gradle | 1 + .../transport-5.3/transport-5.3.gradle | 2 ++ .../transport-5/transport-5.gradle | 7 ++----- .../transport-6/transport-6.gradle | 6 ++---- 11 files changed, 77 insertions(+), 20 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java index ab3e25d022..9e8562ad32 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java @@ -34,7 +34,8 @@ public abstract class HttpClientDecorator extends ClientDecor Tags.HTTP_METHOD.set(span, method(request)); Tags.HTTP_URL.set(span, url(request)); Tags.PEER_HOSTNAME.set(span, hostname(request)); - Tags.PEER_PORT.set(span, port(request)); + final Integer port = port(request); + Tags.PEER_PORT.set(span, port != null && port > 0 ? port : null); if (Config.get().isHttpClientSplitByDomain()) { span.setTag(DDTags.SERVICE_NAME, hostname(request)); diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-5/rest-5.gradle b/dd-java-agent/instrumentation/elasticsearch/rest-5/rest-5.gradle index 6cb7286cea..002f2ca18d 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-5/rest-5.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/rest-5/rest-5.gradle @@ -38,10 +38,8 @@ dependencies { implementation deps.autoservice testCompile project(':dd-java-agent:testing') - // 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 Apache's HttpAsyncClient instrumentation when that is complete. testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4') + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0' testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.0' diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-5/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch/rest-5/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy index 661c563a62..06b2c4e30e 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-5/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch/rest-5/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy @@ -78,7 +78,7 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { result.status == "green" assertTraces(1) { - trace(0, 1) { + trace(0, 2) { span(0) { serviceName "elasticsearch" resourceName "GET _cluster/health" @@ -96,6 +96,21 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { defaultTags() } } + span(1) { + serviceName "elasticsearch" + resourceName "GET _cluster/health" + operationName "http.request" + spanType DDSpanTypes.HTTP_CLIENT + childOf span(0) + tags { + "$Tags.COMPONENT.key" "apache-httpasyncclient" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.HTTP_URL.key" "_cluster/health" + "$Tags.HTTP_STATUS.key" 200 + defaultTags() + } + } } } } diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-5/src/test/groovy/Elasticsearch5RestClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch/rest-5/src/test/groovy/Elasticsearch5RestClientTest.groovy index 5d297c357a..927eb1ff57 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-5/src/test/groovy/Elasticsearch5RestClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch/rest-5/src/test/groovy/Elasticsearch5RestClientTest.groovy @@ -83,7 +83,7 @@ class Elasticsearch5RestClientTest extends AgentTestRunner { result.status == "green" assertTraces(1) { - trace(0, 1) { + trace(0, 2) { span(0) { serviceName "elasticsearch" resourceName "GET _cluster/health" @@ -101,6 +101,21 @@ class Elasticsearch5RestClientTest extends AgentTestRunner { defaultTags() } } + span(1) { + serviceName "elasticsearch" + resourceName "GET _cluster/health" + operationName "http.request" + spanType DDSpanTypes.HTTP_CLIENT + childOf span(0) + tags { + "$Tags.COMPONENT.key" "apache-httpasyncclient" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.HTTP_URL.key" "_cluster/health" + "$Tags.HTTP_STATUS.key" 200 + defaultTags() + } + } } } } diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/rest-6.4.gradle b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/rest-6.4.gradle index 57e24be1d9..d9cc973e25 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/rest-6.4.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/rest-6.4.gradle @@ -39,10 +39,10 @@ dependencies { implementation deps.autoservice testCompile project(':dd-java-agent:testing') - // 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 Apache's HttpAsyncClient instrumentation when that is complete. testCompile project(':dd-java-agent:instrumentation:apache-httpclient-4') + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') + // Netty is used, but it adds complexity to the tests since we're using embedded ES. + //testCompile project(':dd-java-agent:instrumentation:netty-4.1') testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0' testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.0' diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy index 7967171399..0176c2c542 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/latestDepTest/groovy/Elasticsearch6RestClientTest.groovy @@ -82,7 +82,7 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { result.status == "green" assertTraces(1) { - trace(0, 1) { + trace(0, 2) { span(0) { serviceName "elasticsearch" resourceName "GET _cluster/health" @@ -100,6 +100,21 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { defaultTags() } } + span(1) { + serviceName "elasticsearch" + resourceName "GET _cluster/health" + operationName "http.request" + spanType DDSpanTypes.HTTP_CLIENT + childOf span(0) + tags { + "$Tags.COMPONENT.key" "apache-httpasyncclient" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.HTTP_URL.key" "_cluster/health" + "$Tags.HTTP_STATUS.key" 200 + defaultTags() + } + } } } } diff --git a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/test/groovy/Elasticsearch6RestClientTest.groovy b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/test/groovy/Elasticsearch6RestClientTest.groovy index 661c563a62..06b2c4e30e 100644 --- a/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/test/groovy/Elasticsearch6RestClientTest.groovy +++ b/dd-java-agent/instrumentation/elasticsearch/rest-6.4/src/test/groovy/Elasticsearch6RestClientTest.groovy @@ -78,7 +78,7 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { result.status == "green" assertTraces(1) { - trace(0, 1) { + trace(0, 2) { span(0) { serviceName "elasticsearch" resourceName "GET _cluster/health" @@ -96,6 +96,21 @@ class Elasticsearch6RestClientTest extends AgentTestRunner { defaultTags() } } + span(1) { + serviceName "elasticsearch" + resourceName "GET _cluster/health" + operationName "http.request" + spanType DDSpanTypes.HTTP_CLIENT + childOf span(0) + tags { + "$Tags.COMPONENT.key" "apache-httpasyncclient" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$Tags.HTTP_METHOD.key" "GET" + "$Tags.HTTP_URL.key" "_cluster/health" + "$Tags.HTTP_STATUS.key" 200 + defaultTags() + } + } } } } diff --git a/dd-java-agent/instrumentation/elasticsearch/transport-2/transport-2.gradle b/dd-java-agent/instrumentation/elasticsearch/transport-2/transport-2.gradle index c5ab3b9c88..d03188c48b 100644 --- a/dd-java-agent/instrumentation/elasticsearch/transport-2/transport-2.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/transport-2/transport-2.gradle @@ -36,6 +36,7 @@ dependencies { // Ensure no cross interference testCompile project(':dd-java-agent:instrumentation:elasticsearch:rest-5') testCompile project(':dd-java-agent:instrumentation:elasticsearch:transport-5') + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') testCompile group: 'org.elasticsearch', name: 'elasticsearch', version: '2.0.0' diff --git a/dd-java-agent/instrumentation/elasticsearch/transport-5.3/transport-5.3.gradle b/dd-java-agent/instrumentation/elasticsearch/transport-5.3/transport-5.3.gradle index 8ba24cf665..9843a6335a 100644 --- a/dd-java-agent/instrumentation/elasticsearch/transport-5.3/transport-5.3.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/transport-5.3/transport-5.3.gradle @@ -40,6 +40,8 @@ dependencies { implementation deps.autoservice testCompile project(':dd-java-agent:testing') + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') + testCompile project(':dd-java-agent:instrumentation:netty-4.1') testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0' testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.0' diff --git a/dd-java-agent/instrumentation/elasticsearch/transport-5/transport-5.gradle b/dd-java-agent/instrumentation/elasticsearch/transport-5/transport-5.gradle index 146a1d12cb..7abb2e80cd 100644 --- a/dd-java-agent/instrumentation/elasticsearch/transport-5/transport-5.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/transport-5/transport-5.gradle @@ -42,11 +42,8 @@ dependencies { testCompile project(':dd-java-agent:testing') // Ensure no cross interference testCompile project(':dd-java-agent:instrumentation:elasticsearch:rest-5') - // 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') - // TODO: add netty instrumentation when that is complete. + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') + testCompile project(':dd-java-agent:instrumentation:netty-4.1') testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0' testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.0' diff --git a/dd-java-agent/instrumentation/elasticsearch/transport-6/transport-6.gradle b/dd-java-agent/instrumentation/elasticsearch/transport-6/transport-6.gradle index 185d49faf5..ac4bd215a1 100644 --- a/dd-java-agent/instrumentation/elasticsearch/transport-6/transport-6.gradle +++ b/dd-java-agent/instrumentation/elasticsearch/transport-6/transport-6.gradle @@ -42,10 +42,8 @@ dependencies { testCompile project(':dd-java-agent:testing') // Ensure no cross interference testCompile project(':dd-java-agent:instrumentation:elasticsearch:rest-5') - // 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') + testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4') + testCompile project(':dd-java-agent:instrumentation:netty-4.1') testCompile group: 'org.elasticsearch.plugin', name: 'transport-netty4-client', version: '6.0.0' testCompile group: 'org.elasticsearch.client', name: 'transport', version: '6.0.0' From 68a68f10575b03cb92d8ad7698f793db5cdf75fb Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 25 Apr 2019 14:58:45 -0700 Subject: [PATCH 3/3] =?UTF-8?q?Ensure=20that=20http.url=20tag=20doesn?= =?UTF-8?q?=E2=80=99t=20have=20query=20params=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make handling of it more consistent in decorator. --- .../agent/decorator/HttpClientDecorator.java | 37 ++++++++++++++++- .../agent/decorator/HttpServerDecorator.java | 39 +++++++++++------- .../decorator/HttpClientDecoratorTest.groovy | 40 +++++++++++++++++-- .../decorator/HttpServerDecoratorTest.groovy | 30 +++++++++++++- .../akkahttp/AkkaHttpClientDecorator.java | 6 ++- .../akkahttp/AkkaHttpServerDecorator.java | 5 ++- .../ApacheHttpAsyncClientDecorator.java | 19 +++++---- .../ApacheHttpClientDecorator.java | 7 +++- .../aws/v0/AwsSdkClientDecorator.java | 6 ++- .../src/test/groovy/AWSClientTest.groovy | 12 +++--- .../groovy/AWSClientTest.groovy | 12 +++--- .../aws/v2/AwsSdkClientDecorator.java | 10 ++--- .../src/test/groovy/AwsClientTest.groovy | 18 +++------ .../HttpUrlConnectionDecorator.java | 6 ++- .../test/groovy/HttpUrlConnectionTest.groovy | 18 ++++----- .../src/test/groovy/UrlConnectionTest.groovy | 2 +- .../jaxrs/JaxRsClientDecorator.java | 5 ++- .../jetty8/JettyDecorator.java | 5 ++- .../client/NettyHttpClientDecorator.java | 18 +++------ .../server/NettyHttpServerDecorator.java | 18 +++------ .../client/NettyHttpClientDecorator.java | 18 +++------ .../server/NettyHttpServerDecorator.java | 18 +++------ .../okhttp3/OkHttpClientDecorator.java | 5 ++- .../play/PlayHttpServerDecorator.java | 15 +------ .../servlet2/Servlet2Decorator.java | 5 ++- .../servlet3/Servlet3Decorator.java | 5 ++- .../SpringWebHttpServerDecorator.java | 5 ++- .../agent/test/base/HttpClientTest.groovy | 3 +- 28 files changed, 231 insertions(+), 156 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java index 9e8562ad32..09dc011a8e 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java @@ -5,12 +5,16 @@ import datadog.trace.api.DDSpanTypes; import datadog.trace.api.DDTags; import io.opentracing.Span; import io.opentracing.tag.Tags; +import java.net.URI; +import java.net.URISyntaxException; +import lombok.extern.slf4j.Slf4j; +@Slf4j public abstract class HttpClientDecorator extends ClientDecorator { protected abstract String method(REQUEST request); - protected abstract String url(REQUEST request); + protected abstract URI url(REQUEST request) throws URISyntaxException; protected abstract String hostname(REQUEST request); @@ -32,7 +36,36 @@ public abstract class HttpClientDecorator extends ClientDecor assert span != null; if (request != null) { Tags.HTTP_METHOD.set(span, method(request)); - Tags.HTTP_URL.set(span, url(request)); + + // Copy of HttpServerDecorator url handling + try { + final URI url = url(request); + if (url != null) { + final StringBuilder urlNoParams = new StringBuilder(); + if (url.getScheme() != null) { + urlNoParams.append(url.getScheme()); + urlNoParams.append("://"); + } + if (url.getHost() != null) { + urlNoParams.append(url.getHost()); + if (url.getPort() > 0 && url.getPort() != 80 && url.getPort() != 443) { + urlNoParams.append(":"); + urlNoParams.append(url.getPort()); + } + } + final String path = url.getPath(); + if (path.isEmpty()) { + urlNoParams.append("/"); + } else { + urlNoParams.append(path); + } + + Tags.HTTP_URL.set(span, urlNoParams.toString()); + } + } catch (final Exception e) { + log.debug("Error tagging url", e); + } + Tags.PEER_HOSTNAME.set(span, hostname(request)); final Integer port = port(request); Tags.PEER_PORT.set(span, port != null && port > 0 ? port : null); diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java index 9e63f2e680..9fd2b0563c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java @@ -5,6 +5,7 @@ import datadog.trace.api.DDSpanTypes; import io.opentracing.Span; import io.opentracing.tag.Tags; import java.net.URI; +import java.net.URISyntaxException; import java.util.regex.Pattern; import lombok.extern.slf4j.Slf4j; @@ -17,7 +18,7 @@ public abstract class HttpServerDecorator extends protected abstract String method(REQUEST request); - protected abstract URI url(REQUEST request); + protected abstract URI url(REQUEST request) throws URISyntaxException; protected abstract String peerHostname(CONNECTION connection); @@ -42,23 +43,31 @@ public abstract class HttpServerDecorator extends if (request != null) { Tags.HTTP_METHOD.set(span, method(request)); + // Copy of HttpClientDecorator url handling try { final URI url = url(request); - final StringBuilder urlNoParams = new StringBuilder(url.getScheme()); - urlNoParams.append("://"); - urlNoParams.append(url.getHost()); - if (url.getPort() > 0 && url.getPort() != 80 && url.getPort() != 443) { - urlNoParams.append(":"); - urlNoParams.append(url.getPort()); - } - final String path = url.getPath(); - if (path.isEmpty()) { - urlNoParams.append("/"); - } else { - urlNoParams.append(path); - } + if (url != null) { + final StringBuilder urlNoParams = new StringBuilder(); + if (url.getScheme() != null) { + urlNoParams.append(url.getScheme()); + urlNoParams.append("://"); + } + if (url.getHost() != null) { + urlNoParams.append(url.getHost()); + if (url.getPort() > 0 && url.getPort() != 80 && url.getPort() != 443) { + urlNoParams.append(":"); + urlNoParams.append(url.getPort()); + } + } + final String path = url.getPath(); + if (path.isEmpty()) { + urlNoParams.append("/"); + } else { + urlNoParams.append(path); + } - Tags.HTTP_URL.set(span, urlNoParams.toString()); + Tags.HTTP_URL.set(span, urlNoParams.toString()); + } } catch (final Exception e) { log.debug("Error tagging url", e); } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy index 92fe79a0fa..0eccd44bf3 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy @@ -4,11 +4,15 @@ import datadog.trace.api.Config import datadog.trace.api.DDTags import io.opentracing.Span import io.opentracing.tag.Tags +import spock.lang.Shared import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride class HttpClientDecoratorTest extends ClientDecoratorTest { + @Shared + def testUrl = new URI("http://myhost/somepath") + def span = Mock(Span) def "test onRequest"() { @@ -23,7 +27,7 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { then: if (req) { 1 * span.setTag(Tags.HTTP_METHOD.key, "test-method") - 1 * span.setTag(Tags.HTTP_URL.key, "test-url") + 1 * span.setTag(Tags.HTTP_URL.key, "$testUrl") 1 * span.setTag(Tags.PEER_HOSTNAME.key, "test-host") 1 * span.setTag(Tags.PEER_PORT.key, 555) if (renameService) { @@ -36,8 +40,36 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { renameService | req false | null true | null - false | [method: "test-method", url: "test-url", host: "test-host", port: 555] - true | [method: "test-method", url: "test-url", host: "test-host", port: 555] + false | [method: "test-method", url: testUrl, host: "test-host", port: 555] + true | [method: "test-method", url: testUrl, host: "test-host", port: 555] + } + + def "test url handling"() { + setup: + def decorator = newDecorator() + + when: + decorator.onRequest(span, req) + + then: + if (expected) { + 1 * span.setTag(Tags.HTTP_URL.key, expected) + } + 1 * span.setTag(Tags.HTTP_METHOD.key, null) + 1 * span.setTag(Tags.PEER_HOSTNAME.key, null) + 1 * span.setTag(Tags.PEER_PORT.key, null) + 0 * _ + + where: + url | expected + null | null + "" | "/" + "/path?query" | "/path" + "https://host:0" | "https://host/" + "https://host/path" | "https://host/path" + "http://host:99/path?query#fragment" | "http://host:99/path" + + req = [url: url == null ? null : new URI(url)] } def "test onResponse"() { @@ -113,7 +145,7 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { } @Override - protected String url(Map m) { + protected URI url(Map m) { return m.url } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy index a5cacc3f51..7848b4cc30 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy @@ -1,11 +1,11 @@ package datadog.trace.agent.decorator -import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride - import datadog.trace.api.Config import io.opentracing.Span import io.opentracing.tag.Tags +import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride + class HttpServerDecoratorTest extends ServerDecoratorTest { def span = Mock(Span) @@ -34,6 +34,32 @@ class HttpServerDecoratorTest extends ServerDecoratorTest { [method: "test-method", url: URI.create("http://123:8080/some/path")] | "http://123:8080/some/path" } + def "test url handling"() { + setup: + def decorator = newDecorator() + + when: + decorator.onRequest(span, req) + + then: + if (expected) { + 1 * span.setTag(Tags.HTTP_URL.key, expected) + } + 1 * span.setTag(Tags.HTTP_METHOD.key, null) + 0 * _ + + where: + url | expected + null | null + "" | "/" + "/path?query" | "/path" + "https://host:0" | "https://host/" + "https://host/path" | "https://host/path" + "http://host:99/path?query#fragment" | "http://host:99/path" + + req = [url: url == null ? null : new URI(url)] + } + def "test onConnection"() { setup: def decorator = newDecorator() diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientDecorator.java b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientDecorator.java index 6a5e32a445..83232f9ebc 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientDecorator.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/main/scala/datadog/trace/instrumentation/akkahttp/AkkaHttpClientDecorator.java @@ -3,6 +3,8 @@ package datadog.trace.instrumentation.akkahttp; import akka.http.scaladsl.model.HttpRequest; import akka.http.scaladsl.model.HttpResponse; import datadog.trace.agent.decorator.HttpClientDecorator; +import java.net.URI; +import java.net.URISyntaxException; public class AkkaHttpClientDecorator extends HttpClientDecorator { public static final AkkaHttpClientDecorator DECORATE = new AkkaHttpClientDecorator(); @@ -23,8 +25,8 @@ public class AkkaHttpClientDecorator extends HttpClientDecorator { @@ -25,8 +26,8 @@ public class AkkaHttpServerDecorator } @Override - protected URI url(final HttpRequest httpRequest) { - return URI.create(httpRequest.uri().toString()); + protected URI url(final HttpRequest httpRequest) throws URISyntaxException { + return new URI(httpRequest.uri().toString()); } @Override diff --git a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java index c8e4a9d312..d5ce9b0c14 100644 --- a/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java +++ b/dd-java-agent/instrumentation/apache-httpasyncclient-4/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java @@ -2,6 +2,7 @@ package datadog.trace.instrumentation.apachehttpasyncclient; import datadog.trace.agent.decorator.HttpClientDecorator; import java.net.URI; +import java.net.URISyntaxException; import org.apache.http.HttpRequest; import org.apache.http.HttpResponse; import org.apache.http.RequestLine; @@ -30,18 +31,19 @@ public class ApacheHttpAsyncClientDecorator extends HttpClientDecorator { @@ -24,8 +26,9 @@ public class ApacheHttpClientDecorator extends HttpClientDecorator { @@ -23,8 +25,8 @@ public class HttpUrlConnectionDecorator extends HttpClientDecorator extends AgentTestRu def "basic #method request"() { when: - def status = doRequest(method, server.address.resolve("/success")) + def status = doRequest(method, server.address.resolve(url)) then: status == 200 @@ -78,6 +78,7 @@ abstract class HttpClientTest extends AgentTestRu where: method = "GET" + url << ["/success", "/success?with=params"] } def "basic #method request with parent"() {