From 03632d9bae05a31ed0edd6ceb3b95221c551e116 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 1 Aug 2021 21:12:37 -0700 Subject: [PATCH] Fix JDK http client propagation of non-sampled traces (#3736) * Fix JDK http client propagation of non-sampled traces * Lower wait time --- .../httpclient/HttpHeadersInstrumentation.java | 5 +---- .../test/base/HttpClientTest.groovy | 6 +++++- .../testing/AgentTestRunner.java | 6 ++++++ .../testing/InstrumentationTestRunner.java | 3 +++ .../testing/LibraryTestRunner.java | 6 ++++++ .../testing/TestInstrumenters.java | 17 +++++++++++++++++ .../junit/http/AbstractHttpClientTest.java | 14 ++++++++++++++ 7 files changed, 52 insertions(+), 5 deletions(-) diff --git a/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersInstrumentation.java b/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersInstrumentation.java index 08eeccd6b9b5..3505775169d0 100644 --- a/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersInstrumentation.java +++ b/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersInstrumentation.java @@ -13,7 +13,6 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import java.net.http.HttpHeaders; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; @@ -40,9 +39,7 @@ public static class HeadersAdvice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void methodExit(@Advice.Return(readOnly = false) HttpHeaders headers) { - if (Java8BytecodeBridge.currentSpan().isRecording()) { - headers = tracer().inject(headers); - } + headers = tracer().inject(headers); } } } 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 5dc1034fbb3f..b6f3cf47a487 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 @@ -263,6 +263,11 @@ abstract class HttpClientTest extends InstrumentationSpecification { method << BODY_METHODS } + def "basic GET request with not sampled parent"() { + expect: + junitTest.successfulRequestWithNotSampledParent() + } + def "should suppress nested CLIENT span if already under parent CLIENT span (#method)"() { assumeTrue(testWithClientParent()) expect: @@ -272,7 +277,6 @@ abstract class HttpClientTest extends InstrumentationSpecification { method << BODY_METHODS } - //FIXME: add tests for POST with large/chunked data def "trace request with callback and parent"() { diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java index c14f17c2034e..fe5ecc7357d4 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java @@ -105,4 +105,10 @@ public T runWithServerSpan( String spanName, ThrowingSupplier callback) throws E { return testInstrumenters.runWithServerSpan(spanName, callback); } + + @Override + public T runWithNonRecordingSpan(ThrowingSupplier callback) + throws E { + return testInstrumenters.runWithNonRecordingSpan(callback); + } } diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/InstrumentationTestRunner.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/InstrumentationTestRunner.java index 790c4cbaaefd..2dc6c0e8b06a 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/InstrumentationTestRunner.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/InstrumentationTestRunner.java @@ -142,4 +142,7 @@ default void runWithServerSpan( */ T runWithServerSpan(String spanName, ThrowingSupplier callback) throws E; + + /** Runs the provided {@code callback} inside the scope of a non-recording span. */ + T runWithNonRecordingSpan(ThrowingSupplier callback) throws E; } diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/LibraryTestRunner.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/LibraryTestRunner.java index 0a8d199fdc87..52e978938b31 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/LibraryTestRunner.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/LibraryTestRunner.java @@ -124,6 +124,12 @@ public T runWithServerSpan( return testInstrumenters.runWithServerSpan(spanName, callback); } + @Override + public T runWithNonRecordingSpan(ThrowingSupplier callback) + throws E { + return testInstrumenters.runWithNonRecordingSpan(callback); + } + private static class FlushTrackingSpanProcessor implements SpanProcessor { @Override public void onStart(Context parentContext, ReadWriteSpan span) {} diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/TestInstrumenters.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/TestInstrumenters.java index 6cddd27ce9da..5ebfd7db36c6 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/TestInstrumenters.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/TestInstrumenters.java @@ -6,6 +6,10 @@ package io.opentelemetry.instrumentation.testing; import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.api.trace.TraceFlags; +import io.opentelemetry.api.trace.TraceState; import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; @@ -58,6 +62,19 @@ T runWithServerSpan(String spanName, ThrowingSupplier T runWithNonRecordingSpan(ThrowingSupplier callback) throws E { + SpanContext spanContext = + SpanContext.create( + "12345678123456781234567812345678", + "1234567812345678", + TraceFlags.getDefault(), + TraceState.getDefault()); + try (Scope ignored = Span.wrap(spanContext).makeCurrent()) { + return callback.get(); + } + } + private static T runWithInstrumenter( String spanName, Instrumenter instrumenter, ThrowingSupplier callback) throws E { diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java index 095d90be1b70..9f4a78dcb311 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java @@ -257,6 +257,20 @@ void successfulRequestWithParent(String method) { }); } + @Test + void successfulRequestWithNotSampledParent() throws InterruptedException { + String method = "GET"; + URI uri = resolveAddress("/success"); + int responseCode = testing.runWithNonRecordingSpan(() -> doRequest(method, uri)); + + assertThat(responseCode).isEqualTo(200); + + // sleep to ensure no spans are emitted + Thread.sleep(200); + + assertThat(testing.traces()).isEmpty(); + } + @ParameterizedTest @ValueSource(strings = {"PUT", "POST"}) void shouldSuppressNestedClientSpanIfAlreadyUnderParentClientSpan(String method) {