Baggage API: Thread-safety

[CodeBlanch]

This is a follow-up to the discussion on #2284.

Changes

• The static methods on Baggage which mutate Baggage.Current (SetBaggage, RemoveBaggage, & ClearBaggage) are now thread-safe within the scope of an ExecutionContext.
• The instance methods on Baggage no longer mutate Baggage.Current.

Breaking behavior changes

Previously if you called SetBaggage, RemoveBaggage, or ClearBaggage on a Baggage instance it would mutate Baggage.Current. For example:

var baggage = Baggage.Current;
baggage.SetBaggage(K1, V1);

Baggage.Current.SetBaggage(K2, V2);

Now you have to explicitly set back the instance. Like this:

var baggage = Baggage.Current;
baggage.SetBaggage(K1, V1);
Baggage.Current = baggage;

Baggage.Current = Baggage.Current.SetBaggage(K2, V2);

If you use the statics, this is done automatically:

Baggage.SetBaggage(K1, V1);
Baggage.SetBaggage(K2, V2);

The reason for the change is Baggage is a readonly struct so when you do Baggage.Current you get a copy. You can write code like this:

var currentBaggage = Baggage.Current;
currentBaggage.SetBaggage(K1, V1); // User probably wants this to become Current

var detachedBaggage = Baggage.Create();
detachedBaggage.SetBaggage(K2, V2); // What does the user want in this case? Mutating Current might be unexpected

Basically when the instance SetBaggage executes it doesn't know if it is current or not. I thought about managing a flag on Baggage so it could know if it was attached or not, but it is readonly so there's no (safe) way to update the flag. We could compare it back to Current, but another thread could have already changed it. I decided the best option was to always treat it as detached and allow the user to make the decision about whether or not it should become the "Current" version or not. I think this makes the API more consistent and usable but it will break anyone reliant on this behavior today.

Alternatively, we could [Obsolete] Baggage and introduce something new with a contract.

TODOs

• CHANGELOG.md updated for non-trivial changes

[CodeBlanch]

Created as a draft to get feedback on the breaking behavior change. If we're OK with this, I'll update changelog with a note and convert to a real PR.

/cc @cijothomas @alanwest @reyang

[reyang]

LGTM, it is not a breaking change from the signature perspective.

It does change the behavior which might break the users if they rely on certain behavior, but I feel that if we leave it as is (which is a bit confusing I guess?) we probably will end up surprising/confusing more users moving forward.

[codecov]

Codecov Report

Merging #2298 (fb5650e) into main (86c524c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2298      +/-   ##
==========================================
+ Coverage   79.61%   79.66%   +0.04%     
==========================================
  Files         233      233              
  Lines        7325     7346      +21     
==========================================
+ Hits         5832     5852      +20     
- Misses       1493     1494       +1     

Impacted Files	Coverage Δ
src/OpenTelemetry.Api/Baggage.cs	100.00% <100.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	76.47% <0.00%> (-5.89%)	⬇️
...Zipkin/Implementation/ZipkinExporterEventSource.cs	72.72% <0.00%> (+9.09%)	⬆️

[cijothomas]

can you reverse the order of the example codes below, so that it matches the Note: section.

[CodeBlanch]

I reversed your request to reverse the code examples and instead reversed the notes. I wanted to have the preferred approach (static methods) first. I also updated the README to the safer static API.

[reyang]

I reversed your request to reverse the code examples and instead reversed the notes.

This makes my day 😵😆

[CodeBlanch]

Ya had some fun with that one 😄 Also very excited about the pun opportunities for my next PR: "Lost Baggage"

[cijothomas]

LGTM.
Suggested to modify the readme to guide users to use static (Baggage.SetBaggage).