enforce fixed length of 16 for _dd.p.tid#42
Conversation
| span_data->tags[tags::internal::propagation_error] = | ||
| "inconsistent_tid " + extant->second; | ||
| extant->second = hex_high; | ||
| } else { |
There was a problem hiding this comment.
Maybe add a comment here, supporting the comment above, describing what's going on. For example, the appearance of extant->second = hex_high; twice might be surprising.
cgilmour
left a comment
cgilmour
left a comment
There was a problem hiding this comment.
Approved, with a usual amount of comments for consideration
| span_tags[tags::internal::propagation_error] = "malformed_tid " + value; | ||
| const Optional<std::uint64_t> high = | ||
| parse_trace_id_high(value, span_tags); | ||
| if (!high) { |
There was a problem hiding this comment.
I'd consider tagging things in this block instead. It's the point where the error is being handled
There was a problem hiding this comment.
I could remove the span_tags argument from parse_trace_id_high, and duplicate the setting of "malformed_tid " ... in the two places where it is used. Yeah, I like that better.
| const std::string& value, | ||
| std::unordered_map<std::string, std::string>& span_tags) { | ||
| auto high = parse_uint64(value, 16); | ||
| if (!high || value.size() != 16) { |
There was a problem hiding this comment.
Size/length could be checked before attempting to parse it
There was a problem hiding this comment.
Oh the underlying parse_integer is stripping spaces. That's fairly liberal. And makes the size check inaccurate.
It also means there should be some quoting around value when setting an error.
There was a problem hiding this comment.
parse_integer maybe ought to be more strict. I don't remember why I stripped the input -- maybe I was anticipating data coming from HTTP headers and environment variables. Probably better to be be more strict. I'll consider changing that...
| auto test_case = GENERATE(values<TestCase>( | ||
| {{__LINE__, "invalid _dd.p.tid", "noodle", "malformed_tid "}, | ||
| {__LINE__, "_dd.p.tid inconsistent with trace ID", "adfeed", | ||
| {__LINE__, "short _dd.p.tid", "beef", "malformed_tid "}, |
There was a problem hiding this comment.
I'd probably test with values of length 0, 1, 15, and 17.
4 and 22 seems a bit arbitrary
There was a problem hiding this comment.
It is arbitrary. For the long tests I held down 0 until it looked longer than 16. For the short test I omitted any 0s.
1, 15, and 17, to be just below and above 16? Yeah, I see your point. I'll leave this, though.
BenchmarksBenchmark execution time: 2023-07-11 19:33:20 Comparing candidate commit 20f8dd0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
dgoffredo
force-pushed
the
david.goffredo/tid-has-fixed-length
branch
from
f2e918e to
20f8dd0
Compare
Fixed a regression introduced in #42 preventing HTTP headers with leading spaces to be parsed.
Fixed a regression introduced in #42 preventing HTTP headers with leading spaces to be parsed.
Fixed a regression introduced in #42 preventing HTTP headers with leading spaces to be parsed.
2 participants
I missed a spot in #41. The integration test's
-kcommand line argument, used to select a subset of tests to run, doesn't work the way I thought, so I wasn't running all of the tests.This revision enforces a fixed length of 16 for the
_dd.p.tidtrace tag, i.e. the 64-bit hex value must be padded with zeroes.