CorrelationContext -> Baggage #1106
Conversation
…use it generates a compiler warning about struct default constructor being used.
| namespace OpenTelemetry.Context | ||
| { | ||
| /// <summary> | ||
| /// Baggage context. |
There was a problem hiding this comment.
nit: Add a link to OTel spec here.
There was a problem hiding this comment.
Done. It's a dead link right now, but should work once the rename PR is merged.
| /// <summary> | ||
| /// Baggage context. | ||
| /// </summary> | ||
| public readonly struct BaggageContext : IEquatable<BaggageContext> |
There was a problem hiding this comment.
I think we should name it just Baggage.
https://w3c.github.io/baggage/
There was a problem hiding this comment.
I have a different opinion on this. This is similar to the fact that we have SpanContext while W3C has TraceContext.
The W3C spec is describing the wire format. The in process representation and class name don't have to (or even should not) follow a name mentioned in the wire format spec.
There was a problem hiding this comment.
We can call it whatever OpenTelemetry spec decide to call it. It looked to me like the plan is to replace CorrelationContext to Baggage.
open-telemetry/opentelemetry-specification#536
If its called BaggageContext, then we are good here.
There was a problem hiding this comment.
Yes, totally agree. We should follow the OpenTelemetry spec (unless the name is already taken by .NET - such like Span 😆).
There was a problem hiding this comment.
When I was doing this work, I had it called "Baggage" initially. But for some reason, I felt like the spec would stick with keeping "Context" in the name and it would end up being "BaggageContext" instead. It's really just a guess, we can always rename when it settles?
There was a problem hiding this comment.
Yes, lets keep BaggageContext for now.
There was a problem hiding this comment.
There was a problem hiding this comment.
Changed name to Baggage
|
@CodeBlanch the overall approach looks good! Please mark as non-draft when you are ready. |
Codecov Report
@@ Coverage Diff @@
## master #1106 +/- ##
==========================================
+ Coverage 77.41% 77.73% +0.31%
==========================================
Files 219 219
Lines 6270 6279 +9
==========================================
+ Hits 4854 4881 +27
+ Misses 1416 1398 -18
|
| /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns> | ||
| public Baggage RemoveBaggage(string name) | ||
| { | ||
| var baggage = new Dictionary<string, string>(this.baggage ?? EmptyBaggage, StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
minor: if the key does not exist, we might be able to return the same baggage?
There was a problem hiding this comment.
I guess it is not important at this stage, and we probably will replace dictionary with a better data structure before GA anyways.
There was a problem hiding this comment.
OK I'll leave as is. I did consider that case. I figured if user is calling RemoveBaggage, they probably know it exists. So I didn't want to waste perf doing a hash + lookup where in most cases it would be wasteful? My thinking could be off tho.
|
|
||
| HttpRequestMessage thisRequest = (HttpRequestMessage)activity.GetCustomProperty(HttpHandlerDiagnosticListener.RequestCustomPropertyName); | ||
|
|
||
| string[] correlationContext = thisRequest.Headers.GetValues("Correlation-Context").First().Split(','); |
There was a problem hiding this comment.
this is not a feature of Otel right? do we want to test it?
|
No blocking comments. Merging. We can write docs/examples as follow ups. |
Changes
We talked on the last SIG about changing the work done on #1048 so that it isn't built on top of Activity.Baggage.
CorrelationContextBaggage API spec entirely, with some additions for perf and usability.TODO: