Simplify propagators #608
Simplify propagators #608
Conversation
Codecov Report
@@ Coverage Diff @@
## main #608 +/- ##
==========================================
+ Coverage 94.45% 94.51% +0.05%
==========================================
Files 197 197
Lines 9183 9101 -82
==========================================
- Hits 8674 8602 -72
+ Misses 509 499 -10
|
|
|
||
| TraceFlags() noexcept : rep_{0} {} | ||
|
|
||
| explicit TraceFlags(uint8_t flags) noexcept : rep_(flags) {} | ||
|
|
||
| bool IsSampled() const noexcept { return rep_ & kIsSampled; } | ||
|
|
||
| bool IsValid() const noexcept { return rep_ != kForbidden; } |
There was a problem hiding this comment.
Where is it specified that 0xff is an invalid value for trace state?
There was a problem hiding this comment.
Ah my bad, thanks! 0xFF was an invalid marker for the version not trace flags. Will fix in the morning, not at the computer atm
There was a problem hiding this comment.
Fixed - it now checks the version for 0xFF not trace flags 🤦
| TraceId trace_id = TraceIdFromHex(trace_id_hex); | ||
| SpanId span_id = SpanIdFromHex(span_id_hex); |
There was a problem hiding this comment.
I didn't test it, but it seems to me this wouldn't catch too short trace and span ids. Like when parsing a header 00-1-1-00.
There was a problem hiding this comment.
Added more checks and additional test cases for w3c propagation.
| @@ -43,13 +43,13 @@ bool IsValidHex(nostd::string_view s) | |||
| */ | |||
| bool HexToBinary(nostd::string_view hex, uint8_t *buffer, size_t buffer_size) | |||
| { | |||
| std::memset(buffer, 0, buffer_size); | |||
There was a problem hiding this comment.
Also check the input hex is empty, as it cannot be handled in below code?
There was a problem hiding this comment.
It is handled - nothing is written to the buffer if hex is empty.
|
quick validated these changes against w3c tracecontext tests (https://github.com/open-telemetry/opentelemetry-cpp/tree/main/ext/test/w3c_tracecontext_test), and don't see any new failure (other than existing trace-state failures). Will go ahead and merge them now. |
Changes
detailnamespace)I'd also like to rename the propagator files for consistency:
propagation/b3_propagator.h->propagation/b3.hpropagation/http_trace_context->propagation/w3c.hpropagation/jaeger.hwill stay as isWhat do you think?
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes