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

Jump to bottom

Automatic context propagation stops working after an error occurs #30013

Closed
surmabck opened this issue Feb 22, 2023 · 6 comments
Closed

Automatic context propagation stops working after an error occurs #30013

surmabck opened this issue Feb 22, 2023 · 6 comments
Assignees
@bclozel
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: observability An issue related to observability and tracing type: bug A general bug
Milestone
6.1.0-M1

Comments

@surmabck
Copy link

Expected Behavior

I believe WebFilters should have auto context propagation regardless if error occurred or not.

Actual Behavior

Automatic context propagation enabled via Hooks.enableAutomaticContextPropagation(); stops working when error was being handled in WebFilter. Once error is caught, all further WebFilters are going to be stripped of MDC context.

Reproductor

I've prepared reproducer with tests describing positive and negative(failing) flow, here -> https://github.com/surmabck/spring-boot-3xx-micrometer-issues/tree/only-reactor

FYI, ticket was originally opened against reactor-core

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 22, 2023
@bclozel bclozel self-assigned this Feb 22, 2023
@chemicL
Copy link
Member

chemicL commented Feb 22, 2023 •
edited
Loading

ExceptionHandlingWebHandler assembles the WebFilters and then actual handlers, and on top of that wraps around the existing WebExceptionHandlers using Mono.onErrorResume.

The fact that the Observation is created in a WebFilter (ServerHttpObservationFilter) makes it impossible for the WebExceptionHandlers to see the Observation, as the Context is limited to the upstream of the chain.

In order to allow WebExceptionHandlers to see an Observation, Spring would need to initiate it earlier than in a WebFilter.

As a side note, this doesn't have anything to do with automatic context propagation from Reactor 3.5.3. If handle or tap operators were used to log, they would not see the MDC context populated either, as the Reactor Context is empty.

@bclozel bclozel added type: bug A general bug theme: observability An issue related to observability and tracing labels Feb 24, 2023
@bclozel bclozel added this to the 6.0.x milestone Feb 24, 2023
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 24, 2023
@bclozel
Copy link
Member

bclozel commented Feb 24, 2023

As discussed with @chemicL, this is a limitation of the current design which is using a WebFilter. The exception handling mechanism is applied with ExceptionHandlingWebHandler, which is a HttpHandlerDecorator. WebFilter instances are applied at a higher level and are not aware of this.

This limitation is serious enough that I'm considering this a bug and should be fixed in 6.0.x. A possible solution would be to reimplement the instrumentation as a HttpHandlerDecorator itself, but this needs to validated. I'm scheduling this issue for 6.0.x but this can be revisited depending on our findings with HttpHandlerDecorator. As a result, the existing WebFilter should be deprecated.

@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Mar 7, 2023
@jhoeller jhoeller changed the title Automatic context propagation stops working after an error occurrs Automatic context propagation stops working after an error occurs Mar 7, 2023
@jhoeller jhoeller modified the milestones: 6.0.x, 6.0.8 Mar 7, 2023
@chemicL chemicL mentioned this issue Apr 5, 2023
Closed
@bclozel bclozel modified the milestones: 6.0.8, 6.1.0-M1 Apr 6, 2023
bclozel added a commit to bclozel/spring-framework that referenced this issue Apr 6, 2023
@bclozel
Move reactive server instrumentation out of WebFilter
fed0f16 
Prior to this commit, the Observation instrumentation for Reactive
server applications was implemented with a `WebFilter`. This allowed to
record observations and set up a tracing context for the controller
handlers.

The limitation of this approach is that all processing happening at a
lower level is not aware of any observation. Here, the
`HttpWebHandlerAdapter` handles several interesting aspects:

* logging of HTTP requests and responses at the TRACE level
* logging of client disconnect errors
* handling of unresolved errors

With the current instrumentation, these logging statements will miss the
tracing context information. As a result, this commit deprecates the
`ServerHttpObservationFilter` in favor of a more direct instrumentation
of the `HttpWebHandlerAdapter`. This enables a more precise
instrumentattion and allows to set up the current observation earlier in
the reactor context: log statements will now contain the relevant
information.

Fixes spring-projectsgh-30013
@bclozel
Copy link
Member

bclozel commented Apr 6, 2023

Changes are ready in https://github.com/bclozel/spring-framework/tree/gh-30013, but we've decided to reschedule this for 6.1.0 as they are quite involved for a late maintenance release in the 6.0.x line.

rstoyanchev added a commit that referenced this issue May 23, 2023
@rstoyanchev
Fix link to code in documentation
f926ded 
See gh-30013
@bclozel bclozel mentioned this issue May 25, 2023
Closed
@shakuzen shakuzen mentioned this issue May 29, 2023
Closed
GregBragg pushed a commit to GregBragg/spring-integration that referenced this issue Jun 7, 2023
@artembilan
Fix deprecations in test from SF
c24d10c 
Related to spring-projects/spring-framework#30013

The `WebHttpHandlerBuilder` customization with an `ObservationRegistry`
doesn't add a `SERVER` trace as it was with deprecated `ServerHttpObservationFilter`
jhoeller added a commit that referenced this issue Jun 14, 2023
@jhoeller
Remove ServerWebExchange dependency in ServerRequestObservationContext
f00a8cb 
Avoiding cycle between http.server and web.server packages.

See gh-30013
jhoeller added a commit that referenced this issue Jun 14, 2023
@jhoeller
Adapt UserController example to current ServerRequestObservationContext
362e189 
See gh-30013
@bclozel bclozel mentioned this issue Jun 29, 2023
Closed
@izeye izeye mentioned this issue Jun 30, 2023
Merged
izeye added a commit to izeye/spring-framework that referenced this issue Jun 30, 2023
@izeye
Polish spring-projectsgh-30013
ea3868e 
See spring-projectsgh-30013
jhoeller pushed a commit that referenced this issue Jun 30, 2023
@izeye @jhoeller
Polish gh-30013
41f8b69 
See gh-30013
@bclozel bclozel mentioned this issue Jul 5, 2023
Closed
This was referenced Sep 11, 2023
Closed
Closed
@kasra-haghpanah kasra-haghpanah mentioned this issue Sep 23, 2023
Closed
@bclozel bclozel mentioned this issue Oct 2, 2023
Closed
@nyckyta
Copy link

nyckyta commented Aug 30, 2024

Hi.

Any news regarding releasing this change for web flux?

@bclozel
Copy link
Member

bclozel commented Aug 30, 2024

@nyckyta this has been released with 6.1.0

@nyckyta
Copy link

nyckyta commented Aug 30, 2024

@bclozel. Aha, excuse me, I see it now. Thank you

@chemicL chemicL mentioned this issue Oct 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bclozel bclozel

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: observability An issue related to observability and tracing type: bug A general bug
Projects
None yet
Milestone
6.1.0-M1
Development

No branches or pull requests
6 participants
@bclozel @jhoeller @chemicL @spring-projects-issues @surmabck @nyckyta