Skip to content

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