-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Fix traceId discrepancy in case error in servlet web #17134
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: 6.4.x
Are you sure you want to change the base?
Conversation
cc7ac16
to
f526143
Compare
PTAL @jzheaux |
… version until spring-projects/spring-security#17134 will be merged
Signed-off-by: Nikita Konev <nikit.cpp@yandex.ru>
@jzheaux |
@marcusdacoregio @jzheaux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and the sample application, @nkonev. I've left some feedback inline.
Also, I think we should try adding a test for it. That may mean dispatching to ERROR
manually in the test.
@@ -250,9 +258,30 @@ private void wrapFilter(ServletRequest request, ServletResponse response, Filter | |||
private AroundFilterObservation parent(HttpServletRequest request) { | |||
FilterChainObservationContext beforeContext = FilterChainObservationContext.before(); | |||
FilterChainObservationContext afterContext = FilterChainObservationContext.after(); | |||
|
|||
Object maybeBeforeObservationView = request.getAttribute(ATTRIBUTE_BEFORE_OBSERVATION_VIEW); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the parent observations are available through the AroundFilterObservation
instance, it seems like the ATTRIBUTE
attribute should be sufficient.
For example, instead of two other attributes, would this give you what you need:
AroundFilterObservation parent = (AroundFilterObservation) request.getAttribute(ATTRIBUTE);
if (parent != null) {
beforeContext.setParentObservation(parent.before().getContext().getParentObservation());
afterContext.setParentObservation(parent.after().getContext().getParentObservation());
}
Note that I tried this modification locally and the traceId is preserved in your project.
Thank you for review @jzheaux !
Can you please explain it a bit more ? I still don't understand for what I need to write an assertion ? May be somewhere there is a similar test which I can copy and modify for this particular issue ? Did you mean to use MockMvc ? In the time when I wrote this PR I considered to make the following test: I decided it is too artificial in that time. Do you mean to something like this ? |
Fixes #12610
As I see from my research, the bug happens only in the real servlet environment, supposedly because of HttpServletResponse.sendError(), I didn't manage to reproduce it via mockMvc.
Tomcat creates a new HttpServletRequest in case error, which is related to "/error" path, which will be served by ErrorController.
Fortunately, request attributes are copied from the original request to the "/error"-related one, so we can put the ObservationView into the original request in order to extract this ObservationView from "/error"-related request and create a new parent observation with a given parent ObservationView. The last ObservationView propagates TraceContext to the new Observation so we have the same traceId.
Reproducer is here https://github.com/nkonev/trace-discrepancy-reproducer
To check that the bug is fixed, you can run
publishMavenJavaPublicationToMavenLocal
in spring-security:webor via IDE

then switch to the SNAPSHOT dependency nkonev/trace-discrepancy-reproducer@131a271
Before:


After:


UPD:
I also checked this PR with jetty and undertow, it works fine.