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

Manually created baggage fields do not propagate across threads using ObservationAwareSpanThreadLocalAccessor #412

Closed
wapkch opened this issue Oct 18, 2023 · 3 comments
Labels
bug A general bug
Milestone

Comments

@wapkch
Copy link

wapkch commented Oct 18, 2023

A sample is like this, the value of baggage field 'tenant' is expected to propagated correctly. But it's null now.

management.tracing.baggage.remote-fields=tenant
@SpringBootApplication
public class MicrometerTracingDemoApplication {

    public static void main(String[] args) {
        SpringApplication.run(MicrometerTracingDemoApplication.class, args);
    }

    @Bean
    CommandLineRunner commandLineRunner(ObservationRegistry observationRegistry, Tracer tracer) {
        return args -> {
            ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
            executor.setTaskDecorator(runnable ->
                ContextSnapshotFactory.builder().build().captureAll().wrap(runnable));
            executor.initialize();

            ScopedSpan scopedSpan = tracer.startScopedSpan("test");
            try (BaggageInScope baggageInScope = tracer.getBaggage("tenant").makeCurrent("tenant")) {
                executor.submit(() -> {
                    String tenant = tracer.getBaggage("tenant").get();
                    // tenant should not be null
                    System.out.println(tenant);
                });
            } finally {
                scopedSpan.end();
            }
        };
    }

}

@Component
class Registrar implements SmartInitializingSingleton {

    @Autowired
    ObservationRegistry observationRegistry;

    @Autowired
    Tracer tracer;

    @Override
    public void afterSingletonsInstantiated() {
        ContextRegistry.getInstance().registerThreadLocalAccessor(
            new ObservationAwareSpanThreadLocalAccessor(observationRegistry, tracer));
    }
}

The sample can be found here https://github.com/wapkch/micrometer-tracing-demo/tree/master

@krodyrobi
Copy link

krodyrobi commented Oct 26, 2023

From playing with the library I noticed that:

  • spans get recreated even if Hooks.enableContextPropagation is used and executors are instrumented
  • since custom baggages are manually created they won't be refreshed or created when thread switching

All of this comes down to a subtle difference in what is available by default in each context.
Controllers/main schedulers wrap code with Observation.start(..).observe(..) implicitly for us and that without an active observation nothing gets propagated (for instance tests / command line runners don't have this - can check if observationRegistry has a currentObservation).

Tracing ids take the lack of an observation in context as a sign to recreate themselves, while custom baggages can't possibly be re-set as code creating them is not apart of the same library flow.

If you by chance wrap the submit part in the observe scoped lambda do you still see them not propagating?

@wapkch
Copy link
Author

wapkch commented Oct 31, 2023

Yes, using Observation.start(..).observe(..) and explicitly passing a ReceiverContext could do the trick:

ReceiverContext<Map<String, String>> context = new ReceiverContext<>(Map::get);
            context.setCarrier(Map.of("tenant", "tenant"));
            Observation.createNotStarted("test", () -> context, observationRegistry).observe(() -> {
                executor.submit(() -> {
                    String tenant = tracer.getBaggage("tenant").get();
                    // tenant should not be null
                    System.out.println(tenant);
                });
            });

Because ReceiverContext activates the PropagatingReceiverTracingObservationHandler which propagates the custom baggages.

But in this way, we don't even need a ObservationAwareSpanThreadLocalAccessor.

So, i still think ObservationAwareSpanThreadLocalAccessor should work with the tracer.startScopedSpan(...) style.

@wapkch
Copy link
Author

wapkch commented Oct 31, 2023

After taking a look into it, i found that when invoking captureFromThreadLocals, the tenant baggage value in the span is exist:
image

But when invoking setThreadLocals subsequencely, the tenant baggage value is missing in the restored span:
image

I'm not sure why now.

marcingrzejszczak added a commit that referenced this issue Dec 12, 2023
In Brave, baggage is tightly associated with the TraceContext. When Baggage is put in scope, the value gets attached to TraceContext (TC). In version 1.1 we've introduced scoping also for Brave, which means that the moment you close the scope we will return to the previously created baggage version.

Let us now imagine a situation where we open a scope for a span, within it we create a scope for baggage within which we are passing work to a separate thread (Span Scope -> Baggage Scope -> separate thread).

Without this change when the baggage scope on outer part is closed, the TC will have the baggage value reverted to the previous value. That will impact the same TC that is passed to the separate thread. That means the value of the baggage will be reverted for the same TC which is in the main and the separate thread. This is why it can be surprising that even though in the main thread there was baggage, it was not propagated to the other thread (in reality the main thread altered a shared resource between main and separate threads).

With this change, when OASTLA is getting the current span (in fact we're taking a snapshot of the span and baggage situation at this point in time) we are dumping also the corresponding baggage. That means that when the scope in the main thread gets closed, thus e.g. the values get reverted the TC in the second thread will not be affected. That's because when Context Propagation recreates the span scope, we will also recreate all the baggage scopes. Once the Context Propagation finishes its jobs we will close all the baggage scopes and lastly the span scope too.

Fixed issues with SimpleTracer and its support for baggage.

Also moved the OASTLA tests to integration tests module where we are using the same test logic with 3 different tracers, SimpleTracer, BraveTracer and OtelTracer

fixes gh-412
@marcingrzejszczak marcingrzejszczak added this to the 1.1.9 milestone Dec 12, 2023
@marcingrzejszczak marcingrzejszczak added the bug A general bug label Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants