-
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
trace: ParseTraceState ignores duplicated tracestate keys #4638
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4638 +/- ##
=======================================
- Coverage 81.3% 81.3% -0.1%
=======================================
Files 222 222
Lines 17682 17682
=======================================
- Hits 14391 14389 -2
- Misses 2991 2993 +2
Partials 300 300
|
@@ -121,7 +120,7 @@ func ParseTraceState(tracestate string) (TraceState, error) { | |||
} | |||
|
|||
if _, ok := found[m.Key]; ok { | |||
return TraceState{}, wrapErr(errDuplicate) | |||
continue // Duplicate is ignored per https://github.com/w3c/trace-context/blob/551a5b0855171281e98b4c2a814bf9e1f837cd53/test/test.py#L563-L568. |
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.
This is a link to W3C unit tests. Can you instead link to the part of the W3C specification that describes this behavior?
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 have not found anything better than the test code in the specification that describes it. See: https://github.com/instana/trace-context/blob/master/spec/20-http_request_header_format.md
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 created w3c/trace-context#548
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.
Do you have any other suggestion or can this comment be resolved?
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.
It seems like this behavior is only mentioned in the editor draft of the W3C trace-context as optional. This PR seems premature given that unreleased state.
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.
Changing to draft
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.
It seems like this behavior is only mentioned in the editor draft of the W3C trace-context as optional.
For me even the editor draft does not clearly defines this behavior thus I created w3c/trace-context#548
Inspired by open-telemetry/opentelemetry-dotnet#4937 (kudos to @Kielek)
The change is a follow-up on the changes done in the Trace Context Specification:
I also created for completeness: w3c/trace-context#547