Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tomcat async spans #4339

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Fix tomcat async spans #4339

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b63c50a
Add test
trask Oct 10, 2021
ea57d6b
Fix tomcat async spans
trask Oct 10, 2021
113342d
Merge remote-tracking branch 'upstream/main' into fix-tomcat-async-spans
trask Oct 11, 2021
5ee8e4b
Preserve existing test controller behavior
trask Oct 11, 2021
81cb6cf
Comments
trask Oct 11, 2021
aeb6f7e
Merge remote-tracking branch 'upstream/main' into fix-tomcat-async-spans
trask Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions instrumentation/dropwizard-testing/src/test/groovy/DropwizardAsyncTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ class DropwizardAsyncTest extends DropwizardTest {
AsyncServiceResource
}

@Override
boolean verifyServerSpanEndTime() {
// SERVER span is ended inside of JAX-RS controller span
return false
}

static class AsyncTestApp extends Application<Configuration> {
@Override
void initialize(Bootstrap<Configuration> bootstrap) {
Expand Down
6 changes: 6 additions & 0 deletions instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyAsyncTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ class GrizzlyAsyncTest extends GrizzlyTest {
false
}

@Override
boolean verifyServerSpanEndTime() {
// SERVER span is ended inside of JAX-RS controller span
return false
}

@Path("/")
static class AsyncServiceResource {
private ExecutorService executor = Executors.newSingleThreadExecutor()
Expand Down
6 changes: 4 additions & 2 deletions instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,13 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
.header("Content-Length", valueOf(responseParameters.getResponseBody().length))
responseParameters.fillHeaders(builder)
HttpResponsePacket responsePacket = builder.build()
def response
controller(responseParameters.getEndpoint()) {
ctx.write(HttpContent.builder(responsePacket)
response = HttpContent.builder(responsePacket)
.content(wrap(ctx.getMemoryManager(), responseParameters.getResponseBody()))
.build())
.build()
}
ctx.write(response)
}
}
return ctx.getStopAction()
Expand Down
6 changes: 6 additions & 0 deletions instrumentation/play/play-2.4/javaagent/src/test/groovy/server/PlayServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ class PlayServerTest extends HttpServerTest<Server> implements AgentTestTrait {
return true
}

@Override
boolean verifyServerSpanEndTime() {
// TODO (trask) see if the play controller instrumentation can be ended before netty server span
return false
}

@Override
void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
trace.span(index) {
Expand Down
18 changes: 9 additions & 9 deletions ...io/opentelemetry/instrumentation/ratpack/server/AbstractRatpackAsyncHttpServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
SUCCESS
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(endpoint.body)
}
context.response.status(endpoint.status).send(endpoint.body)
}
}
}
Expand All @@ -49,8 +49,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
} then {
controller(INDEXED_CHILD) {
INDEXED_CHILD.collectSpanAttributes { context.request.queryParams.get(it) }
context.response.status(INDEXED_CHILD.status).send()
}
context.response.status(INDEXED_CHILD.status).send()
}
}
}
Expand All @@ -60,8 +60,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
QUERY_PARAM
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(context.request.query)
}
context.response.status(endpoint.status).send(context.request.query)
}
}
}
Expand All @@ -71,8 +71,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
REDIRECT
} then { endpoint ->
controller(endpoint) {
context.redirect(endpoint.body)
}
context.redirect(endpoint.body)
}
}
}
Expand All @@ -82,8 +82,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
ERROR
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(endpoint.body)
}
context.response.status(endpoint.status).send(endpoint.body)
}
}
}
Expand All @@ -104,8 +104,8 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
PATH_PARAM
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(context.pathTokens.id)
}
context.response.status(endpoint.status).send(context.pathTokens.id)
}
}
}
Expand All @@ -115,10 +115,10 @@ abstract class AbstractRatpackAsyncHttpServerTest extends AbstractRatpackHttpSer
CAPTURE_HEADERS
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(endpoint.body)
}
context.response.status(endpoint.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(endpoint.body)
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions ...o/opentelemetry/instrumentation/ratpack/server/AbstractRatpackForkedHttpServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
SUCCESS
}.fork().then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(endpoint.body)
}
context.response.status(endpoint.status).send(endpoint.body)
}
}
}
Expand All @@ -57,8 +57,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
}.fork().then {
controller(INDEXED_CHILD) {
INDEXED_CHILD.collectSpanAttributes { context.request.queryParams.get(it) }
context.response.status(INDEXED_CHILD.status).send()
}
context.response.status(INDEXED_CHILD.status).send()
}
}
}
Expand All @@ -68,8 +68,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
QUERY_PARAM
}.fork().then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(context.request.query)
}
context.response.status(endpoint.status).send(context.request.query)
}
}
}
Expand All @@ -79,8 +79,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
REDIRECT
}.fork().then { endpoint ->
controller(endpoint) {
context.redirect(endpoint.body)
}
context.redirect(endpoint.body)
}
}
}
Expand All @@ -90,8 +90,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
ERROR
}.fork().then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(endpoint.body)
}
context.response.status(endpoint.status).send(endpoint.body)
}
}
}
Expand All @@ -112,8 +112,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
PATH_PARAM
}.fork().then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(context.pathTokens.id)
}
context.response.status(endpoint.status).send(context.pathTokens.id)
}
}
}
Expand All @@ -123,10 +123,10 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
CAPTURE_HEADERS
}.fork().then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(endpoint.body)
}
context.response.status(endpoint.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(endpoint.body)
}
}
}
Expand All @@ -141,8 +141,8 @@ abstract class AbstractRatpackForkedHttpServerTest extends AbstractRatpackHttpSe
Promise.sync { list.get(0).value }
} then { endpoint ->
controller(endpoint) {
context.response.status(endpoint.status).send(endpoint.body)
}
context.response.status(endpoint.status).send(endpoint.body)
}
}
}
Expand Down
18 changes: 9 additions & 9 deletions ...oovy/io/opentelemetry/instrumentation/ratpack/server/AbstractRatpackHttpServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,37 +44,37 @@ abstract class AbstractRatpackHttpServerTest extends HttpServerTest<RatpackServe
it.prefix(SUCCESS.rawPath()) {
it.all { context ->
controller(SUCCESS) {
context.response.status(SUCCESS.status).send(SUCCESS.body)
}
context.response.status(SUCCESS.status).send(SUCCESS.body)
}
}
it.prefix(INDEXED_CHILD.rawPath()) {
it.all { context ->
controller(INDEXED_CHILD) {
INDEXED_CHILD.collectSpanAttributes { context.request.queryParams.get(it) }
context.response.status(INDEXED_CHILD.status).send()
}
context.response.status(INDEXED_CHILD.status).send()
}
}
it.prefix(QUERY_PARAM.rawPath()) {
it.all { context ->
controller(QUERY_PARAM) {
context.response.status(QUERY_PARAM.status).send(context.request.query)
}
context.response.status(QUERY_PARAM.status).send(context.request.query)
}
}
it.prefix(REDIRECT.rawPath()) {
it.all { context ->
controller(REDIRECT) {
context.redirect(REDIRECT.body)
}
context.redirect(REDIRECT.body)
}
}
it.prefix(ERROR.rawPath()) {
it.all { context ->
controller(ERROR) {
context.response.status(ERROR.status).send(ERROR.body)
}
context.response.status(ERROR.status).send(ERROR.body)
}
}
it.prefix(EXCEPTION.rawPath()) {
Expand All @@ -87,17 +87,17 @@ abstract class AbstractRatpackHttpServerTest extends HttpServerTest<RatpackServe
it.prefix("path/:id/param") {
it.all { context ->
controller(PATH_PARAM) {
context.response.status(PATH_PARAM.status).send(context.pathTokens.id)
}
context.response.status(PATH_PARAM.status).send(context.pathTokens.id)
}
}
it.prefix(CAPTURE_HEADERS.rawPath()) {
it.all { context ->
controller(CAPTURE_HEADERS) {
context.response.status(CAPTURE_HEADERS.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(CAPTURE_HEADERS.body)
}
context.response.status(CAPTURE_HEADERS.status)
context.response.headers.set("X-Test-Response", context.request.headers.get("X-Test-Request"))
context.response.send(CAPTURE_HEADERS.body)
}
}
}
Expand Down
13 changes: 2 additions & 11 deletions ...rc/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import io.opentelemetry.instrumentation.test.base.HttpServerTest
import javax.servlet.annotation.WebServlet
import javax.servlet.http.HttpServletRequest
import javax.servlet.http.HttpServletResponse
import java.util.concurrent.CountDownLatch

import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_HEADERS
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
Expand All @@ -26,7 +25,6 @@ class AsyncServlet extends AbstractHttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) {
HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
def latch = new CountDownLatch(1)
def context = req.startAsync()
context.start {
try {
Expand All @@ -36,44 +34,37 @@ class AsyncServlet extends AbstractHttpServlet {
case SUCCESS:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
break
case INDEXED_CHILD:
endpoint.collectSpanAttributes { req.getParameter(it) }
resp.status = endpoint.status
context.complete()
break
case QUERY_PARAM:
resp.status = endpoint.status
resp.writer.print(req.queryString)
context.complete()
break
case REDIRECT:
resp.sendRedirect(endpoint.body)
context.complete()
break
case CAPTURE_HEADERS:
resp.setHeader("X-Test-Response", req.getHeader("X-Test-Request"))
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
break
case ERROR:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
break
case EXCEPTION:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
throw new Exception(endpoint.body)
}
}
} finally {
latch.countDown()
// complete at the end so the server span will end after the controller span
context.complete()
}
}
latch.await()
}
}
5 changes: 4 additions & 1 deletion .../src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.servlet.AppServerBridge;
import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletHelper;
import org.apache.coyote.Request;
import org.apache.coyote.Response;
Expand All @@ -32,7 +33,9 @@ public boolean shouldStart(Context parentContext, Request request) {
}

public Context start(Context parentContext, Request request) {
return instrumenter.start(parentContext, request);
Context context = instrumenter.start(parentContext, request);
request.setAttribute(HttpServerTracer.CONTEXT_ATTRIBUTE, context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the actual fix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be. The problem is in

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

Line 101 in 8066f27

Context context = getServerContext(request);
for async listeners we need to get the context for server span which servlet instrumentation stores in a request attribute. This works because HttpServletRequest implementation on tomcat stores attributes on the underlying coyote request. The only other server that does not use servlet api based instrumenter is undertow where request is always ended from a callback and presumably doesn't need special handling for async requests.
I guess alternatively we could add a field to AppServerBridge to carry context and in ServletHelper try both AppServerBridge and request attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be nice if it leads to unifying app server behavior a bit, I created #4347 to revisit the idea

return context;
}

public void end(
Expand Down
Loading