Adds BaggageThreadLocalAccessor #508

marcingrzejszczak · 2024-01-04T14:30:05Z

previously we've tried to store both baggage and span through TracingObservationHandler. That didn't hold when working with reactive applications because it would require users to create artificial baggage scopes and actually due to scopes getting closed early, we would lose the actual baggage value on the attached TraceContext.

with this change we revert to previous approach where the TOH actually creates a scope for spans only. For context propagation we are treating baggage as a first class citizen that requires its own ThreadLocalAccessor, that's why we create the BaggageThreadLocalAccessor, that should be registered after the ObservationAwareSpanThreadLocalAccessor. With that, in reactive applications, under the BaggageThreadLocalAccessor#KEY users will need to register the BaggageToPropagate in e.g. Reactor Context and then it's up to the Context Propagation library to start and stop scopes.

Additional tasks:

BraveBaggageManager can have the Micrometer Tracer set on it
BraveTracer automatically sets itself on BBM
Current Span will now be resolved from the MT if possible

Related issue - #504
Usage example

previously we've tried to store both baggage and span through TracingObservationHandler. That didn't hold when working with reactive applications because it would require users to create artificial baggage scopes and actually due to scopes getting closed early, we would lose the actual baggage value on the attached TraceContext. with this change we revert to previous approach where the TOH actually creates a scope for spans only. For context propagation we are treating baggage as a first class citizen that requires its own ThreadLocalAccessor, that's why we create the BaggageThreadLocalAccessor, that should be registered after the ObservationAwareSpanThreadLocalAccessor. With that, in reactive applications, under the BaggageThreadLocalAccessor#KEY users will need to register the BaggageToPropagate in e.g. Reactor Context and then it's up to the Context Propagation library to start and stop scopes. Additional tasks: * BraveBaggageManager can have the Micrometer Tracer set on it ** BraveTracer automatically sets itself on BBM ** Current Span will now be resolved from the MT if possible

chemicL

I'm a bit blurry on the situation where traceContext is empty for BaggageInScope, so I just expressed this in some questions. In general this effort looks good, time to add some tests :) Also, are there no changes required in the OTEL counterpart?

chemicL · 2024-01-04T16:21:22Z

...n/java/io/micrometer/tracing/contextpropagation/ObservationAwareSpanThreadLocalAccessor.java

