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

Demonstrate bug: span hierarchy is incorrect when client call is nested within an internal span #510

Draft
wants to merge 5 commits into
base: 6.5.x
Choose a base branch
from

Conversation

lgathy
Copy link
Contributor

@lgathy lgathy commented Feb 12, 2024

Only the very last commit is relevant for the bug itself, I needed to merge in a couple other pending changes though to be able to demonstrate this.

First, I thought that this might be an issue with incorrect tracing implementation of either the @NewSpan or the declarative HTTP client, but then I created a similar test for Jaeger tracing, and the exact same problem is present there.

This is how far I got in trying to figure out where it goes wrong:

  • When the reactive Mono of the internal worker is returned, the HTTP filter chain is not assembled yet for the outgoing client call.
  • Upon calling Mono.toFuture() in the controller method the initialization of the client filter chain is triggered and for some reason it uses the current span context (which is the top-level server span) instead of the internal span.
  • When the implementation of the worker method annotated with @NewSpan starts with the downstream call instead of the Mono.just, then the correct span context is captured as the parent of the client span of the outgoing call.

If we return a Mono in the controller method instead of converting it to CompletableFuture the span hierarchy is captured correctly. However, when using micronaut-graphql integration we need to return CompletableFuture-s.

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2024

CLA assistant check
All committers have signed the CLA.

@lgathy
Copy link
Contributor Author

lgathy commented Feb 12, 2024

@dstepanov Can you please take a look at these failing test cases? We ran into further problems still after the fixes micronaut-projects/micronaut-core#10444 and micronaut-projects/micronaut-core#10445.

In this case I suspect that the issue might be in DefaultHttpClient or HttpClientIntroductionAdvice, but I couldn't yet find a solution.

@dstepanov
Copy link
Contributor

I don't think there is a way to support propagating the context using futures.
Maybe you can create a graphql testcase and we can find a way to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants