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

No traces info anymore in implementation classes of AbstractErrorWebExceptionHandler #35604

Closed
jackcat13 opened this issue May 23, 2023 · 9 comments
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid theme: observability Issues related to observability

Comments

@jackcat13
Copy link

jackcat13 commented May 23, 2023

Reproducer details and problem explanation

A reproducer is shared at the following location : https://github.com/jackcat13/missingTraceAndSpanInAbstractErrorWebExceptionHandler

It contains a SpringBoot application that can be started locally. When reaching the endpoint /api/book, it raises an exception which triggers the class GlobalErrorWebExceptionHandler which extends AbstractErrorWebExceptionHandler

Doing it, it appears that no spanId nor traceId is printed whereas it is present in the controller itslef :

image

The problem appeared with SpringBoot 3.0.3 version, it was not present in previous versions. The reproducer itself is relying on version 3.1.0.

Note : Hooks.enableAutomaticContextPropagation(); statement is already called in the constructor of the SpringBootApplication class.

Thanks in advance for your time on this issue.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 23, 2023
@wilkinsona wilkinsona added the theme: observability Issues related to observability label May 23, 2023
@GurkiratSingh37
Copy link

GurkiratSingh37 commented May 24, 2023

In the handle method of GlobalErrorWebExceptionHandler, retrieve the span and add the spanId and traceId to the error response.

Span currentSpan = tracer.currentSpan();
if (currentSpan != null) {
   response.getHeaders().add("spanId", currentSpan.context().spanId());
   response.getHeaders().add("traceId", currentSpan.context().traceId());
}

Please give these suggestions a try and let me know if it helps resolve the issue.

@deepakraghav0
Copy link

In the handle method of GlobalErrorWebExceptionHandler, retrieve the span and add the spanId and traceId to the error response.

Span currentSpan = tracer.currentSpan(); if (currentSpan != null) { response.getHeaders().add("spanId", currentSpan.context().spanId()); response.getHeaders().add("traceId", currentSpan.context().traceId()); }

Please give these suggestions a try and let me know if it helps resolve the issue.

@GurkiratSingh37 : No, it will not run, as current span is itself not present there, it is there till controller.

@wilkinsona
Copy link
Member

Thanks for the sample, @jackcat13. I've reproduced the behaviour that you have described but I'm curious about earlier versions.

The problem appeared with SpringBoot 3.0.3 version, it was not present in previous versions.

I can reproduce the problem with 3.0.3, but with 3.0.2 the sample does not compile as the Hooks.enableAutomaticContextPropagation() does not exist. How did you configure things when it worked with Spring Boot 3.0.2 and earlier?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label May 24, 2023
@jackcat13
Copy link
Author

@wilkinsona
It's about some automated tests that stopped working on mentioned version. But maybe I can try to setup in a separate branch in the sample project to validate or not my observations.

@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 May 25, 2023
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 25, 2023
@jackcat13
Copy link
Author

@wilkinsona
Hi again. I am able to make the observation work at runtime with the following code in the controller code (even in 3.1.0) :

Mono.deferContextual(contextView -> {
                    ContextSnapshot.captureAll(contextView).setThreadLocals();
                });

I still have the issue in my codebase but I'm not yet able to find out the root cause.
In any case, without capturing the ContextView from reactor context, I'm not able to make it work (same in 3.0.2 version). I don't know if the automatic propagation should be able to manage it or not. What do you think ?

Thanks a lot for your help anyways :)

@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 May 25, 2023
@wilkinsona
Copy link
Member

@jackcat13 Interesting. Thanks. I've asked the observability team to take a look.

@bclozel
Copy link
Member

bclozel commented May 25, 2023

This looks fairly similar to spring-projects/spring-framework#30013. The observability instrumentation in Spring Framework 6.0 is based on a WebFilter, which doesn't wrap the error handling phase. This explains why the MDC is not restored in those phases. Because this requires a major change in the instrumentation, we've applied this for Spring Framework 6.1.0.

I'm not sure how deferContextual works around this problem, nor where it is applied. In all cases, if the root problem is the absence of observation context information in WebFlux error handling, I think this issue can be closed in favor of the Framework one.

@jackcat13
Copy link
Author

Indeed, it looks very similar. I'll give it a try once the fix is available. Thanks a lot for all your feedbacks !

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale May 25, 2023
@wilkinsona wilkinsona added status: invalid An issue that we don't feel is valid for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels May 25, 2023
@chemicL
Copy link
Member

chemicL commented May 25, 2023

My 5c: @jackcat13 calling

ContextSnapshot.captureAll(contextView).setThreadLocals();

creates a leak of ThreadLocal values which can be disastrous (especially in terms of security), please don't use it this way (you're leaving an open ContextSnapshot.Scope, which you never close). The solution @bclozel mentions from the framework 6.1 milestone is what you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid theme: observability Issues related to observability
Projects
None yet
Development

No branches or pull requests

7 participants