Rename CorrelationContext to Baggage #857
Conversation
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
| @@ -13,7 +13,7 @@ The OpenTelemetry specification describes the cross-language requirements and ex | |||
| - [Library Guidelines](specification/library-guidelines.md) | |||
| - [Package/Library Layout](specification/library-layout.md) | |||
| - API Specification | |||
| - [CorrelationContext](specification/correlationcontext/api.md) | |||
| - [Baggage](specification/baggage/api.md) | |||
There was a problem hiding this comment.
|
|
||
| ### Baggage | ||
|
|
||
| `Baggage` is used to annotate telemetry, adding context and information to metrics, traces, and logs. |
There was a problem hiding this comment.
Not part of name change, but a question about Baggage - Does all the Baggage entries becomes part of Span attributes automatically? Or user has to write SpanProcessor to read Baggage, then populate it into Span.Attributes?
| ### Baggage | ||
|
|
||
| `Baggage` is used to annotate telemetry, adding context and information to metrics, traces, and logs. | ||
| It is an abstract data type represented by a set of name/value pairs describing user-defined properties. |
There was a problem hiding this comment.
is the value "string" type always? or does it follow similar to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/common/common.md#attributes
There was a problem hiding this comment.
FWIW I went with a string in the .NET library because we lose all typing info propagating as W3C Baggage.
|
|
||
| #### Header Name | ||
|
|
||
| `Baggage` implementations MUST use the header name `otcorrelations`. |
There was a problem hiding this comment.
Is this still the recommendation or we can now use Baggage?
#536 (comment)
There was a problem hiding this comment.
let's track it separately. I think we should keep the renaming PR small. @cijothomas can you file this issue?
There was a problem hiding this comment.
@cijothomas have you created an issue?
There was a problem hiding this comment.
sorry i didn't hit submit:(
Thanks for creating the issue!
| @@ -0,0 +1,138 @@ | |||
| # Baggage API | |||
There was a problem hiding this comment.
Any thoughts on naming this BaggageContext?
cc: @CodeBlanch
|
|
||
| `Context` the context containing the `Baggage` from which to get the baggages. | ||
|
|
||
| ### Get baggage |
There was a problem hiding this comment.
I just happen to be working on this in the .NET library right now. Here's some questions in case we get into something other than the name change.
-
What type of case sensitivity should we have for keys?
-
What should the behavior be when
nullis passed asnameto GetBaggage/SetBaggage/RemoveBaggage? Throw error or return null/do nothing? -
What happens if you SetBaggage('name', null)? Is null valid? Should that throw? Should null remove the item if it exists (like we do for tags)?
carlosalberto
added
spec:baggage
There was a problem hiding this comment.
I'd like to keep renaming PR small. It would be great to make some of comments to be issues. @cijothomas @bogdandrutu can you file those?
@cijothomas consider re-reviewing this PR with the smaller scope of just renaming. Keeping big PRs open cause more work merging things
There was a problem hiding this comment.
Very happy to see Baggage name be adopted in otel; it's also what we set out with and arrived at in (server side) Swift, great that all terms are finally getting fleshed out 👍
(BaggageContext could also be good (in fact, that's what we do today), but Baggage is good, especially since it is carried in a context and one would want to avoid people writing "context.context" which we've seen some, so taking the context part of the name away helps in clarity).
👍
|
this has enough approvals. Letting this PR cool off for another day and will merge tonight if no objections. |
|
@SergeyKanzhelev CHANGELOG entry is missing (but you that can be done in a follow up as well, in order to merge this change fast). |
|
@bogdandrutu can you update the changelog? |
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com
Fixes #536