@@ -214,6 +179,9 @@ public void restore(Span previousValue) {

    @Override
    public void restore() {
+        if (log.isTraceEnabled()) {
+            log.trace("Restoring no args");


Consider "restoring to empty span scope"

chemicL · 2024-01-04T16:22:52Z

...n/java/io/micrometer/tracing/contextpropagation/ObservationAwareSpanThreadLocalAccessor.java

@@ -226,21 +194,26 @@ static class SpanAction implements Closeable {

        final Map<Thread, SpanAction> todo;

-        Consumer<?> scope;
+        Closeable scope;


Are there any arguments against using AutoCloseable instead?

...meter-tracing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageToPropagate.java

chemicL · 2024-01-04T16:33:17Z

...meter-tracing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageToPropagate.java

+    private final Map<String, String> baggage;
+
+    public BaggageToPropagate(Map<String, String> baggage) {
+        this.baggage = baggage;


Should this be a reference assignment or a copy? Please see my other comment in BaggageThreadLocalAccessor.

chemicL · 2024-01-04T16:35:17Z

...acing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageThreadLocalAccessor.java

+            log.trace("Current baggage in thread local [" + baggageInScope.get(Thread.currentThread())
+                    + "], current baggage from tracer [" + baggage + "]");
+        }
+        return (baggage == null || baggage.isEmpty()) ? null : new BaggageToPropagate(baggage);


I'm a bit concerned about the isEmpty() check. I can imagine a situation where even though the map is empty, it can be filled in later, which won't be reflected. That sounds ok, but at the same time, currently, a non-empty map can become empty later and with a reference used will make the contents be reflected in the captured BaggageToPropagate. This should be consistent. Most probably by just copying the contents in the constructor of the holder.

chemicL · 2024-01-04T16:36:57Z

...acing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageThreadLocalAccessor.java

+
+    @Override
+    public void setValue(BaggageToPropagate value) {
+        BaggageAndScope previousConsumer = baggageInScope.get(Thread.currentThread());


Why is the name consumer used here?

chemicL · 2024-01-04T16:45:17Z

...acing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageThreadLocalAccessor.java

+    public void setValue() {
+        BaggageAndScope previousConsumer = baggageInScope.get(Thread.currentThread());
+        if (log.isTraceEnabled()) {
+            log.trace("setValue no args, current baggage scope [" + baggageInScope.get(Thread.currentThread()) + "]");


Consider "setValue to empty baggage scope ..."

chemicL · 2024-01-04T16:46:23Z

...acing/src/main/java/io/micrometer/tracing/contextpropagation/BaggageThreadLocalAccessor.java

+    @Override
+    public void restore() {
+        if (log.isTraceEnabled()) {
+            log.trace("Calling restore()");


Consider "restoring to empty baggage scope"

chemicL · 2024-01-04T16:54:48Z

...meter-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/BraveTracer.java

@@ -44,6 +44,9 @@ public BraveTracer(brave.Tracer tracer, CurrentTraceContext context, BaggageMana
        this.tracer = tracer;
        this.braveBaggageManager = braveBaggageManager;
        this.currentTraceContext = context;
+        if (braveBaggageManager instanceof BraveBaggageManager) {


If it's not a BraveBaggageManager, isn't it an error? Should the previous assignment still be executed?

Theoretically a user can provide their own baggage manager. Practically there are almost zero chances that this would happen

chemicL · 2024-01-04T16:57:43Z

...acing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/BraveBaggageInScope.java

-            this.delegate.updateValue(value);
+            boolean success = this.delegate.updateValue(value);
+            if (logger.isTraceEnabled()) {
+                logger.trace("Managed to update the baggage on set [" + success + "]");


Would it be beneficial to distinguish the log from the situation where a traceContext is present?

...meter-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/BraveTracer.java

...t/src/test/java/io/micrometer/tracing/contextpropagation/TestBaggageThreadLocalAccessor.java

...ava/io/micrometer/tracing/contextpropagation/ObservationAwareBaggageThreadLocalAccessor.java

marcingrzejszczak requested a review from chemicL January 4, 2024 14:31

marcingrzejszczak force-pushed the baggage_thread_local_accessor branch from e349988 to 5bee69f Compare January 4, 2024 14:33

chemicL reviewed Jan 4, 2024

View reviewed changes

marcingrzejszczak added 2 commits January 5, 2024 10:34

Applied changes following the review

1c99b0b

WIP

6896a04

marcingrzejszczak force-pushed the baggage_thread_local_accessor branch 3 times, most recently from ef4d9fc to 967ca1b Compare January 9, 2024 14:13

Polish

8ff705e

marcingrzejszczak force-pushed the baggage_thread_local_accessor branch from 967ca1b to 8ff705e Compare January 9, 2024 16:00

Added docs

16ad3a4

marcingrzejszczak marked this pull request as ready for review January 9, 2024 16:40

jonatan-ivanov reviewed Jan 10, 2024

View reviewed changes

marcingrzejszczak merged commit 3508fcc into 1.2.x Jan 11, 2024
6 checks passed

marcingrzejszczak deleted the baggage_thread_local_accessor branch January 11, 2024 11:52

marcingrzejszczak mentioned this pull request Jan 11, 2024

Spring Boot 3.2.1 misbehave with micrometer-tracing-bridge-brave:1.2.1 #504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds BaggageThreadLocalAccessor #508

Adds BaggageThreadLocalAccessor #508

marcingrzejszczak commented Jan 4, 2024 •

edited

Loading

chemicL left a comment

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

chemicL Jan 4, 2024

marcingrzejszczak Jan 5, 2024

chemicL Jan 4, 2024

Adds BaggageThreadLocalAccessor #508

Adds BaggageThreadLocalAccessor #508

Conversation

marcingrzejszczak commented Jan 4, 2024 • edited Loading

chemicL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcingrzejszczak commented Jan 4, 2024 •

edited

Loading