Baggage + CorrelationContext improvements by Eddy & Mike

[CodeBlanch]

Changes:

• Added a BaggageFormat (ITextFormat) for propagating Activity.Baggage using W3C Baggage spec.
• Http-in & Http-out instrumentation now uses CompositePropagator(TraceContextFormat, BaggageFormat) by default.
• Switched CorrelationContext to use Activity.Baggage. The CorrelationContext API doesn't match the spec for set, remove, & clear. Those aren't supported on Activity. But the spec seems likely to change again. Seems more important to support Baggage right now?
• Removed DistributedContext. There's no mention of it in the spec at all.
• Removed IsInjected from ITextFormat. It wasn't part of the spec and didn't really make sense for something optional like Baggage.
• Exposed baggage operations on TelemetrySpan. Java does the same.

TODO:

• Add more tests.
• Fix baggage being lost on http-in cases.
• Update CHANGELOG.

[codecov]

Codecov Report

Merging #1048 into master will decrease coverage by 0.52%.
The diff coverage is 69.62%.

@@            Coverage Diff             @@
##           master    #1048      +/-   ##
==========================================
- Coverage   75.87%   75.35%   -0.53%     
==========================================
  Files         225      222       -3     
  Lines        6225     6268      +43     
==========================================
  Hits         4723     4723              
- Misses       1502     1545      +43     

Impacted Files	Coverage Δ
src/OpenTelemetry.Api/Metrics/MeasureMetric.cs	25.00% <0.00%> (ø)
...penTelemetry.Api/Metrics/NoopBoundCounterMetric.cs	0.00% <ø> (ø)
...penTelemetry.Api/Metrics/NoopBoundMeasureMetric.cs	0.00% <ø> (ø)
src/OpenTelemetry.Api/Metrics/NoopCounterMetric.cs	14.28% <ø> (ø)
...enTelemetry/Metrics/DoubleBoundCounterMetricSdk.cs	75.00% <ø> (ø)
...enTelemetry/Metrics/DoubleBoundMeasureMetricSdk.cs	0.00% <ø> (ø)
...rc/OpenTelemetry/Metrics/DoubleCounterMetricSdk.cs	45.45% <ø> (ø)
...penTelemetry/Metrics/Int64BoundCounterMetricSdk.cs	75.00% <ø> (ø)
...penTelemetry/Metrics/Int64BoundMeasureMetricSdk.cs	66.66% <ø> (ø)
src/OpenTelemetry/Metrics/Int64CounterMetricSdk.cs	45.45% <ø> (ø)
... and 39 more

[reyang]

Couple questions:

• Would there be memory allocation concern?
• What would happen if a consumer gets the IEnumerable and tries to iterate through it, while the underlying baggage got modified?
• Does this enforce immutability? https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/correlationcontext/api.md#get-correlations

[CodeBlanch]

Would there be memory allocation concern?

Check out the underlying Activity.Baggage enumeration:
https://github.com/dotnet/runtime/blob/ead2cd50799ecc523e23f50e50b54d8a30d0aa4f/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L290

Right below that method is the get.

The memory won't be too bad. An allocation for the state thing generated by the compiler due to the yield. The internal storage is a LinkedList, not great for searching. O(N)?

What would happen if a consumer gets the IEnumerable and tries to iterate through it, while the underlying baggage got modified?

I think the way the LinkedList works under the hood, it would actually be OK. User can only add to the back of the list. So if you were enumerating you would also consume that new one when you got to the tail.

Does this enforce immutability?

I think so. The enumeration is returning the KVPs. They are structs, so you essentially get a copy?

[reyang]

LGTM, love the fact that we've removed more lines than we added 👍

[CodeBlanch]

I think this is ready for a proper review. There seems to be a couple of flaky tests. Don't seem to be related to the changes. I'll keep digging on those. @cijothomas Anything jump out at you about them?

[cijothomas]

This API will not match the spec, right? As spec wants "Set" behavior, but we have "add" behavior, and also we won't have "Remove" feature.

[CodeBlanch]

Correct. The current version is built on top of Activity.Baggage, which is very limiting. Let's discuss this. The other option is we could make CorrelationContext independent of Baggage which would allow us to fully implement the spec. If we do that, let's remove "AllChildren" from #969. CorrelationContext could be used for "AllChildren" behavior and then EnrichmentScope is just responsible for "FirstChild" behavior?

[cijothomas]

This represents both SpanContext and Baggage. Not sure if the name TextFormatContext is aptly capturing the purpose.

[CodeBlanch]

Shooting down a name without a better suggestion isn't fair 🤣 I went with PropagationContext. Probably some further improvements needed. Let's do a follow-up PR? Our ITextFormat name doesn't really mirror what the other libraries are doing. All I have checked have HttpTextFormat type of things, which is what is called out in the spec. Some chain like TraceContextHttpPropagationFormat is IHttpPropagationFormat is IPropagationFormat?

[eddynaka]

I like IPropagationFormat :)

[cijothomas]

Will follow up for the following

1. Should we make CorrelationContext as spec compliant and build it outside Activity.Baggage or just use the Baggage from Activity.
2. Potential perf issue as .net (asp.net core) and OTel is potentially extracting the same context in the hot path before sampling.