diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java index f0c301b01f..4d032395d1 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java @@ -28,11 +28,16 @@ final class OpenTelemetryClient extends SimpleDecoratingHttpClient { @Override public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Exception { + Context parentContext = Context.current(); + if (!clientTracer.shouldStartSpan(parentContext)) { + return unwrap().execute(ctx, req); + } + // Always available in practice. long requestStartTimeMicros = ctx.log().ensureAvailable(RequestLogProperty.REQUEST_START_TIME).requestStartTimeMicros(); long requestStartTimeNanos = TimeUnit.MICROSECONDS.toNanos(requestStartTimeMicros); - Context context = clientTracer.startSpan(Context.current(), ctx, ctx, requestStartTimeNanos); + Context context = clientTracer.startSpan(parentContext, ctx, ctx, requestStartTimeNanos); Span span = Span.fromContext(context); if (span.isRecording()) { diff --git a/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy b/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy index bf24a2acd8..1e093e0587 100644 --- a/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy +++ b/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy @@ -13,4 +13,10 @@ class ArmeriaHttpClientTest extends AbstractArmeriaHttpClientTest implements Lib WebClientBuilder configureClient(WebClientBuilder clientBuilder) { return clientBuilder.decorator(ArmeriaTracing.create(getOpenTelemetry()).newClientDecorator()) } + + // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet + @Override + boolean testWithClientParent() { + false + } } diff --git a/instrumentation/java-httpclient/javaagent/src/test/groovy/JdkHttpClientTest.groovy b/instrumentation/java-httpclient/javaagent/src/test/groovy/JdkHttpClientTest.groovy index c67f07bccb..1323eebb4e 100644 --- a/instrumentation/java-httpclient/javaagent/src/test/groovy/JdkHttpClientTest.groovy +++ b/instrumentation/java-httpclient/javaagent/src/test/groovy/JdkHttpClientTest.groovy @@ -52,6 +52,13 @@ abstract class JdkHttpClientTest extends HttpClientTest implements AgentTestTrai return false } + // TODO nested client span is not created, but context is still injected + // which is not what the test expects + @Override + boolean testWithClientParent() { + false + } + @Requires({ !System.getProperty("java.vm.name").contains("IBM J9 VM") }) def "test https request"() { given: diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3AsyncTest.groovy b/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3AsyncTest.groovy index f39feb470d..9eb4ba1b54 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3AsyncTest.groovy +++ b/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3AsyncTest.groovy @@ -16,4 +16,10 @@ class OkHttp3AsyncTest extends AbstractOkHttp3AsyncTest implements LibraryTestTr OkHttpClient.Builder configureClient(OkHttpClient.Builder clientBuilder) { return clientBuilder.addInterceptor(OkHttpTracing.create(getOpenTelemetry()).newInterceptor()) } + + // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet + @Override + boolean testWithClientParent() { + false + } } diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy b/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy index 15f99d6032..8ff56daa0d 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy +++ b/instrumentation/okhttp/okhttp-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy @@ -13,4 +13,10 @@ class OkHttp3Test extends AbstractOkHttp3Test implements LibraryTestTrait { OkHttpClient.Builder configureClient(OkHttpClient.Builder clientBuilder) { return clientBuilder.addInterceptor(OkHttpTracing.create(getOpenTelemetry()).newInterceptor()) } + + // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet + @Override + boolean testWithClientParent() { + false + } } diff --git a/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy index 1f585a3ac4..62c6f2ac9f 100644 --- a/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy +++ b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy @@ -45,4 +45,10 @@ class RestTemplateInstrumentationTest extends HttpClientTest implements LibraryT boolean testRemoteConnection() { false } + + // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet + @Override + boolean testWithClientParent() { + false + } } diff --git a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java index b1ca7d3f6a..6c354870e5 100644 --- a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java +++ b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java @@ -51,6 +51,7 @@ public class WebClientTracingFilter implements ExchangeFilterFunction { public void subscribe(CoreSubscriber subscriber) { Context parentContext = Context.current(); if (!tracer().shouldStartSpan(parentContext)) { + next.exchange(request).subscribe(subscriber); return; } diff --git a/instrumentation/vertx-web-3.0/javaagent/src/test/groovy/client/VertxHttpClientTest.groovy b/instrumentation/vertx-web-3.0/javaagent/src/test/groovy/client/VertxHttpClientTest.groovy index c918e9dbec..8a83be6518 100644 --- a/instrumentation/vertx-web-3.0/javaagent/src/test/groovy/client/VertxHttpClientTest.groovy +++ b/instrumentation/vertx-web-3.0/javaagent/src/test/groovy/client/VertxHttpClientTest.groovy @@ -56,6 +56,14 @@ class VertxHttpClientTest extends HttpClientTest implements AgentTestTrait { false } + // TODO the vertx client span is suppressed, but also so is the vertx client instrumentation + // context propagation down to netty, and so netty doesn't see any existing context, + // and so it creates a (not-nested) client span, which is not what the test expects + @Override + boolean testWithClientParent() { + false + } + @Override SingleConnection createSingleConnection(String host, int port) { //This test fails on Vert.x 3.0 and only works starting from 3.1 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 29066b8a76..712a027080 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 @@ -10,6 +10,7 @@ import static io.opentelemetry.api.trace.SpanKind.SERVER import static io.opentelemetry.instrumentation.test.server.http.TestHttpServer.httpServer import static io.opentelemetry.instrumentation.test.utils.PortUtils.UNUSABLE_PORT import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan +import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderParentClientSpan import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace import static org.junit.Assume.assumeTrue @@ -137,6 +138,33 @@ abstract class HttpClientTest extends InstrumentationSpecification { method << BODY_METHODS } + def "should suppress nested CLIENT span if already under parent CLIENT span"() { + given: + assumeTrue(testWithClientParent()) + + when: + def status = runUnderParentClientSpan { + doRequest(method, server.address.resolve("/success")) + } + + then: + status == 200 + // there should be 2 separate traces since the nested CLIENT span is suppressed + // (and the span context propagation along with it) + assertTraces(2) { + trace(0, 1) { + basicSpan(it, 0, "parent-client-span") + } + trace(1, 1) { + serverSpan(it, 0) + } + } + + where: + method << BODY_METHODS + } + + //FIXME: add tests for POST with large/chunked data def "trace request without propagation"() { @@ -568,6 +596,10 @@ abstract class HttpClientTest extends InstrumentationSpecification { 0 } + boolean testWithClientParent() { + true + } + boolean testRedirects() { true } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/utils/TraceUtils.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/utils/TraceUtils.groovy index 80436c77fc..e79e5aaa86 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/utils/TraceUtils.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/utils/TraceUtils.groovy @@ -12,6 +12,7 @@ import io.opentelemetry.api.trace.Span import io.opentelemetry.api.trace.SpanKind import io.opentelemetry.api.trace.StatusCode import io.opentelemetry.api.trace.Tracer +import io.opentelemetry.extension.annotations.WithSpan import io.opentelemetry.instrumentation.test.asserts.AttributesAssert import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.instrumentation.test.server.ServerTraceUtils @@ -53,6 +54,11 @@ class TraceUtils { tracer.spanBuilder(spanName).startSpan().end() } + @WithSpan(value = "parent-client-span", kind = SpanKind.CLIENT) + static T runUnderParentClientSpan(Callable r) { + r.call() + } + static basicSpan(TraceAssert trace, int index, String operation, Object parentSpan = null, Throwable exception = null, @ClosureParams(value = SimpleType, options = ['io.opentelemetry.instrumentation.test.asserts.AttributesAssert']) @DelegatesTo(value = AttributesAssert, strategy = Closure.DELEGATE_FIRST) Closure additionAttributesAssert = null) {