From 6413cfcdf0594b312f7d30279c255ccd0bc97c65 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 30 Jul 2021 10:53:13 -0700 Subject: [PATCH 1/2] Fix JDK http client propagation of non-sampled traces --- .../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 9495756ab7f2..25bef24ed6fb 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 @@ -262,6 +262,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: @@ -271,7 +276,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 13698548fc75..9bb189b04afb 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 @@ -195,6 +195,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(1000); + + assertThat(testing.traces()).isEmpty(); + } + @ParameterizedTest @ValueSource(strings = {"PUT", "POST"}) void shouldSuppressNestedClientSpanIfAlreadyUnderParentClientSpan(String method) { From fdba99cf436e453b8f8200aefd1e1e9851f203bb Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 31 Jul 2021 08:14:54 -0700 Subject: [PATCH 2/2] Lower wait time --- .../testing/junit/http/AbstractHttpClientTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9bb189b04afb..b5ad64f4a7c3 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 @@ -204,7 +204,7 @@ void successfulRequestWithNotSampledParent() throws InterruptedException { assertThat(responseCode).isEqualTo(200); // sleep to ensure no spans are emitted - Thread.sleep(1000); + Thread.sleep(200); assertThat(testing.traces()).isEmpty(); }