Skip to content

Commit f2ed832

Browse files
lauritlaurit
authored
Fix ending server span with servlet async request (open-telemetry#13830)
1 parent 2e5c12b commit f2ed832

File tree

10 files changed

+214
-4
lines changed

10 files changed

+214
-4
lines changed

‎instrumentation/servlet/servlet-3.0/testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/TestServlet3.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static io.opentelemetry.javaagent.instrumentation.servlet.v3_0.AbstractServlet3Test.HTML_PRINT_WRITER;
1818
import static io.opentelemetry.javaagent.instrumentation.servlet.v3_0.AbstractServlet3Test.HTML_SERVLET_OUTPUT_STREAM;
1919

20+
import io.opentelemetry.instrumentation.testing.GlobalTraceUtil;
2021
import io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint;
2122
import java.io.IOException;
2223
import java.io.PrintWriter;
@@ -100,7 +101,12 @@ public static class Async extends HttpServlet {
100101
protected void service(HttpServletRequest req, HttpServletResponse resp) {
101102
ServerEndpoint endpoint = ServerEndpoint.forPath(req.getServletPath());
102103
CountDownLatch latch = new CountDownLatch(1);
103-
AsyncContext context = req.startAsync();
104+
boolean startAsyncInSpan =
105+
SUCCESS.equals(endpoint) && "true".equals(req.getParameter("startAsyncInSpan"));
106+
AsyncContext context =
107+
startAsyncInSpan
108+
? GlobalTraceUtil.runWithSpan("startAsync", () -> req.startAsync())
109+
: req.startAsync();
104110
if (endpoint.equals(EXCEPTION)) {
105111
context.setTimeout(5000);
106112
}

‎instrumentation/servlet/servlet-3.0/testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/jetty/JettyServlet3AsyncTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55

66
package io.opentelemetry.javaagent.instrumentation.servlet.v3_0.jetty;
77

8+
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.trace.SpanKind;
812
import io.opentelemetry.javaagent.instrumentation.servlet.v3_0.TestServlet3;
13+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest;
14+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
15+
import io.opentelemetry.testing.internal.armeria.common.HttpMethod;
916
import javax.servlet.Servlet;
17+
import org.junit.jupiter.api.Test;
1018

1119
class JettyServlet3AsyncTest extends JettyServlet3Test {
1220

@@ -24,4 +32,32 @@ public boolean errorEndpointUsesSendError() {
2432
public boolean isAsyncTest() {
2533
return true;
2634
}
35+
36+
@Test
37+
void startAsyncInSpan() {
38+
AggregatedHttpRequest request =
39+
AggregatedHttpRequest.of(
40+
HttpMethod.GET, resolveAddress(SUCCESS, "h1c://") + "?startAsyncInSpan=true");
41+
AggregatedHttpResponse response = client.execute(request).aggregate().join();
42+
43+
assertThat(response.status().code()).isEqualTo(SUCCESS.getStatus());
44+
assertThat(response.contentUtf8()).isEqualTo(SUCCESS.getBody());
45+
46+
testing()
47+
.waitAndAssertTraces(
48+
trace ->
49+
trace.hasSpansSatisfyingExactly(
50+
span ->
51+
span.hasName("GET " + getContextPath() + "/success")
52+
.hasKind(SpanKind.SERVER)
53+
.hasNoParent(),
54+
span ->
55+
span.hasName("startAsync")
56+
.hasKind(SpanKind.INTERNAL)
57+
.hasParent(trace.getSpan(0)),
58+
span ->
59+
span.hasName("controller")
60+
.hasKind(SpanKind.INTERNAL)
61+
.hasParent(trace.getSpan(0))));
62+
}
2763
}

‎instrumentation/servlet/servlet-3.0/testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/tomcat/TomcatServlet3AsyncTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55

66
package io.opentelemetry.javaagent.instrumentation.servlet.v3_0.tomcat;
77

8+
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.trace.SpanKind;
812
import io.opentelemetry.javaagent.instrumentation.servlet.v3_0.TestServlet3;
13+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest;
14+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
15+
import io.opentelemetry.testing.internal.armeria.common.HttpMethod;
916
import javax.servlet.Servlet;
17+
import org.junit.jupiter.api.Test;
1018

1119
class TomcatServlet3AsyncTest extends TomcatServlet3Test {
1220

@@ -19,4 +27,32 @@ public Class<? extends Servlet> servlet() {
1927
public boolean errorEndpointUsesSendError() {
2028
return false;
2129
}
30+
31+
@Test
32+
void startAsyncInSpan() {
33+
AggregatedHttpRequest request =
34+
AggregatedHttpRequest.of(
35+
HttpMethod.GET, resolveAddress(SUCCESS, "h1c://") + "?startAsyncInSpan=true");
36+
AggregatedHttpResponse response = client.execute(request).aggregate().join();
37+
38+
assertThat(response.status().code()).isEqualTo(SUCCESS.getStatus());
39+
assertThat(response.contentUtf8()).isEqualTo(SUCCESS.getBody());
40+
41+
testing()
42+
.waitAndAssertTraces(
43+
trace ->
44+
trace.hasSpansSatisfyingExactly(
45+
span ->
46+
span.hasName("GET " + getContextPath() + "/success")
47+
.hasKind(SpanKind.SERVER)
48+
.hasNoParent(),
49+
span ->
50+
span.hasName("startAsync")
51+
.hasKind(SpanKind.INTERNAL)
52+
.hasParent(trace.getSpan(0)),
53+
span ->
54+
span.hasName("controller")
55+
.hasKind(SpanKind.INTERNAL)
56+
.hasParent(trace.getSpan(0))));
57+
}
2258
}

‎instrumentation/servlet/servlet-5.0/jetty11-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/jetty/JettyServlet5AsyncTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55

66
package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.jetty;
77

8+
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.trace.SpanKind;
812
import io.opentelemetry.javaagent.instrumentation.servlet.v5_0.TestServlet5;
13+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest;
14+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
15+
import io.opentelemetry.testing.internal.armeria.common.HttpMethod;
916
import jakarta.servlet.Servlet;
17+
import org.junit.jupiter.api.Test;
1018

1119
class JettyServlet5AsyncTest extends JettyServlet5Test {
1220

@@ -19,4 +27,32 @@ public Class<? extends Servlet> servlet() {
1927
public boolean errorEndpointUsesSendError() {
2028
return false;
2129
}
30+
31+
@Test
32+
void startAsyncInSpan() {
33+
AggregatedHttpRequest request =
34+
AggregatedHttpRequest.of(
35+
HttpMethod.GET, resolveAddress(SUCCESS, "h1c://") + "?startAsyncInSpan=true");
36+
AggregatedHttpResponse response = client.execute(request).aggregate().join();
37+
38+
assertThat(response.status().code()).isEqualTo(SUCCESS.getStatus());
39+
assertThat(response.contentUtf8()).isEqualTo(SUCCESS.getBody());
40+
41+
testing()
42+
.waitAndAssertTraces(
43+
trace ->
44+
trace.hasSpansSatisfyingExactly(
45+
span ->
46+
span.hasName("GET " + getContextPath() + "/success")
47+
.hasKind(SpanKind.SERVER)
48+
.hasNoParent(),
49+
span ->
50+
span.hasName("startAsync")
51+
.hasKind(SpanKind.INTERNAL)
52+
.hasParent(trace.getSpan(0)),
53+
span ->
54+
span.hasName("controller")
55+
.hasKind(SpanKind.INTERNAL)
56+
.hasParent(trace.getSpan(0))));
57+
}
2258
}

‎instrumentation/servlet/servlet-5.0/jetty12-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/jetty12/Jetty12Servlet5AsyncTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55

66
package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.jetty12;
77

8+
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.trace.SpanKind;
812
import io.opentelemetry.javaagent.instrumentation.servlet.v5_0.TestServlet5;
13+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest;
14+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
15+
import io.opentelemetry.testing.internal.armeria.common.HttpMethod;
916
import jakarta.servlet.Servlet;
17+
import org.junit.jupiter.api.Test;
1018

1119
class Jetty12Servlet5AsyncTest extends Jetty12Servlet5Test {
1220

@@ -19,4 +27,32 @@ public Class<? extends Servlet> servlet() {
1927
public boolean errorEndpointUsesSendError() {
2028
return false;
2129
}
30+
31+
@Test
32+
void startAsyncInSpan() {
33+
AggregatedHttpRequest request =
34+
AggregatedHttpRequest.of(
35+
HttpMethod.GET, resolveAddress(SUCCESS, "h1c://") + "?startAsyncInSpan=true");
36+
AggregatedHttpResponse response = client.execute(request).aggregate().join();
37+
38+
assertThat(response.status().code()).isEqualTo(SUCCESS.getStatus());
39+
assertThat(response.contentUtf8()).isEqualTo(SUCCESS.getBody());
40+
41+
testing()
42+
.waitAndAssertTraces(
43+
trace ->
44+
trace.hasSpansSatisfyingExactly(
45+
span ->
46+
span.hasName("GET " + getContextPath() + "/success")
47+
.hasKind(SpanKind.SERVER)
48+
.hasNoParent(),
49+
span ->
50+
span.hasName("startAsync")
51+
.hasKind(SpanKind.INTERNAL)
52+
.hasParent(trace.getSpan(0)),
53+
span ->
54+
span.hasName("controller")
55+
.hasKind(SpanKind.INTERNAL)
56+
.hasParent(trace.getSpan(0))));
57+
}
2258
}

‎instrumentation/servlet/servlet-5.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/TestServlet5.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static io.opentelemetry.javaagent.instrumentation.servlet.v5_0.AbstractServlet5Test.HTML_PRINT_WRITER;
1818
import static io.opentelemetry.javaagent.instrumentation.servlet.v5_0.AbstractServlet5Test.HTML_SERVLET_OUTPUT_STREAM;
1919

20+
import io.opentelemetry.instrumentation.testing.GlobalTraceUtil;
2021
import io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint;
2122
import jakarta.servlet.AsyncContext;
2223
import jakarta.servlet.RequestDispatcher;
@@ -100,7 +101,12 @@ public static class Async extends HttpServlet {
100101
protected void service(HttpServletRequest req, HttpServletResponse resp) {
101102
ServerEndpoint endpoint = ServerEndpoint.forPath(req.getServletPath());
102103
CountDownLatch latch = new CountDownLatch(1);
103-
AsyncContext context = req.startAsync();
104+
boolean startAsyncInSpan =
105+
SUCCESS.equals(endpoint) && "true".equals(req.getParameter("startAsyncInSpan"));
106+
AsyncContext context =
107+
startAsyncInSpan
108+
? GlobalTraceUtil.runWithSpan("startAsync", () -> req.startAsync())
109+
: req.startAsync();
104110
if (endpoint.equals(EXCEPTION)) {
105111
context.setTimeout(5000);
106112
}

‎instrumentation/servlet/servlet-5.0/tomcat-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/tomcat/TomcatServlet5AsyncTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55

66
package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.tomcat;
77

8+
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.trace.SpanKind;
812
import io.opentelemetry.javaagent.instrumentation.servlet.v5_0.TestServlet5;
13+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest;
14+
import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
15+
import io.opentelemetry.testing.internal.armeria.common.HttpMethod;
916
import jakarta.servlet.Servlet;
17+
import org.junit.jupiter.api.Test;
1018

1119
class TomcatServlet5AsyncTest extends TomcatServlet5Test {
1220

@@ -19,4 +27,32 @@ public Class<? extends Servlet> servlet() {
1927
public boolean errorEndpointUsesSendError() {
2028
return false;
2129
}
30+
31+
@Test
32+
void startAsyncInSpan() {
33+
AggregatedHttpRequest request =
34+
AggregatedHttpRequest.of(
35+
HttpMethod.GET, resolveAddress(SUCCESS, "h1c://") + "?startAsyncInSpan=true");
36+
AggregatedHttpResponse response = client.execute(request).aggregate().join();
37+
38+
assertThat(response.status().code()).isEqualTo(SUCCESS.getStatus());
39+
assertThat(response.contentUtf8()).isEqualTo(SUCCESS.getBody());
40+
41+
testing()
42+
.waitAndAssertTraces(
43+
trace ->
44+
trace.hasSpansSatisfyingExactly(
45+
span ->
46+
span.hasName("GET " + getContextPath() + "/success")
47+
.hasKind(SpanKind.SERVER)
48+
.hasNoParent(),
49+
span ->
50+
span.hasName("startAsync")
51+
.hasKind(SpanKind.INTERNAL)
52+
.hasParent(trace.getSpan(0)),
53+
span ->
54+
span.hasName("controller")
55+
.hasKind(SpanKind.INTERNAL)
56+
.hasParent(trace.getSpan(0))));
57+
}
2258
}

‎instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ServletAsyncContext.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ public class ServletAsyncContext implements ImplicitContextKeyed {
1919
private boolean isAsyncListenerAttached;
2020
private Throwable throwable;
2121
private Object response;
22+
private Context context;
2223

2324
public static Context init(Context context) {
2425
if (context.get(CONTEXT_KEY) != null) {
@@ -61,13 +62,22 @@ public static Object getAsyncListenerResponse(@Nullable Context context) {
6162
return servletAsyncContext != null ? servletAsyncContext.response : null;
6263
}
6364

64-
public static void setAsyncListenerResponse(@Nullable Context context, Object response) {
65+
public static void setAsyncListenerResponse(Context context, Object response) {
6566
ServletAsyncContext servletAsyncContext = get(context);
6667
if (servletAsyncContext != null) {
6768
servletAsyncContext.response = response;
69+
servletAsyncContext.context = context;
6870
}
6971
}
7072

73+
public static Context getAsyncListenerContext(Context context) {
74+
ServletAsyncContext servletAsyncContext = get(context);
75+
if (servletAsyncContext != null) {
76+
return servletAsyncContext.context;
77+
}
78+
return null;
79+
}
80+
7181
@Override
7282
public Context storeInContext(Context context) {
7383
return context.with(CONTEXT_KEY, this);

‎instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/AsyncRequestCompletionListener.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,14 @@ public AsyncRequestCompletionListener(
2323
Instrumenter<ServletRequestContext<REQUEST>, ServletResponseContext<RESPONSE>> instrumenter,
2424
ServletRequestContext<REQUEST> requestContext,
2525
Context context) {
26+
// The context passed into this method may contain other spans besides the server span. To end
27+
// the server span we get the context that set at the start of the request with
28+
// ServletHelper#setAsyncListenerResponse that contains just the server span.
29+
Context serverSpanContext = servletHelper.getAsyncListenerContext(context);
2630
this.servletHelper = servletHelper;
2731
this.instrumenter = instrumenter;
2832
this.requestContext = requestContext;
29-
this.context = context;
33+
this.context = serverSpanContext != null ? serverSpanContext : context;
3034
}
3135

3236
@Override

‎instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletHelper.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,4 +120,8 @@ public void recordAsyncException(Context context, Throwable throwable) {
120120
public Throwable getAsyncException(Context context) {
121121
return ServletAsyncContext.getAsyncException(context);
122122
}
123+
124+
public Context getAsyncListenerContext(Context context) {
125+
return ServletAsyncContext.getAsyncListenerContext(context);
126+
}
123127
}

0 commit comments

Comments
 (0)