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

The current active span is not considered while creating a child span in WebClientTracing #5216

Open
iampratapak opened this issue Oct 19, 2022 · 5 comments
Assignees
Labels
4.x Version 4.x enhancement New feature or request P3 tracing

Comments

@iampratapak
Copy link

The current active span is not considered while creating a child span in WebClientTracing. It's always referring to the span context that exists in the server request.

Environment Details

  • Helidon Version: 2.5.0
  • Helidon SE or Helidon MP: Helidon SE
  • JDK version: Java 11
  • OS: Windows 11
  • Docker version (if applicable):

Problem Description

I'm creating the first span as a child of the span context that exists within the server request. The second span I'm creating as a child of the first span. The code where the second span is active is making the outbound call.

The outbound call is intercepted by WebClientTracing and creates a span. Instead of reading the current active span from the scope manager, it's reading the span context that exists in the server request and creating a new span.

With this tracing tree, the parent-child relationship is not correct.

[WebClientTracing.java](https://github.com/helidon-io/helidon/blob/helidon-2.x/webclient/tracing/src/main/java/io/helidon/webclient/tracing/WebClientTracing.java#L61)

Expected:
image

Actual:
image

I'm doing the below workaround for now.

WebClient.builder()
                .addService(request -> {
                    Optional<Tracer> optionalTracer = request.context().get(Tracer.class);
                    Tracer tracer = optionalTracer.orElseGet(GlobalTracer::get);
                    final var activeSpan = tracer.scopeManager().activeSpan();
                    if (activeSpan != null) {
                        request.context().register(activeSpan.context());
                    }
                    return Single.just(request);
                })
                .addService(WebClientTracing.create())
                .build();

Steps to reproduce

  1. Enable WebClientTracing
  2. Create a span and use WebClient to make an outbound call.

Should I go ahead with the workaround since we don't have ScopeManager anymore in Helidon 3.X.X?

@tjquinno
Copy link
Member

tjquinno commented Oct 20, 2022

With Helidon SE there is “no magic” and the developer has the flexibility (and therefore the responsibility) of complete control.

You need to prepare some context so that the Helidon tracing code can find what it needs. Something like the following code in the service endpoint handler method should set things up so Helidon's code will create the span for the outbound WebClient request however you want:

        ServerRequest request = // the request delivered to your handler.

        // If present, use the tracer from the incoming request.
        Tracer tracer = request.context().get(Tracer.class).orElse(GlobalTracer.get());
        Tracer.SpanBuilder customSpanBuilder = tracer.buildSpan("my-span");

        // Set the parent of your span to the incoming request's span, if present.
        request.context().get(SpanContext.class).ifPresent(customSpanBuilder::asChildOf);

        Span span = customSpanBuilder.start();

        // Prepare the context for the outbound web client request.
        Context ctx = Context.create(request.context());
        ctx.register(tracer);
        ctx.register(span.context());
        
        WebClientRequestBuilder webClientRequestBuilder = // however you prepare it

        // Set the context you've prepared so Helidon's WebClient tracing code will use it.
        webClientRequestBuilder.context(ctx); 

        // Insert your code to send the request and deal with the response.
        ...
        // Remember to end the span.
        span.finish(); 

Two other notes:

  1. The existing Helidon WebClient tracing code looks for the tracer and span context settings which the example above prepares. If it finds none, it uses the currently active tracer to create a new span and does not explicitly set the new span's parent.

    We could consider as an enhancement that, in the absence of these context settings, the Helidon Webclient tracing code could use the currently-active span (if there is one) for the parent.

    For that to work, your code would need to set your span as the current/active one. Simply creating it would probably not be enough.

  2. Even without the code enhancement, we should probably add some content to the Helidon SE doc to explain how to set the tracer and span context explicitly as in the example above.

@tjquinno
Copy link
Member

@tomas-langer and @Verdent Is there any reason we should not use the currently-active span as the parent in this use case, assuming that the span context is absent from the request context?

@tjquinno
Copy link
Member

tjquinno commented Nov 3, 2022

@tomas-langer Reminded me that, in Helidon SE and WebClient, span context is kept in ThreadLocal. There is no guarantee that the tracing information in ThreadLocal when the WebClient code is handling tracing is the same as what was current when the application initiated the WebClient work.

As a result, we do not want to use a possibly-incorrect "current" span.

@tjquinno tjquinno closed this as completed Nov 3, 2022
@tjquinno
Copy link
Member

tjquinno commented Jan 4, 2024

Re-opening this issue for 4.x. In a virtual threads environment, as in 4.x, the outbound WebClient request will normally be on the same thread as the incoming request, so the WebClient request span could be parented to the current span if there is one.

@tjquinno tjquinno reopened this Jan 4, 2024
@tjquinno tjquinno added enhancement New feature or request 4.x Version 4.x labels Jan 4, 2024
@m0mus m0mus added the P3 label Jan 4, 2024
@m0mus m0mus added this to Backlog Aug 12, 2024
@m0mus m0mus moved this to Normal priority in Backlog Aug 12, 2024
@RickyFrost
Copy link

#8957
This was likely the missing "magic" referred to above, even for SE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x enhancement New feature or request P3 tracing
Projects
Status: Normal priority
Development

No branches or pull requests

4 participants