-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add baggage implementation based on the W3C and OpenTelemetry specification #1967
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor notes.
Like I said that is a bikeshed comment. I personally think that we should try to keep to W3C, but that might put us at odds eventually.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, If these are unexported we can't match against them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, andProperty
types are added to thego.opentelemetry.io/otel/baggage
package along with their related functions.ContextWithBaggage
,ContextWithoutBaggage
, andFromContext
functions were added to thego.opentelemetry.io/otel/baggage
package. These functions replace theSet
,Value
,ContextWithValue
,ContextWithoutValue
, andContextWithEmpty
functions from that package and directly work with the newBaggage
type.Changed
baggage
package 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
, andContextWithEmpty
functions in thego.opentelemetry.io/otel/baggage
package are removed. Handling of baggage is now done using the addedBaggage
type and related context functions (ContextWithBaggage
,ContextWithoutBaggage
, andFromContext
) in that package.Related Issues
Resolves #1539