Separate span for input and output streams.
MODIFY
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java
@@ -57,7 +57,12 @@ public class AgentInstaller {
.or(named("java.net.HttpURLConnection"))
.or(nameStartsWith("java.util.concurrent.")))))
.or(nameStartsWith("com.sun."))
- .or(nameStartsWith("sun.").and(not(nameStartsWith("sun.net.www.protocol."))))
+ .or(
+ nameStartsWith("sun.")
+ .and(
+ not(
+ nameStartsWith("sun.net.www.protocol.")
+ .or(named("sun.net.www.http.HttpClient")))))
.or(nameStartsWith("jdk."))
.or(nameStartsWith("org.aspectj."))
.or(nameStartsWith("org.groovy."))

MODIFY
dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java
dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java
@@ -4,7 +4,6 @@ import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperT
import static io.opentracing.log.Fields.ERROR_OBJECT;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
-import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;
@@ -16,13 +15,11 @@ import datadog.trace.bootstrap.CallDepthThreadLocalMap;
import io.opentracing.Scope;
import io.opentracing.Span;
import io.opentracing.Tracer;
-import io.opentracing.propagation.Format;
import io.opentracing.tag.Tags;
import io.opentracing.util.GlobalTracer;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.Collections;
-import java.util.HashMap;
import java.util.Map;
import java.util.WeakHashMap;
import javax.net.ssl.HttpsURLConnection;
@@ -50,26 +47,16 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
- return new String[] {
- "datadog.trace.instrumentation.http_url_connection.MessageHeadersInjectAdapter",
- HttpUrlConnectionInstrumentation.class.getName() + "$HttpURLState"
- };
+ return new String[] {HttpUrlConnectionInstrumentation.class.getName() + "$HttpURLState"};
}
@Override
public Map<ElementMatcher, String> transformers() {
- final Map<ElementMatcher, String> transformers = new HashMap<>();
- transformers.put(
+ return Collections.<ElementMatcher, String>singletonMap(
isMethod()
.and(isPublic())
- .and(
- named("getResponseCode")
- .or(named("connect"))
- .or(named("getOutputStream"))
- .or(named("getInputStream"))
- .or(nameStartsWith("getHeaderField"))),
+ .and(named("connect").or(named("getOutputStream")).or(named("getInputStream"))),
HttpUrlConnectionAdvice.class.getName());
- return transformers;
}
public static class HttpUrlConnectionAdvice {
@@ -82,22 +69,37 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default {
final HttpURLState state = HttpURLState.get(thiz);
String operationName = "http.request";
- if ("connect".equals(methodName)) {
+
+ switch (methodName) {
+ case "connect":
if (connected) {
return null;
}
// We get here in cases where connect() is called first.
- // We need to inject headers now because after connected=true no more headers can be added.
+ // We need to inject headers now because after connected=true no more headers can be
+ // added.
// In total there will be two spans:
// - one for the connect() which does propagation
// - one for the input or output stream (presumably called after connect())
operationName += ".connect";
- } else {
- if (state.hasDoneIO()) {
+ break;
+
+ case "getOutputStream":
+ if (state.calledOutputStream) {
+ return null;
+ }
+ state.calledOutputStream = true;
+ operationName += ".output-stream";
+ break;
+
+ case "getInputStream":
+ if (state.calledInputStream) {
return null;
}
- state.setHasDoneIO(true);
+ state.calledInputStream = true;
+ operationName += ".input-stream";
+ break;
}
// AgentWriter uses HttpURLConnection to report to the trace-agent. We don't want to trace
@@ -143,8 +145,6 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default {
}
Tags.HTTP_METHOD.set(span, thiz.getRequestMethod());
- tracer.inject(
- span.context(), Format.Builtin.HTTP_HEADERS, new MessageHeadersInjectAdapter(thiz));
return scope;
}
@@ -187,16 +187,9 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default {
return state;
}
- public boolean hasDoneIO = false;
+ public boolean calledOutputStream = false;
+ public boolean calledInputStream = false;
private HttpURLState() {}
-
- public boolean hasDoneIO() {
- return hasDoneIO;
- }
-
- public void setHasDoneIO(final boolean value) {
- hasDoneIO = value;
- }
}
}

DELETE
dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/MessageHeadersInjectAdapter.java
-package datadog.trace.instrumentation.http_url_connection;
-
-import io.opentracing.propagation.TextMap;
-import java.net.HttpURLConnection;
-import java.util.Iterator;
-import java.util.Map;
-
-public class MessageHeadersInjectAdapter implements TextMap {
-
- private final HttpURLConnection connection;
-
- public MessageHeadersInjectAdapter(final HttpURLConnection connection) {
- this.connection = connection;
- }
-
- @Override
- public void put(final String key, final String value) {
- try {
- if (connection.getRequestProperty(key) == null) {
- connection.setRequestProperty(key, value);
- }
- } catch (final IllegalStateException e) {
- // Connection is already established. Too late to set headers.
- }
- }
-
- @Override
- public Iterator<Map.Entry<String, String>> iterator() {
- throw new UnsupportedOperationException("This class should be used only with tracer#inject()");
- }
-}

ADD
dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/SunHttpClientInstrumentation.java
+package datadog.trace.instrumentation.http_url_connection;
+
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.isPublic;
+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 io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.propagation.Format;
+import io.opentracing.propagation.TextMap;
+import io.opentracing.util.GlobalTracer;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.Map;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.matcher.ElementMatcher;
+import sun.net.www.MessageHeader;
+import sun.net.www.http.HttpClient;
+
+@AutoService(Instrumenter.class)
+public class SunHttpClientInstrumentation extends Instrumenter.Default {
+
+ public SunHttpClientInstrumentation() {
+ super("httpurlconnection");
+ }
+
+ @Override
+ protected boolean defaultEnabled() {
+ return false;
+ }
+
+ @Override
+ public ElementMatcher typeMatcher() {
+ return named("sun.net.www.http.HttpClient");
+ }
+
+ @Override
+ public String[] helperClassNames() {
+ return new String[] {
+ SunHttpClientInstrumentation.class.getName() + "$MessageHeadersInjectAdapter"
+ };
+ }
+
+ @Override
+ public Map<ElementMatcher, String> transformers() {
+ return Collections.<ElementMatcher, String>singletonMap(
+ isMethod()
+ .and(isPublic())
+ .and(named("writeRequests"))
+ .and(takesArgument(0, named("sun.net.www.MessageHeader")))
+ // exclude the delegating method:
+ .and(takesArguments(1).or(takesArguments(2))),
+ InjectAdvice.class.getName());
+ }
+
+ public static class InjectAdvice {
+
+ @Advice.OnMethodEnter(suppress = Throwable.class)
+ public static void inject(
+ @Advice.Argument(0) final MessageHeader header, @Advice.This final HttpClient client) {
+ final Tracer tracer = GlobalTracer.get();
+ final Span span = tracer.activeSpan();
+ // Use the active span, not the one created in SpanStartAdvice,
+ // since we aren't sure that span will complete.
+
+ if (span != null) {
+ tracer.inject(
+ span.context(), Format.Builtin.HTTP_HEADERS, new MessageHeadersInjectAdapter(header));
+ }
+ }
+ }
+
+ public static class MessageHeadersInjectAdapter implements TextMap {
+
+ private final MessageHeader header;
+
+ public MessageHeadersInjectAdapter(final MessageHeader header) {
+ this.header = header;
+ }
+
+ @Override
+ public void put(final String key, final String value) {
+ header.setIfNotSet(key, value);
+ }
+
+ @Override
+ public Iterator<Map.Entry<String, String>> iterator() {
+ throw new UnsupportedOperationException(
+ "This class should be used only with tracer#inject()");
+ }
+ }
+}

MODIFY
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy
@@ -29,6 +29,10 @@ class HttpUrlConnectionTest extends AgentTestRunner {
all {
RatpackUtils.handleDistributedRequest(context)
+ request.body.then {
+ if (it != null) {
+ println "RECEIVED: $it.text"
+ }
response.status(STATUS)
// Ratpack seems to be sending body with HEAD requests - RFC specifically forbids this.
// This becomes a major problem with keep-alived requests - client seems to fail to parse
@@ -41,11 +45,13 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
}
+ }
def "trace request with propagation"() {
setup:
runUnderTrace("someTrace") {
HttpURLConnection connection = server.address.toURL().openConnection()
+ connection.useCaches = useCaches
assert GlobalTracer.get().scopeManager().active() != null
def stream = connection.inputStream
def lines = stream.readLines()
@@ -55,9 +61,11 @@ class HttpUrlConnectionTest extends AgentTestRunner {
// call again to ensure the cycling is ok
connection = server.getAddress().toURL().openConnection()
+ connection.useCaches = useCaches
assert GlobalTracer.get().scopeManager().active() != null
assert connection.getResponseCode() == STATUS // call before input stream to test alternate behavior
- stream = connection.inputStream
+ connection.inputStream
+ stream = connection.inputStream // one more to ensure state is working
lines = stream.readLines()
stream.close()
assert lines == [RESPONSE]
@@ -95,7 +103,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -111,7 +119,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(2) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -128,15 +136,20 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
}
+
+ where:
+ useCaches << [false, true]
}
def "trace request without propagation"() {
setup:
runUnderTrace("someTrace") {
HttpURLConnection connection = server.address.toURL().openConnection()
+ connection.useCaches = useCaches
connection.addRequestProperty("is-dd-server", "false")
assert GlobalTracer.get().scopeManager().active() != null
def stream = connection.inputStream
+ connection.inputStream // one more to ensure state is working
def lines = stream.readLines()
stream.close()
assert connection.getResponseCode() == STATUS
@@ -144,6 +157,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
// call again to ensure the cycling is ok
connection = server.getAddress().toURL().openConnection()
+ connection.useCaches = useCaches
connection.addRequestProperty("is-dd-server", "false")
assert GlobalTracer.get().scopeManager().active() != null
assert connection.getResponseCode() == STATUS // call before input stream to test alternate behavior
@@ -165,7 +179,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -181,7 +195,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(2) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -198,6 +212,9 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
}
+
+ where:
+ useCaches << [false, true]
}
def "test response code"() {
@@ -222,7 +239,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -241,6 +258,54 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
+ def "test broken API usage"() {
+ setup:
+ HttpURLConnection conn = runUnderTrace("someTrace") {
+ HttpURLConnection connection = server.address.toURL().openConnection()
+ connection.setRequestProperty("Connection", "close")
+ connection.addRequestProperty("is-dd-server", "false")
+ assert GlobalTracer.get().scopeManager().active() != null
+ assert connection.getResponseCode() == STATUS
+ return connection
+ }
+
+ expect:
+ assertTraces(TEST_WRITER, 1) {
+ trace(0, 2) {
+ span(0) {
+ operationName "someTrace"
+ parent()
+ errored false
+ tags {
+ defaultTags()
+ }
+ }
+ span(1) {
+ operationName "http.request.input-stream"
+ childOf span(0)
+ errored false
+ tags {
+ "$Tags.COMPONENT.key" "HttpURLConnection"
+ "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
+ "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
+ "$Tags.HTTP_URL.key" "$server.address"
+ "$Tags.HTTP_METHOD.key" "GET"
+ "$Tags.HTTP_STATUS.key" STATUS
+ "$Tags.PEER_HOSTNAME.key" "localhost"
+ "$Tags.PEER_PORT.key" server.address.port
+ defaultTags()
+ }
+ }
+ }
+ }
+
+ cleanup:
+ conn.disconnect()
+
+ where:
+ iteration << (1..10)
+ }
+
def "test post request"() {
setup:
runUnderTrace("someTrace") {
@@ -276,7 +341,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
}
- trace(1, 2) {
+ trace(1, 3) {
span(0) {
operationName "someTrace"
parent()
@@ -286,7 +351,23 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
+ childOf span(0)
+ errored false
+ tags {
+ "$Tags.COMPONENT.key" "HttpURLConnection"
+ "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
+ "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
+ "$Tags.HTTP_URL.key" "$server.address"
+ "$Tags.HTTP_METHOD.key" "POST"
+ "$Tags.HTTP_STATUS.key" STATUS
+ "$Tags.PEER_HOSTNAME.key" "localhost"
+ "$Tags.PEER_PORT.key" server.address.port
+ defaultTags()
+ }
+ }
+ span(2) {
+ operationName "http.request.output-stream"
childOf span(0)
errored false
tags {
@@ -350,7 +431,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
setup:
runUnderTrace("someTrace") {
RestTemplate restTemplate = new RestTemplate()
- String res = restTemplate.getForObject(server.address.toString(), String)
+ String res = restTemplate.postForObject(server.address.toString(), "Hello", String)
assert res == RESPONSE
}
@@ -366,7 +447,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
}
- trace(1, 3) {
+ trace(1, 4) {
span(0) {
operationName "someTrace"
parent()
@@ -376,7 +457,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored false
tags {
@@ -384,7 +465,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
"$Tags.HTTP_URL.key" "$server.address"
- "$Tags.HTTP_METHOD.key" "GET"
+ "$Tags.HTTP_METHOD.key" "POST"
"$Tags.HTTP_STATUS.key" STATUS
"$Tags.PEER_HOSTNAME.key" "localhost"
"$Tags.PEER_PORT.key" server.address.port
@@ -392,6 +473,21 @@ class HttpUrlConnectionTest extends AgentTestRunner {
}
}
span(2) {
+ operationName "http.request.output-stream"
+ childOf span(0)
+ errored false
+ tags {
+ "$Tags.COMPONENT.key" "HttpURLConnection"
+ "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
+ "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
+ "$Tags.HTTP_URL.key" "$server.address"
+ "$Tags.HTTP_METHOD.key" "POST"
+ "$Tags.PEER_HOSTNAME.key" "localhost"
+ "$Tags.PEER_PORT.key" server.address.port
+ defaultTags()
+ }
+ }
+ span(3) {
operationName "http.request.connect"
childOf span(0)
errored false
@@ -400,7 +496,7 @@ class HttpUrlConnectionTest extends AgentTestRunner {
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT
"$Tags.HTTP_URL.key" "$server.address"
- "$Tags.HTTP_METHOD.key" "GET"
+ "$Tags.HTTP_METHOD.key" "POST"
"$Tags.PEER_HOSTNAME.key" "localhost"
"$Tags.PEER_PORT.key" server.address.port
defaultTags()

MODIFY
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy
@@ -41,7 +41,7 @@ class UrlConnectionTest extends AgentTestRunner {
}
}
span(1) {
- operationName "http.request"
+ operationName "http.request.input-stream"
childOf span(0)
errored true
tags {

MODIFY
dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy
dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/RatpackUtils.groovy
package datadog.trace.agent.test
-import io.opentracing.Scope
import io.opentracing.SpanContext
import io.opentracing.propagation.Format
import io.opentracing.propagation.TextMap
@@ -18,12 +17,12 @@ class RatpackUtils {
final SpanContext extractedContext =
GlobalTracer.get()
.extract(Format.Builtin.HTTP_HEADERS, new RatpackResponseAdapter(context))
- Scope scope =
- GlobalTracer.get()
+ def builder = GlobalTracer.get()
.buildSpan("test-http-server")
- .asChildOf(extractedContext)
- .startActive(true)
- scope.close()
+ if (extractedContext != null) {
+ builder.asChildOf(extractedContext)
+ }
+ builder.start().finish()
}
}

Separate span for input and output streams.