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.

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

Trace and Span IDs are no longer propagated RequestBodyAdvice beans #33091

Closed
philwebb opened this issue Jun 25, 2024 · 6 comments
Closed

Trace and Span IDs are no longer propagated RequestBodyAdvice beans #33091

philwebb opened this issue Jun 25, 2024 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches theme: observability An issue related to observability and tracing type: regression A bug that is also a regression
Milestone

Comments

@philwebb
Copy link
Member

Originally raised in spring-projects/spring-boot#41220 (with a reproducer project).

It looks to me like the reason for lack of Trance and Span IDs is the fix for #32730.

If you run the sample with Spring Boot 3.3.0 with a breakpoint in OncePerRequestFilter.doFilter() you'll see that the call toskipDispatch() return false. With Spring Boot 3.3.1 skipDispatch() returns true because the shouldNotFilterAsyncDispatch() method has been removed.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 25, 2024
@jhoeller jhoeller added in: web Issues in web modules (web, webmvc, webflux, websocket) theme: observability An issue related to observability and tracing labels Jun 25, 2024
@bclozel bclozel self-assigned this Jun 25, 2024
@bclozel
Copy link
Member

bclozel commented Jun 25, 2024

@syedyusufh just for confirmation, your sample application is named "webflux" but is actually using Spring MVC. Did you mean to use WebFlux instead? In this case, can you try with spring.main.web-application-type=reactive?

Also, could you elaborate on why you're relying on a trace being present during an asynchronous write? This is outside the scope of the controller handler, quite similar to spawning a new thread to process the request with Spring MVC async support (in this case, the threadlocals would not be set there automatically).

I understand that #32730 changes the behavior here, but I'm not sure this was ever intended/supported.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jun 25, 2024
@syedyusufh
Copy link

Hi @bclozel Our applications have always been Spring Web + Spring WebFlux combined, wherein Tomcat supports the Mono via Async.

This has been working fine for us since Spring Boot 2.5.x atleast and until the new v3.3.1

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 25, 2024
@bclozel
Copy link
Member

bclozel commented Jun 25, 2024

@syedyusufh ok thanks for confirming - so the "webflux" in your sample is a typo, as it was never the intent to use WebFlux. I'll see if there's a way to support async dispatches again without undoing the previous fix.

bclozel added a commit to bclozel/spring-framework that referenced this issue Jun 26, 2024
Prior to this commit, the fix for spring-projectsgh-32730 disabled the involvment of
the osbervation filter for async dispatches. Instead of relying on ASYNC
dispatches to close the observation for async requests, this is now
using an async listener instead: async dispatches are not guaranteed to
happen once the async request is handled.

This change caused another side-effect: because async dispatches are not
considered anymore by this filter, the observation scope is not
reinstated for async dispatches. For example, `ResponseBodyAdvice`
implementations do not have the observation scope opened during their
execution.

This commit re-enables async dispatches for this filter, but ensures
that observations are not closed during such dispatches as this will be
done by the async listener.

Fixes spring-projectsgh-33091
@bclozel
Copy link
Member

bclozel commented Jun 26, 2024

I have a commit ready for this but I'm not in favor of applying it right now for the following reasons:

  • the main goal of this filter is to observe the handling of the HTTP request, especially in controllers
  • we never guaranteed that observation scopes would be reinstated during async dispatches
  • handling ASYNC dispatch types would also mean that the onScopeOpened method would be called multiple times (once for REQUEST dispatch, and another time for ASYNC). This is meant to be used to handle HTTP response headers. At the point of ASYNC dispatches, response headers are likely to be read-only.

I do understand that the trace id is not logged anymore for ResponseBodyAdvice implementations, but right now I'm leaning towards this being a niche use case, compared to the performance and consistency aspects.

I'm closing this issue for now, but we can always reconsider if we receive more feedback and descriptions of use cases that are not possible anymore.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jun 26, 2024
@bclozel bclozel changed the title Trace and Span IDs are no longer propogated RequestBodyAdvice beans Trace and Span IDs are no longer propagated RequestBodyAdvice beans Jun 26, 2024
@syedyusufh
Copy link

syedyusufh commented Jun 26, 2024

@bclozel TracingContext is lost in RestControllerAdvice as well where you define ExceptionHandler for logging and throwing user defined exceptions.

Also, ResponseBodyAdvice helps customize the responseBody and definitely any info / failure there needs to be logged along with the original TraceContext

Our implementation is quite similar to what is mentioned in this official documentation page https://docs.spring.io/spring-boot/reference/web/reactive.html#web.reactive.webflux

Having this been available since atleast Spring Boot v2.5.x, kindly reconsider. Thanks

@bclozel
Copy link
Member

bclozel commented Jun 28, 2024

Reopening to check the @ExceptionHandler case.

@bclozel bclozel reopened this Jun 28, 2024
@bclozel bclozel added status: waiting-for-triage An issue we've not yet triaged or decided on type: bug A general bug and removed status: declined A suggestion or change that we don't feel we should currently apply status: waiting-for-triage An issue we've not yet triaged or decided on in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jun 28, 2024
@bclozel bclozel added this to the 6.1.11 milestone Jul 1, 2024
@bclozel bclozel removed the type: bug A general bug label Jul 1, 2024
@bclozel bclozel added type: regression A bug that is also a regression for: backport-to-6.0.x labels Jul 1, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x labels Jul 1, 2024
@bclozel bclozel closed this as completed in ab236c7 Jul 1, 2024
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches theme: observability An issue related to observability and tracing type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

5 participants