Fix tomcat async spans #4339
Fix tomcat async spans #4339
Conversation
|
@trask Just curious does this have anything to do with the strict context stuff we talked about? |
| return null; | ||
| })); | ||
| ctx -> { | ||
| HttpServerTest.controller(QUERY_PARAM, () -> null); |
There was a problem hiding this comment.
WDYT about removing the second argument of controller? The passed lambda seems to be unneeded now
There was a problem hiding this comment.
it's still needed in some cases. i'm not super convinced about changing the structure of the controllers the way I did, since the existing structure probably mimic'd user code better. so i reverted those changes, and just suppressed the test in more places for now short of a better idea
| @@ -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); | |||
There was a problem hiding this comment.
Is this the actual fix?
There was a problem hiding this comment.
It should be. The problem is in
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.
There was a problem hiding this comment.
this would be nice if it leads to unifying app server behavior a bit, I created #4347 to revisit the idea
there's definitely overlap, the new test points out places where the server span can end before the controller span IIRC one option we discussed previously was introducing some mechanism to delay the span end until the active scope was closed |
trask
requested review from
anuraaga,
iNikem,
jkwatson,
pavolloffay and
tylerbenson
as code owners
* Add test * Fix tomcat async spans * Preserve existing test controller behavior * Comments
Resolves #4334
Adds test for all server instrumentation to verify that server spans end after all of their child spans (with a couple of exceptions).