Record exception for async servlet invocations #4677
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| // we expect this request to fail with http 500 but is succeeds with http 200 | ||
| // when using -PtestLatestDeps=true |
There was a problem hiding this comment.
do u know if this is a limitation of jetty, our instrumentation, or our tests?
There was a problem hiding this comment.
It is hard to say. My guess is that our test is weird and could very well depend on unspecified behaviour. Test first completes an AsyncContext and then lets exception propagate out of servlet. It might be better to not complete AsyncContext and rely on timeout (if AsyncContext isn't completed container will close it with timeout). In that case the behaviour should be better defined, but timeout has its own issues. There is always the problem that we'd need to make the timeout as short as possible to not waste time but still have it long enough so that test doesn't timeout before the exception is thrown even when running under load.
The tests that are currently failing also exhibit weirdness, apparently on tomcat10 exception test can break the subsequent test.
instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy
Outdated
Show resolved
Hide resolved
| Throwable throwable = servletHelper.getAsyncException(requestContext.request()); | ||
| instrumenter.end(context, requestContext, responseContext, throwable); |
There was a problem hiding this comment.
does an exception thrown from a Runnable executed under AsyncContext.start(Runnable) always lead to servlet failure?
There was a problem hiding this comment.
This is a good question. If the response has already been sent then exception can not turn it back. This behaviour is the same for regular servlets. So it is possible to have requests that were ok from http client's perspective, but failed (got an exception) from server's perspective.
| throw new ServletException(endpoint.body) | ||
| } | ||
| } | ||
| } finally { | ||
| // complete at the end so the server span will end after the controller span |
|
💯 |
* Record exception for asyn servlet invocations * add back accidentally commented out line * rearrange test so that it passes on both jetty and tomcat
Resolves #807 this should be the last remaining instrumentation in that issue.