Test whether http client span ends after headers or body is received

instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTest.java

@@ -9,6 +9,7 @@
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.net.URI;
@@ -69,7 +70,7 @@ HttpAsyncClient getClient(URI uri) {
   }
 
   @Nested
-  class ApacheClientUriRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
+  class ApacheClientUriRequestTest extends AbstractTest {
 
     @Override
     public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -95,7 +96,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientHostRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
+  class ApacheClientHostRequestTest extends AbstractTest {
 
     @Override
     public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -129,7 +130,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientHostAbsoluteUriRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
+  class ApacheClientHostAbsoluteUriRequestTest extends AbstractTest {
 
     @Override
     public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -161,6 +162,15 @@ public void sendRequestWithCallback(
     }
   }
 
+  abstract static class AbstractTest extends AbstractHttpClientTest<HttpUriRequest> {
+
+    @Override
+    protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
+      super.configure(optionsBuilder);
+      optionsBuilder.spanEndsAfterBody();
+    }
+  }
+
   HttpUriRequest configureRequest(HttpUriRequest request, Map<String, String> headers) {
     request.addHeader("user-agent", "httpasyncclient");
     headers.forEach((key, value) -> request.setHeader(new BasicHeader(key, value)));

instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpAsyncClientTest.java

@@ -8,6 +8,7 @@
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
 import java.net.URI;
 import java.util.Map;
 import java.util.concurrent.CancellationException;
@@ -86,6 +87,13 @@ protected HttpContext getContext() {
   }
 
   abstract class AbstractTest extends AbstractApacheHttpClientTest<SimpleHttpRequest> {
+
+    @Override
+    protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
+      super.configure(optionsBuilder);
+      optionsBuilder.spanEndsAfterBody();
+    }
+
     @Override
     public SimpleHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
       SimpleHttpRequest httpRequest = super.buildRequest(method, uri, headers);

instrumentation/armeria/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java

@@ -111,6 +111,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestReusedRequest();
     // can only use methods from HttpMethod enum
     optionsBuilder.disableTestNonStandardHttpMethod();
+    optionsBuilder.spanEndsAfterBody();
     // armeria uses http/2
     optionsBuilder.setHttpProtocolVersion(
         uri -> {

instrumentation/async-http-client/async-http-client-1.9/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/asynchttpclient/v1_9/AsyncHttpClientTest.java

@@ -95,6 +95,7 @@ public void onThrowable(Throwable throwable) {
 
   @Override
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
+    optionsBuilder.spanEndsAfterBody();
     optionsBuilder.disableTestRedirects();
 
     // disable read timeout test for non latest because it is flaky with 1.9.0

instrumentation/async-http-client/async-http-client-2.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/asynchttpclient/v2_0/AsyncHttpClientTest.java

@@ -96,5 +96,6 @@ public void onThrowable(Throwable throwable) {
   @Override
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestRedirects();
+    optionsBuilder.spanEndsAfterBody();
   }
 }

instrumentation/finagle-http-23.11/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/finaglehttp/v23_11/ClientTest.java

@@ -120,6 +120,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.setHttpAttributes(ClientTest::getHttpAttributes);
     optionsBuilder.setExpectedClientSpanNameMapper(ClientTest::getExpectedClientSpanName);
     optionsBuilder.disableTestRedirects();
+    optionsBuilder.spanEndsAfterBody();
     optionsBuilder.setClientSpanErrorMapper(
         (uri, error) -> {
           // all errors should be wrapped in RuntimeExceptions due to how we run things in

instrumentation/google-http-client-1.19/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/googlehttpclient/AbstractGoogleHttpClientTest.java

@@ -83,7 +83,10 @@ public int sendRequest(HttpRequest request, String method, URI uri, Map<String,
       throws Exception {
     HttpResponse response = sendRequest(request);
     // read request body to avoid broken pipe errors on the server side
-    response.parseAsString();
+    // skip reading body of long-request to make the test a bit faster
+    if (!uri.toString().contains("/long-request")) {
+      response.parseAsString();
+    }
     return response.getStatusCode();
   }
 

instrumentation/http-url-connection/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionUseCachesFalseTest.java

@@ -45,7 +45,10 @@ public int sendRequest(
       Span parentSpan = Span.current();
       InputStream stream = connection.getInputStream();
       assertThat(Span.current()).isEqualTo(parentSpan);
-      readLines(stream);
+      // skip reading body of long-request to make the test a bit faster
+      if (!uri.toString().contains("/long-request")) {
+        readLines(stream);
+      }
       stream.close();
       return connection.getResponseCode();
     } finally {

instrumentation/java-http-client/testing/src/main/java/io/opentelemetry/instrumentation/httpclient/AbstractJavaHttpClientTest.java

@@ -84,6 +84,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     // TODO nested client span is not created, but context is still injected
     //  which is not what the test expects
     optionsBuilder.disableTestWithClientParent();
+    optionsBuilder.spanEndsAfterBody();
 
     optionsBuilder.setHttpAttributes(
         uri -> {

instrumentation/jetty-httpclient/jetty-httpclient-12.0/testing/src/main/java/io/opentelemetry/instrumentation/jetty/httpclient/v12_0/AbstractJettyClient12Test.java

@@ -56,6 +56,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     // jetty 12 does not support to reuse request
     // use request.send() twice will block the program infinitely
     optionsBuilder.disableTestReusedRequest();
+    optionsBuilder.spanEndsAfterBody();
   }
 
   @Override

instrumentation/jetty-httpclient/jetty-httpclient-9.2/testing/src/main/java/io/opentelemetry/instrumentation/jetty/httpclient/v9_2/AbstractJettyClient9Test.java

@@ -49,6 +49,7 @@ public void after() throws Exception {
   @Override
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestRedirects();
+    optionsBuilder.spanEndsAfterBody();
   }
 
   @Override

instrumentation/jodd-http-4.2/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/joddhttp/v4_2/JoddHttpTest.java

@@ -52,5 +52,6 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestCallback();
     // Circular Redirects are not explicitly handled by jodd-http
     optionsBuilder.disableTestCircularRedirects();
+    optionsBuilder.spanEndsAfterBody();
   }
 }

instrumentation/ktor/ktor-2.0/testing/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v2_0/client/AbstractKtorHttpClientTest.kt

@@ -10,21 +10,15 @@ import io.ktor.client.engine.cio.*
 import io.ktor.client.plugins.*
 import io.ktor.client.request.*
 import io.ktor.http.*
-import io.opentelemetry.api.trace.SpanKind
 import io.opentelemetry.context.Context
 import io.opentelemetry.extension.kotlin.asContextElement
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES
-import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat
-import io.opentelemetry.sdk.testing.assertj.TraceAssert
-import io.opentelemetry.sdk.trace.data.StatusData
 import io.opentelemetry.semconv.NetworkAttributes
 import kotlinx.coroutines.*
-import org.junit.jupiter.api.Test
 import java.net.URI
-import java.util.function.Consumer
 
 abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBuilder>() {
 
@@ -69,6 +63,7 @@ abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBu
       // this instrumentation creates a span per each physical request
       // related issue https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/5722
       disableTestRedirects()
+      spanEndsAfterBody()
 
       setHttpAttributes { DEFAULT_HTTP_ATTRIBUTES - setOf(NetworkAttributes.NETWORK_PROTOCOL_VERSION) }
 
@@ -77,24 +72,4 @@ abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBu
       }
     }
   }
-
-  @Test
-  fun checkSpanEndsAfterBodyReceived() {
-    val method = "GET"
-    val path = "/long-request"
-    val uri = resolveAddress(path)
-    val responseCode = doRequest(method, uri)
-
-    assertThat(responseCode).isEqualTo(200)
-
-    testing.waitAndAssertTraces(
-      Consumer { trace: TraceAssert ->
-        val span = trace.getSpan(0)
-        assertThat(span).hasKind(SpanKind.CLIENT).hasStatus(StatusData.unset())
-        assertThat(span.endEpochNanos - span.startEpochNanos >= 1_000_000_000)
-          .describedAs("Span duration should be at least 1000ms")
-          .isTrue()
-      }
-    )
-  }
 }

instrumentation/netty/netty-4.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/Netty40ClientTest.java

@@ -36,6 +36,7 @@
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeUnit;
 import org.jetbrains.annotations.NotNull;
+import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 class Netty40ClientTest extends AbstractHttpClientTest<DefaultFullHttpRequest> {
@@ -44,15 +45,12 @@ class Netty40ClientTest extends AbstractHttpClientTest<DefaultFullHttpRequest> {
   static final InstrumentationExtension testing = HttpClientInstrumentationExtension.forAgent();
 
   private final EventLoopGroup eventLoopGroup = new NioEventLoopGroup();
-
   private final Bootstrap bootstrap = buildBootstrap(false);
-
   private final Bootstrap readTimeoutBootstrap = buildBootstrap(true);
 
-  void cleanupSpec() {
-    if (eventLoopGroup != null) {
-      eventLoopGroup.shutdownGracefully();
-    }
+  @AfterAll
+  void cleanup() {
+    eventLoopGroup.shutdownGracefully();
   }
 
   Bootstrap buildBootstrap(boolean readTimeout) {
@@ -137,7 +135,7 @@ public void sendRequestWithCallback(
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestRedirects();
     optionsBuilder.disableTestHttps();
-    optionsBuilder.disableTestReadTimeout();
+    optionsBuilder.spanEndsAfterBody();
 
     optionsBuilder.setExpectedClientSpanNameMapper(Netty40ClientTest::expectedClientSpanName);
     optionsBuilder.setHttpAttributes(Netty40ClientTest::httpAttributes);

instrumentation/netty/netty-4.1/testing/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/AbstractNetty41ClientTest.java

@@ -112,6 +112,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
                 this::configureChannel));
 
     optionsBuilder.disableTestRedirects();
+    optionsBuilder.spanEndsAfterBody();
   }
 
   private static Set<AttributeKey<?>> getHttpAttributes(URI uri) {

instrumentation/ratpack/ratpack-1.4/testing/src/main/java/io/opentelemetry/instrumentation/ratpack/client/AbstractRatpackHttpClientTest.java

@@ -148,9 +148,10 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.setHttpAttributes(this::computeHttpAttributes);
 
     optionsBuilder.disableTestRedirects();
-
     // these tests will pass, but they don't really test anything since REQUEST is Void
     optionsBuilder.disableTestReusedRequest();
+
+    optionsBuilder.spanEndsAfterBody();
   }
 
   protected Set<AttributeKey<?>> computeHttpAttributes(URI uri) {

instrumentation/ratpack/ratpack-1.7/library/src/test/java/io/opentelemetry/instrumentation/ratpack/v1_7/AbstractRatpackHttpClientTest.java

@@ -152,6 +152,8 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestReusedRequest();
 
     optionsBuilder.setHttpAttributes(this::getHttpAttributes);
+
+    optionsBuilder.spanEndsAfterBody();
   }
 
   protected Set<AttributeKey<?>> getHttpAttributes(URI uri) {

instrumentation/reactor/reactor-netty/reactor-netty-0.9/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v0_9/AbstractReactorNettyHttpClientTest.java

@@ -90,6 +90,7 @@ public void sendRequestWithCallback(
   @Override
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.disableTestRedirects();
+    optionsBuilder.spanEndsAfterBody();
 
     optionsBuilder.setExpectedClientSpanNameMapper(
         (uri, method) -> {

instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/AbstractReactorNettyHttpClientTest.java

@@ -109,6 +109,7 @@ public void sendRequestWithCallback(
   protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.markAsLowLevelInstrumentation();
     optionsBuilder.setMaxRedirects(52);
+    optionsBuilder.spanEndsAfterBody();
 
     // TODO: remove this test altogether? this scenario is (was) only implemented in reactor-netty,
     // all other HTTP clients worked in a different way

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy

@@ -354,6 +354,10 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
     junitTest.highConcurrencyOnSingleConnection()
   }
 
+  def "http client span ends after headers are received"() {
+    junitTest.spanEndsAfterHeadersReceived()
+  }
+
   // ideally private, but then groovy closures in this class cannot find them
   final int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
     def request = buildRequest(method, uri, headers)

testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java

@@ -43,6 +43,7 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
@@ -958,6 +959,75 @@ void highConcurrencyOnSingleConnection() {
     pool.shutdown();
   }
 
+  @Test
+  void spanEndsAfterBodyReceived() throws Exception {
+    assumeTrue(options.isSpanEndsAfterBody());
+
+    String method = "GET";
+    URI uri = resolveAddress("/long-request");
+
+    int responseCode =
+        doRequest(
+            method,
+            uri,
+            // the time that server waits before completing the response
+            Collections.singletonMap("delay", String.valueOf(TimeUnit.SECONDS.toMillis(1))));
+
+    assertThat(responseCode).isEqualTo(200);
+
+    testing.waitAndAssertTraces(
+        trace -> {
+          trace.hasSpansSatisfyingExactly(
+              span ->
+                  assertClientSpan(span, uri, method, 200, null)
+                      .hasNoParent()
+                      .hasStatus(StatusData.unset()),
+              span -> assertServerSpan(span).hasParent(trace.getSpan(0)));
+          SpanData span = trace.getSpan(0);
+          // make sure the span is at least as long as the delay we set when sending the request
+          assertThat(
+                  span.getEndEpochNanos() - span.getStartEpochNanos()
+                      >= TimeUnit.SECONDS.toNanos(1))
+              .describedAs("Span duration should be at least 1s")
+              .isTrue();
+        });
+  }
+
+  @Test
+  void spanEndsAfterHeadersReceived() throws Exception {
+    assumeTrue(options.isSpanEndsAfterHeaders());
+
+    String method = "GET";
+    URI uri = resolveAddress("/long-request");
+
+    int responseCode =
+        doRequest(
+            method,
+            uri,
+            // the time that server waits before completing the response, we expect the response
+            // headers to arrive much sooner
+            Collections.singletonMap("delay", String.valueOf(TimeUnit.SECONDS.toMillis(2))));
+
+    assertThat(responseCode).isEqualTo(200);
+
+    testing.waitAndAssertTraces(
+        trace -> {
+          trace.hasSpansSatisfyingExactly(
+              span ->
+                  assertClientSpan(span, uri, method, 200, null)
+                      .hasNoParent()
+                      .hasStatus(StatusData.unset()),
+              span -> assertServerSpan(span).hasParent(trace.getSpan(0)));
+          SpanData span = trace.getSpan(0);
+          // verify that the span length is less than the delay used to complete the response body
+          assertThat(
+                  span.getEndEpochNanos() - span.getStartEpochNanos()
+                      <= TimeUnit.SECONDS.toNanos(2))
+              .describedAs("Span duration should be less than 2s")
+              .isTrue();
+        });
+  }
+
   // Visible for spock bridge.
   SpanDataAssert assertClientSpan(
       SpanDataAssert span,
@@ -1053,7 +1123,7 @@ static SpanDataAssert assertServerSpan(SpanDataAssert span) {
     return span.hasName("test-http-server").hasKind(SpanKind.SERVER);
   }
 
-  protected int doRequest(String method, URI uri) throws Exception {
+  private int doRequest(String method, URI uri) throws Exception {
     return doRequest(method, uri, Collections.emptyMap());
   }