Add baggage implementation based on the W3C and OpenTelemetry specification #1967
Conversation
MrAlias
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod and
paivagustavo
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1967 +/- ##
=======================================
+ Coverage 76.5% 77.1% +0.6%
=======================================
Files 160 161 +1
Lines 8495 8554 +59
=======================================
+ Hits 6503 6602 +99
+ Misses 1733 1696 -37
+ Partials 259 256 -3
|
|
This is a bikeshed comment, please feel free to disregard it. The OTEL spec uses the language name/value pair for Member, and metadata for Property. I'm ok using the W3C names, but that won't be in line with otel. |
I don't have strong feelings here. I'll rename. |
Digging into this I might have stronger feelings. The validation errors we return reference W3C terms. The OpenTelemetry specification says nothing about these kinds of errors other than the W3C specification needs to be implemented. It seems prudent to give error messages relevant to the error meaning they will be in terms of the W3C. Additionally, the OpenTelemetry specification terms are only partially prescriptive in the areas it does specify. The name/value paradigm is not specified for Metadata meaning that we would provide an interface with assumed names. This will be confusing if we are saying the name passed does not match the W3C specification, but the specification calls it a key. There have been prior discussions in the specification SIG about name choices, and the guidance has been to follow it when it makes sense. I don't think it makes sense here, and there are examples of other SIGs making similar choices and the inconsistency of introducing the terms in some places and not others will lead to confusion. For these reasons I'm going to stick with the current naming. |
| errInvalidKey = errors.New("invalid key") | ||
| errInvalidValue = errors.New("invalid value") | ||
| errInvalidProperty = errors.New("invalid baggage list-member property") | ||
| errInvalidMember = errors.New("invalid baggage list-member") | ||
| errMemberNumber = errors.New("too many list-members in baggage-string") | ||
| errMemberBytes = errors.New("list-member too large") | ||
| errBaggageBytes = errors.New("baggage-string too large") |
There was a problem hiding this comment.
Nit, If these are unexported we can't match against them.
There was a problem hiding this comment.
I'm fine exporting if we see a need from users here. I didn't want to pollute the API and would prefer to have user need before doing so. As for matching, I figured only the local unit tests would need to do this, but do you think there is already a need that motivate these being exported?
Related Docs
Changes
Added
Baggage,Member, andPropertytypes are added to thego.opentelemetry.io/otel/baggagepackage along with their related functions.ContextWithBaggage,ContextWithoutBaggage, andFromContextfunctions were added to thego.opentelemetry.io/otel/baggagepackage. These functions replace theSet,Value,ContextWithValue,ContextWithoutValue, andContextWithEmptyfunctions from that package and directly work with the newBaggagetype.Changed
baggagepackage is refactored to be the minimal types needed to work with baggage and context. This package was persisted to maintain the OpenTracing bridge's ability to synchronize state with any baggage updates using hooks.Removed
Set,Value,ContextWithValue,ContextWithoutValue, andContextWithEmptyfunctions in thego.opentelemetry.io/otel/baggagepackage are removed. Handling of baggage is now done using the addedBaggagetype and related context functions (ContextWithBaggage,ContextWithoutBaggage, andFromContext) in that package.Related Issues
Resolves #1539