-
Notifications
You must be signed in to change notification settings - Fork 1.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 TraceState to SpanContext in API #1340
Conversation
- stdout exporter test - SDK test
Codecov Report
@@ Coverage Diff @@
## master #1340 +/- ##
========================================
+ Coverage 78.3% 78.5% +0.1%
========================================
Files 125 125
Lines 6259 6324 +65
========================================
+ Hits 4906 4966 +60
- Misses 1110 1114 +4
- Partials 243 244 +1
|
- Adjust tests for trace API - Adjust adjacent parts of codebase (test utils, SDK etc.)
I'm looking at this reference implementation of the tracestate and I'm wondering if we want to take a similar approach the I think the only outstanding issue would be around how to handle invalid (e.g. too many members, invalid members) tracestate when serializing back into the header. |
Unless I'm missing something, we would not want the user to directly interact with the slice, as the API spec puts some limitations on |
Ah, makes sense. Thanks for the link to the specification, TIL. 👍 |
- for type SpanContext, if SpanID, TraceID, TraceFlag and TraceState are equal - for type TraceState, if entries of both respective trace states are equal Signed-off-by: Matej Gera <matejgera@gmail.com>
@MrAlias thanks for the feedback ❤️, I updated parts which are clear, waiting for clarifications on the rest. |
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.
None of my feedback should be considered blocking. If you agree with the suggestion to use label.Set
and its map-key equality support, that could be a TODO. The suggestion to move IsEqualWith()
into a test package feels important to do now. I'll approve and let the maintainers decide. :-)
- Move IsEqualWith method to be only in test package - Minor improvements, typos etc.
The specification on
SpanContext
requires thatTraceState
is part of the of the tracing API.This PR adds
TraceState
toSpanContext
, implements operations according to the spec and adjusts some related parts of the code base (OTLP span transform, SDK test,stdout
exporter test).Some parts based on the previously umerged PR #81.
Does not include changes to propagators, however, if required they can be done as part of this PR as well.
Resolves #1302.