From 2ab92f556e9de58442b5d4bbba43151691962b6a Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 23 Apr 2021 13:05:10 -0700 Subject: [PATCH] Fix duplicate http client tracing headers (#2842) * Fix duplicate spring webclient tracing headers * Fix Akka Http * Fix Armeria * Fix Java 11 Http Client * Fix OkHttp 2.2 * Fix PlayWS --- .../AkkaHttpClientInstrumentationModule.java | 3 +- .../v1_3/ClientRequestContextSetter.java | 2 +- .../httpclient/JdkHttpClientTracer.java | 3 +- .../src/test/groovy/OkHttp2Test.groovy | 7 ++-- .../playws/HeadersInjectAdapter.java | 2 +- .../client/HttpHeadersInjectAdapter.java | 2 +- .../test/base/HttpClientTest.groovy | 40 +++++++++++++++++-- .../test/server/http/TestHttpServer.groovy | 2 +- 8 files changed, 47 insertions(+), 14 deletions(-) diff --git a/instrumentation/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/AkkaHttpClientInstrumentationModule.java b/instrumentation/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/AkkaHttpClientInstrumentationModule.java index 22d89b6046e4..4a501d970bb7 100644 --- a/instrumentation/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/AkkaHttpClientInstrumentationModule.java +++ b/instrumentation/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/AkkaHttpClientInstrumentationModule.java @@ -154,7 +154,8 @@ public void set(AkkaHttpHeaders carrier, String key, String value) { HttpRequest request = carrier.getRequest(); if (request != null) { // It looks like this cast is only needed in Java, Scala would have figured it out - carrier.setRequest((HttpRequest) request.addHeader(RawHeader.create(key, value))); + carrier.setRequest( + (HttpRequest) request.removeHeader(key).addHeader(RawHeader.create(key, value))); } } } diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ClientRequestContextSetter.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ClientRequestContextSetter.java index dcca604ceed9..ff4aee9ebe28 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ClientRequestContextSetter.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ClientRequestContextSetter.java @@ -15,7 +15,7 @@ enum ClientRequestContextSetter implements TextMapSetter { @Override public void set(@Nullable ClientRequestContext carrier, String key, String value) { if (carrier != null) { - carrier.addAdditionalRequestHeader(key, value); + carrier.setAdditionalRequestHeader(key, value); } } } diff --git a/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java b/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java index b05e07d2a4df..525960061a8e 100644 --- a/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java +++ b/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java @@ -80,13 +80,12 @@ protected TextMapSetter getSetter() { } public HttpHeaders inject(HttpHeaders original) { - Map> headerMap = new HashMap<>(); + Map> headerMap = new HashMap<>(original.map()); inject( Context.current(), headerMap, (carrier, key, value) -> carrier.put(key, Collections.singletonList(value))); - headerMap.putAll(original.map()); return HttpHeaders.of(headerMap, (s, s2) -> true); } diff --git a/instrumentation/okhttp/okhttp-2.2/javaagent/src/test/groovy/OkHttp2Test.groovy b/instrumentation/okhttp/okhttp-2.2/javaagent/src/test/groovy/OkHttp2Test.groovy index e1a38a749a1a..bb22ebfb918b 100644 --- a/instrumentation/okhttp/okhttp-2.2/javaagent/src/test/groovy/OkHttp2Test.groovy +++ b/instrumentation/okhttp/okhttp-2.2/javaagent/src/test/groovy/OkHttp2Test.groovy @@ -4,7 +4,6 @@ */ import com.squareup.okhttp.Callback -import com.squareup.okhttp.Headers import com.squareup.okhttp.MediaType import com.squareup.okhttp.OkHttpClient import com.squareup.okhttp.Request @@ -27,11 +26,11 @@ class OkHttp2Test extends HttpClientTest implements AgentTestTrait { @Override Request buildRequest(String method, URI uri, Map headers) { def body = HttpMethod.requiresRequestBody(method) ? RequestBody.create(MediaType.parse("text/plain"), "") : null - return new Request.Builder() + def request = new Request.Builder() .url(uri.toURL()) .method(method, body) - .headers(Headers.of(HeadersUtil.headersToArray(headers))) - .build() + headers.forEach({ key, value -> request.header(key, value) }) + return request.build() } @Override diff --git a/instrumentation/play-ws/play-ws-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/HeadersInjectAdapter.java b/instrumentation/play-ws/play-ws-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/HeadersInjectAdapter.java index b9953fffc473..8273c518cce4 100644 --- a/instrumentation/play-ws/play-ws-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/HeadersInjectAdapter.java +++ b/instrumentation/play-ws/play-ws-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/HeadersInjectAdapter.java @@ -14,6 +14,6 @@ public class HeadersInjectAdapter implements TextMapSetter { @Override public void set(HttpHeaders carrier, String key, String value) { - carrier.add(key, value); + carrier.set(key, value); } } diff --git a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/HttpHeadersInjectAdapter.java b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/HttpHeadersInjectAdapter.java index 2f73eee4df4e..518b7d4bb943 100644 --- a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/HttpHeadersInjectAdapter.java +++ b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/HttpHeadersInjectAdapter.java @@ -14,6 +14,6 @@ class HttpHeadersInjectAdapter implements TextMapSetter { @Override public void set(ClientRequest.Builder carrier, String key, String value) { - carrier.header(key, value); + carrier.headers(httpHeaders -> httpHeaders.set(key, value)); } } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy index 4267a76c9ee2..59632d02fd95 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy @@ -18,6 +18,7 @@ import static org.junit.Assume.assumeTrue import groovy.transform.stc.ClosureParams import groovy.transform.stc.SimpleType +import io.opentelemetry.api.GlobalOpenTelemetry import io.opentelemetry.api.common.AttributeKey import io.opentelemetry.api.trace.Span import io.opentelemetry.instrumentation.test.InstrumentationSpecification @@ -95,10 +96,18 @@ abstract class HttpClientTest extends InstrumentationSpecification { return sendRequest(request, method, uri, headers) } - // ideally private, but then groovy closures in this class cannot find them - final int doReusedRequest(String method, URI uri, Map headers = [:]) { + private int doReusedRequest(String method, URI uri) { + def request = buildRequest(method, uri, [:]) + sendRequest(request, method, uri, [:]) + return sendRequest(request, method, uri, [:]) + } + + private int doRequestWithExistingTracingHeaders(String method, URI uri) { + def headers = new HashMap() + for (String field : GlobalOpenTelemetry.getPropagators().getTextMapPropagator().fields()) { + headers.put(field, "12345789") + } def request = buildRequest(method, uri, headers) - sendRequest(request, method, uri, headers) return sendRequest(request, method, uri, headers) } @@ -509,6 +518,31 @@ abstract class HttpClientTest extends InstrumentationSpecification { url = server.address.resolve(path) } + // this test verifies two things: + // * the javaagent doesn't cause multiples of tracing headers to be added + // (TestHttpServer throws exception if there are multiples) + // * the javaagent overwrites the existing tracing headers + // (so that it propagates the same trace id / span id that it reports to the backend + // and the trace is not broken) + def "request with existing tracing headers"() { + when: + def responseCode = doRequestWithExistingTracingHeaders(method, url) + + then: + responseCode == 200 + assertTraces(1) { + trace(0, 2 + extraClientSpans()) { + clientSpan(it, 0, null, method, url) + serverSpan(it, 1 + extraClientSpans(), span(extraClientSpans())) + } + } + + where: + path = "/success" + method = "GET" + url = server.address.resolve(path) + } + def "connection error (unopened port)"() { given: assumeTrue(testConnectionFailure()) diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/server/http/TestHttpServer.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/server/http/TestHttpServer.groovy index e8a479f97473..00068fc5d41b 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/server/http/TestHttpServer.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/server/http/TestHttpServer.groovy @@ -267,7 +267,7 @@ class TestHttpServer implements AutoCloseable { for (String field : GlobalOpenTelemetry.getPropagators().getTextMapPropagator().fields()) { def headers = req.getHeaders(field) if (headers.hasMoreElements() && headers.nextElement() && headers.hasMoreElements()) { - throw new AssertionError("more than one traceparent header present") + throw new AssertionError("more than one " + field + " header present") } } }