-
Notifications
You must be signed in to change notification settings - Fork 772
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
Fix TraceContextPropagator
behavior when trace parent flags contain…
#4893
Fix TraceContextPropagator
behavior when trace parent flags contain…
#4893
Conversation
258e8e0
to
cf29bfb
Compare
…s illegal characters. fixes: test_traceparent_trace_flags_illegal_characters
cf29bfb
to
7fe4d2d
Compare
Codecov Report
@@ Coverage Diff @@
## main #4893 +/- ##
==========================================
+ Coverage 83.26% 83.30% +0.04%
==========================================
Files 295 295
Lines 12324 12325 +1
==========================================
+ Hits 10261 10267 +6
+ Misses 2063 2058 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@open-telemetry/dotnet-approvers, I have 2 more tests fixes ready to review on separate branch. As it touches some internal implementation/decision it is worth to make separate reviews. If you think otherwise I can bring all of them in one PR. Finally it passes all W3c tests on .NET7+. 2 other fixes on the top of branch: https://github.com/Kielek/opentelemetry-dotnet/commits/tace-context-propagator-other-fixes |
try | ||
{ | ||
spanId = ActivitySpanId.CreateFromString(traceparent.AsSpan().Slice(VersionAndTraceIdLength, SpanIdLength)); | ||
options1 = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]); | ||
_ = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength]); // to verify if there is no bad chars on options position | ||
optionsLowByte = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]); | ||
} | ||
catch (ArgumentOutOfRangeException) |
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 outside the scope of this PR but we don't use the exception in any way. We could simply update HexCharToByte
method to return false
instead of throwing an unused exception.
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.
@Kielek Would you mind updating this follow-up PRs?
Fixes test_traceparent_trace_flags_illegal_characters from w3c test suite
Design discussion issue #
Changes
Fix
TraceContextPropagator
behavior when trace parent flags contains illegal characters.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)