Skip to content

Commit 7bb0c6b

Browse files
lauritlaurit
authored
Test whether http client span ends after headers or body is received (#12149)
1 parent d9f7030 commit 7bb0c6b

File tree

23 files changed

+163
-42
lines changed

23 files changed

+163
-42
lines changed

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

+13-3
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest;
1010
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
1111
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
12+
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
1213
import java.io.IOException;
1314
import java.io.UncheckedIOException;
1415
import java.net.URI;
@@ -69,7 +70,7 @@ HttpAsyncClient getClient(URI uri) {
6970
}
7071

7172
@Nested
72-
class ApacheClientUriRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
73+
class ApacheClientUriRequestTest extends AbstractTest {
7374

7475
@Override
7576
public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -95,7 +96,7 @@ public void sendRequestWithCallback(
9596
}
9697

9798
@Nested
98-
class ApacheClientHostRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
99+
class ApacheClientHostRequestTest extends AbstractTest {
99100

100101
@Override
101102
public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -129,7 +130,7 @@ public void sendRequestWithCallback(
129130
}
130131

131132
@Nested
132-
class ApacheClientHostAbsoluteUriRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
133+
class ApacheClientHostAbsoluteUriRequestTest extends AbstractTest {
133134

134135
@Override
135136
public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -161,6 +162,15 @@ public void sendRequestWithCallback(
161162
}
162163
}
163164

165+
abstract static class AbstractTest extends AbstractHttpClientTest<HttpUriRequest> {
166+
167+
@Override
168+
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
169+
super.configure(optionsBuilder);
170+
optionsBuilder.spanEndsAfterBody();
171+
}
172+
}
173+
164174
HttpUriRequest configureRequest(HttpUriRequest request, Map<String, String> headers) {
165175
request.addHeader("user-agent", "httpasyncclient");
166176
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
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
99
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
1010
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
11+
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
1112
import java.net.URI;
1213
import java.util.Map;
1314
import java.util.concurrent.CancellationException;
@@ -86,6 +87,13 @@ protected HttpContext getContext() {
8687
}
8788

8889
abstract class AbstractTest extends AbstractApacheHttpClientTest<SimpleHttpRequest> {
90+
91+
@Override
92+
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
93+
super.configure(optionsBuilder);
94+
optionsBuilder.spanEndsAfterBody();
95+
}
96+
8997
@Override
9098
public SimpleHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
9199
SimpleHttpRequest httpRequest = super.buildRequest(method, uri, headers);

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

+1
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
111111
optionsBuilder.disableTestReusedRequest();
112112
// can only use methods from HttpMethod enum
113113
optionsBuilder.disableTestNonStandardHttpMethod();
114+
optionsBuilder.spanEndsAfterBody();
114115
// armeria uses http/2
115116
optionsBuilder.setHttpProtocolVersion(
116117
uri -> {

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

+1
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ public void onThrowable(Throwable throwable) {
9595

9696
@Override
9797
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
98+
optionsBuilder.spanEndsAfterBody();
9899
optionsBuilder.disableTestRedirects();
99100

100101
// 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

+1
Original file line numberDiff line numberDiff line change
@@ -96,5 +96,6 @@ public void onThrowable(Throwable throwable) {
9696
@Override
9797
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
9898
optionsBuilder.disableTestRedirects();
99+
optionsBuilder.spanEndsAfterBody();
99100
}
100101
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
120120
optionsBuilder.setHttpAttributes(ClientTest::getHttpAttributes);
121121
optionsBuilder.setExpectedClientSpanNameMapper(ClientTest::getExpectedClientSpanName);
122122
optionsBuilder.disableTestRedirects();
123+
optionsBuilder.spanEndsAfterBody();
123124
optionsBuilder.setClientSpanErrorMapper(
124125
(uri, error) -> {
125126
// 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

+4-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,10 @@ public int sendRequest(HttpRequest request, String method, URI uri, Map<String,
8383
throws Exception {
8484
HttpResponse response = sendRequest(request);
8585
// read request body to avoid broken pipe errors on the server side
86-
response.parseAsString();
86+
// skip reading body of long-request to make the test a bit faster
87+
if (!uri.toString().contains("/long-request")) {
88+
response.parseAsString();
89+
}
8790
return response.getStatusCode();
8891
}
8992

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ public int sendRequest(
4545
Span parentSpan = Span.current();
4646
InputStream stream = connection.getInputStream();
4747
assertThat(Span.current()).isEqualTo(parentSpan);
48-
readLines(stream);
48+
// skip reading body of long-request to make the test a bit faster
49+
if (!uri.toString().contains("/long-request")) {
50+
readLines(stream);
51+
}
4952
stream.close();
5053
return connection.getResponseCode();
5154
} finally {

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

+1
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
8484
// TODO nested client span is not created, but context is still injected
8585
// which is not what the test expects
8686
optionsBuilder.disableTestWithClientParent();
87+
optionsBuilder.spanEndsAfterBody();
8788

8889
optionsBuilder.setHttpAttributes(
8990
uri -> {

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

+1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
5656
// jetty 12 does not support to reuse request
5757
// use request.send() twice will block the program infinitely
5858
optionsBuilder.disableTestReusedRequest();
59+
optionsBuilder.spanEndsAfterBody();
5960
}
6061

6162
@Override

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

+1
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ public void after() throws Exception {
4949
@Override
5050
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
5151
optionsBuilder.disableTestRedirects();
52+
optionsBuilder.spanEndsAfterBody();
5253
}
5354

5455
@Override

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

+1
Original file line numberDiff line numberDiff line change
@@ -52,5 +52,6 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
5252
optionsBuilder.disableTestCallback();
5353
// Circular Redirects are not explicitly handled by jodd-http
5454
optionsBuilder.disableTestCircularRedirects();
55+
optionsBuilder.spanEndsAfterBody();
5556
}
5657
}

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

+1-26
Original file line numberDiff line numberDiff line change
@@ -10,21 +10,15 @@ import io.ktor.client.engine.cio.*
1010
import io.ktor.client.plugins.*
1111
import io.ktor.client.request.*
1212
import io.ktor.http.*
13-
import io.opentelemetry.api.trace.SpanKind
1413
import io.opentelemetry.context.Context
1514
import io.opentelemetry.extension.kotlin.asContextElement
1615
import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest
1716
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult
1817
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions
1918
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES
20-
import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat
21-
import io.opentelemetry.sdk.testing.assertj.TraceAssert
22-
import io.opentelemetry.sdk.trace.data.StatusData
2319
import io.opentelemetry.semconv.NetworkAttributes
2420
import kotlinx.coroutines.*
25-
import org.junit.jupiter.api.Test
2621
import java.net.URI
27-
import java.util.function.Consumer
2822

2923
abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBuilder>() {
3024

@@ -69,6 +63,7 @@ abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBu
6963
// this instrumentation creates a span per each physical request
7064
// related issue https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/5722
7165
disableTestRedirects()
66+
spanEndsAfterBody()
7267

7368
setHttpAttributes { DEFAULT_HTTP_ATTRIBUTES - setOf(NetworkAttributes.NETWORK_PROTOCOL_VERSION) }
7469

@@ -77,24 +72,4 @@ abstract class AbstractKtorHttpClientTest : AbstractHttpClientTest<HttpRequestBu
7772
}
7873
}
7974
}
80-
81-
@Test
82-
fun checkSpanEndsAfterBodyReceived() {
83-
val method = "GET"
84-
val path = "/long-request"
85-
val uri = resolveAddress(path)
86-
val responseCode = doRequest(method, uri)
87-
88-
assertThat(responseCode).isEqualTo(200)
89-
90-
testing.waitAndAssertTraces(
91-
Consumer { trace: TraceAssert ->
92-
val span = trace.getSpan(0)
93-
assertThat(span).hasKind(SpanKind.CLIENT).hasStatus(StatusData.unset())
94-
assertThat(span.endEpochNanos - span.startEpochNanos >= 1_000_000_000)
95-
.describedAs("Span duration should be at least 1000ms")
96-
.isTrue()
97-
}
98-
)
99-
}
10075
}

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

+5-7
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.util.concurrent.CompletableFuture;
3737
import java.util.concurrent.TimeUnit;
3838
import org.jetbrains.annotations.NotNull;
39+
import org.junit.jupiter.api.AfterAll;
3940
import org.junit.jupiter.api.extension.RegisterExtension;
4041

4142
class Netty40ClientTest extends AbstractHttpClientTest<DefaultFullHttpRequest> {
@@ -44,15 +45,12 @@ class Netty40ClientTest extends AbstractHttpClientTest<DefaultFullHttpRequest> {
4445
static final InstrumentationExtension testing = HttpClientInstrumentationExtension.forAgent();
4546

4647
private final EventLoopGroup eventLoopGroup = new NioEventLoopGroup();
47-
4848
private final Bootstrap bootstrap = buildBootstrap(false);
49-
5049
private final Bootstrap readTimeoutBootstrap = buildBootstrap(true);
5150

52-
void cleanupSpec() {
53-
if (eventLoopGroup != null) {
54-
eventLoopGroup.shutdownGracefully();
55-
}
51+
@AfterAll
52+
void cleanup() {
53+
eventLoopGroup.shutdownGracefully();
5654
}
5755

5856
Bootstrap buildBootstrap(boolean readTimeout) {
@@ -137,7 +135,7 @@ public void sendRequestWithCallback(
137135
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
138136
optionsBuilder.disableTestRedirects();
139137
optionsBuilder.disableTestHttps();
140-
optionsBuilder.disableTestReadTimeout();
138+
optionsBuilder.spanEndsAfterBody();
141139

142140
optionsBuilder.setExpectedClientSpanNameMapper(Netty40ClientTest::expectedClientSpanName);
143141
optionsBuilder.setHttpAttributes(Netty40ClientTest::httpAttributes);

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

+1
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
112112
this::configureChannel));
113113

114114
optionsBuilder.disableTestRedirects();
115+
optionsBuilder.spanEndsAfterBody();
115116
}
116117

117118
private static Set<AttributeKey<?>> getHttpAttributes(URI uri) {

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,10 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
148148
optionsBuilder.setHttpAttributes(this::computeHttpAttributes);
149149

150150
optionsBuilder.disableTestRedirects();
151-
152151
// these tests will pass, but they don't really test anything since REQUEST is Void
153152
optionsBuilder.disableTestReusedRequest();
153+
154+
optionsBuilder.spanEndsAfterBody();
154155
}
155156

156157
protected Set<AttributeKey<?>> computeHttpAttributes(URI uri) {

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

+2
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,8 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
152152
optionsBuilder.disableTestReusedRequest();
153153

154154
optionsBuilder.setHttpAttributes(this::getHttpAttributes);
155+
156+
optionsBuilder.spanEndsAfterBody();
155157
}
156158

157159
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

+1
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ public void sendRequestWithCallback(
9090
@Override
9191
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
9292
optionsBuilder.disableTestRedirects();
93+
optionsBuilder.spanEndsAfterBody();
9394

9495
optionsBuilder.setExpectedClientSpanNameMapper(
9596
(uri, method) -> {

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

+1
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ public void sendRequestWithCallback(
109109
protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
110110
optionsBuilder.markAsLowLevelInstrumentation();
111111
optionsBuilder.setMaxRedirects(52);
112+
optionsBuilder.spanEndsAfterBody();
112113

113114
// TODO: remove this test altogether? this scenario is (was) only implemented in reactor-netty,
114115
// all other HTTP clients worked in a different way

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

+4
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,10 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
354354
junitTest.highConcurrencyOnSingleConnection()
355355
}
356356
357+
def "http client span ends after headers are received"() {
358+
junitTest.spanEndsAfterHeadersReceived()
359+
}
360+
357361
// ideally private, but then groovy closures in this class cannot find them
358362
final int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
359363
def request = buildRequest(method, uri, headers)

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

+71-1
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import java.util.concurrent.ExecutionException;
4444
import java.util.concurrent.ExecutorService;
4545
import java.util.concurrent.Executors;
46+
import java.util.concurrent.TimeUnit;
4647
import java.util.concurrent.atomic.AtomicReference;
4748
import java.util.function.Consumer;
4849
import java.util.stream.Collectors;
@@ -958,6 +959,75 @@ void highConcurrencyOnSingleConnection() {
958959
pool.shutdown();
959960
}
960961

962+
@Test
963+
void spanEndsAfterBodyReceived() throws Exception {
964+
assumeTrue(options.isSpanEndsAfterBody());
965+
966+
String method = "GET";
967+
URI uri = resolveAddress("/long-request");
968+
969+
int responseCode =
970+
doRequest(
971+
method,
972+
uri,
973+
// the time that server waits before completing the response
974+
Collections.singletonMap("delay", String.valueOf(TimeUnit.SECONDS.toMillis(1))));
975+
976+
assertThat(responseCode).isEqualTo(200);
977+
978+
testing.waitAndAssertTraces(
979+
trace -> {
980+
trace.hasSpansSatisfyingExactly(
981+
span ->
982+
assertClientSpan(span, uri, method, 200, null)
983+
.hasNoParent()
984+
.hasStatus(StatusData.unset()),
985+
span -> assertServerSpan(span).hasParent(trace.getSpan(0)));
986+
SpanData span = trace.getSpan(0);
987+
// make sure the span is at least as long as the delay we set when sending the request
988+
assertThat(
989+
span.getEndEpochNanos() - span.getStartEpochNanos()
990+
>= TimeUnit.SECONDS.toNanos(1))
991+
.describedAs("Span duration should be at least 1s")
992+
.isTrue();
993+
});
994+
}
995+
996+
@Test
997+
void spanEndsAfterHeadersReceived() throws Exception {
998+
assumeTrue(options.isSpanEndsAfterHeaders());
999+
1000+
String method = "GET";
1001+
URI uri = resolveAddress("/long-request");
1002+
1003+
int responseCode =
1004+
doRequest(
1005+
method,
1006+
uri,
1007+
// the time that server waits before completing the response, we expect the response
1008+
// headers to arrive much sooner
1009+
Collections.singletonMap("delay", String.valueOf(TimeUnit.SECONDS.toMillis(2))));
1010+
1011+
assertThat(responseCode).isEqualTo(200);
1012+
1013+
testing.waitAndAssertTraces(
1014+
trace -> {
1015+
trace.hasSpansSatisfyingExactly(
1016+
span ->
1017+
assertClientSpan(span, uri, method, 200, null)
1018+
.hasNoParent()
1019+
.hasStatus(StatusData.unset()),
1020+
span -> assertServerSpan(span).hasParent(trace.getSpan(0)));
1021+
SpanData span = trace.getSpan(0);
1022+
// verify that the span length is less than the delay used to complete the response body
1023+
assertThat(
1024+
span.getEndEpochNanos() - span.getStartEpochNanos()
1025+
<= TimeUnit.SECONDS.toNanos(2))
1026+
.describedAs("Span duration should be less than 2s")
1027+
.isTrue();
1028+
});
1029+
}
1030+
9611031
// Visible for spock bridge.
9621032
SpanDataAssert assertClientSpan(
9631033
SpanDataAssert span,
@@ -1053,7 +1123,7 @@ static SpanDataAssert assertServerSpan(SpanDataAssert span) {
10531123
return span.hasName("test-http-server").hasKind(SpanKind.SERVER);
10541124
}
10551125

1056-
protected int doRequest(String method, URI uri) throws Exception {
1126+
private int doRequest(String method, URI uri) throws Exception {
10571127
return doRequest(method, uri, Collections.emptyMap());
10581128
}
10591129

0 commit comments

Comments
 (0)