Skip to content

Commit 440c28e

Browse files
dgoffredodgoffredo
authored
enforce fixed length of 16 for _dd.p.tid (#42)
* enforce fixed length of 16 for _dd.p.tid * integer parsing doesn't skip whitespace * unfactor setting of _dd.propagation_error tag * add a clarifying comment * test parse_int and parse_uint64
1 parent 9b2ac56 commit 440c28e

File tree

5 files changed

+193
-19
lines changed

5 files changed

+193
-19
lines changed

‎src/datadog/parse_util.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ namespace {
1717
template <typename Integer>
1818
Expected<Integer> parse_integer(StringView input, int base, StringView kind) {
1919
Integer value;
20-
input = strip(input);
2120
const auto status = std::from_chars(input.begin(), input.end(), value, base);
2221
if (status.ec == std::errc::invalid_argument) {
2322
std::string message;

‎src/datadog/tracer.cpp‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,22 @@ namespace datadog {
2727
namespace tracing {
2828
namespace {
2929

30+
// Parse the high 64 bits of a trace ID from the specified `value`. If `value`
31+
// is correctly formatted, then return the resulting bits. If `value` is
32+
// incorrectly formatted then return `nullopt`.
33+
Optional<std::uint64_t> parse_trace_id_high(const std::string& value) {
34+
if (value.size() != 16) {
35+
return nullopt;
36+
}
37+
38+
auto high = parse_uint64(value, 16);
39+
if (high) {
40+
return *high;
41+
}
42+
43+
return nullopt;
44+
}
45+
3046
// Decode the specified `trace_tags` and integrate them into the specified
3147
// `result`. If an error occurs, add a `tags::internal::propagation_error` tag
3248
// to the specified `span_tags` and log a diagnostic using the specified
@@ -48,12 +64,8 @@ void handle_trace_tags(StringView trace_tags, ExtractedData& result,
4864

4965
if (key == tags::internal::trace_id_high) {
5066
// _dd.p.tid contains the high 64 bits of the trace ID.
51-
auto high = parse_uint64(value, 16);
52-
if (auto* error = high.if_error()) {
53-
logger.log_error(
54-
error->with_prefix("Unable to parse high bits of the trace ID in "
55-
"Datadog style from the "
56-
"\"_dd.p.tid\" trace tag: "));
67+
const Optional<std::uint64_t> high = parse_trace_id_high(value);
68+
if (!high) {
5769
span_tags[tags::internal::propagation_error] = "malformed_tid " + value;
5870
continue;
5971
}
@@ -274,7 +286,7 @@ Span Tracer::create_span(const SpanConfig& config) {
274286
span_data->trace_id = generator_->trace_id(span_data->start);
275287
if (span_data->trace_id.high) {
276288
trace_tags.emplace_back(tags::internal::trace_id_high,
277-
hex(span_data->trace_id.high));
289+
hex_padded(span_data->trace_id.high));
278290
}
279291
span_data->span_id = span_data->trace_id.low;
280292
span_data->parent_id = 0;
@@ -401,21 +413,27 @@ Expected<Span> Tracer::extract_span(const DictReader& reader,
401413
//
402414
// First, though, if the `trace_id_high` tag is already set and has a bogus
403415
// value or a value inconsistent with the trace ID, tag an error.
404-
const std::string hex_high = hex(span_data->trace_id.high);
416+
const auto hex_high = hex_padded(span_data->trace_id.high);
405417
const auto extant = std::find_if(
406418
trace_tags.begin(), trace_tags.end(), [&](const auto& pair) {
407419
return pair.first == tags::internal::trace_id_high;
408420
});
409421
if (extant == trace_tags.end()) {
410422
trace_tags.emplace_back(tags::internal::trace_id_high, hex_high);
411-
} else if (!parse_uint64(extant->second, 16)) {
412-
span_data->tags[tags::internal::propagation_error] =
413-
"malformed_tid " + extant->second;
414-
extant->second = hex_high;
415-
} else if (extant->second != hex_high) {
416-
span_data->tags[tags::internal::propagation_error] =
417-
"inconsistent_tid " + extant->second;
418-
extant->second = hex_high;
423+
} else {
424+
// There is already a `trace_id_high` tag. `hex_high` is its proper value.
425+
// Check if the extant value is malformed or different from `hex_high`. In
426+
// either case, tag an error and overwrite the tag with `hex_high`.
427+
const Optional<std::uint64_t> high = parse_trace_id_high(extant->second);
428+
if (!high) {
429+
span_data->tags[tags::internal::propagation_error] =
430+
"malformed_tid " + extant->second;
431+
extant->second = hex_high;
432+
} else if (*high != span_data->trace_id.high) {
433+
span_data->tags[tags::internal::propagation_error] =
434+
"inconsistent_tid " + extant->second;
435+
extant->second = hex_high;
436+
}
419437
}
420438
}
421439

‎test/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ add_executable(tests
2323
test_glob.cpp
2424
test_limiter.cpp
2525
test_msgpack.cpp
26+
test_parse_util.cpp
2627
test_smoke.cpp
2728
test_span.cpp
2829
test_span_sampler.cpp

‎test/test_parse_util.cpp‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
#include <datadog/error.h>
2+
#include <datadog/parse_util.h>
3+
4+
#include <cassert>
5+
#include <cstdint>
6+
#include <limits>
7+
#include <string>
8+
#include <variant>
9+
10+
#include "test.h"
11+
12+
using namespace datadog::tracing;
13+
14+
TEST_CASE("parse_int") {
15+
struct TestCase {
16+
int line;
17+
std::string name;
18+
std::string argument;
19+
int base;
20+
std::variant<int, Error::Code> expected;
21+
};
22+
23+
// clang-format off
24+
auto test_case = GENERATE(values<TestCase>({
25+
{__LINE__, "zero (dec)", "0", 10, 0},
26+
{__LINE__, "zeros (dec)", "000", 10, 0},
27+
{__LINE__, "zero (hex)", "0", 16, 0},
28+
{__LINE__, "zeros (hex)", "000", 16, 0},
29+
{__LINE__, "leading whitespace (dec 1)", " 42", 10, Error::INVALID_INTEGER},
30+
{__LINE__, "leading whitespace (dec 2)", "\t42", 10, Error::INVALID_INTEGER},
31+
{__LINE__, "leading whitespace (dec 3)", "\n42", 10, Error::INVALID_INTEGER},
32+
{__LINE__, "trailing whitespace (dec 1)", "42 ", 10, Error::INVALID_INTEGER},
33+
{__LINE__, "trailing whitespace (dec 2)", "42\t", 10, Error::INVALID_INTEGER},
34+
{__LINE__, "trailing whitespace (dec 3)", "42\n", 10, Error::INVALID_INTEGER},
35+
{__LINE__, "leading whitespace (hex 1)", " 42", 16, Error::INVALID_INTEGER},
36+
{__LINE__, "leading whitespace (hex 2)", "\t42", 16, Error::INVALID_INTEGER},
37+
{__LINE__, "leading whitespace (hex 3)", "\n42", 16, Error::INVALID_INTEGER},
38+
{__LINE__, "trailing whitespace (hex 1)", "42 ", 16, Error::INVALID_INTEGER},
39+
{__LINE__, "trailing whitespace (hex 2)", "42\t", 16, Error::INVALID_INTEGER},
40+
{__LINE__, "trailing whitespace (hex 3)", "42\n", 16, Error::INVALID_INTEGER},
41+
{__LINE__, "no hex prefix", "0xbeef", 16, Error::INVALID_INTEGER},
42+
{__LINE__, "dec rejects hex", "42beef", 10, Error::INVALID_INTEGER},
43+
{__LINE__, "hex accepts hex", "42beef", 16, 0x42beef},
44+
{__LINE__, "no trailing nonsense (dec)", "42xyz", 10, Error::INVALID_INTEGER},
45+
{__LINE__, "no trailing nonsense (hex)", "42xyz", 16, Error::INVALID_INTEGER},
46+
{__LINE__, "no leading nonsense (dec)", "xyz42", 10, Error::INVALID_INTEGER},
47+
{__LINE__, "no leading nonsense (hex)", "xyz42", 16, Error::INVALID_INTEGER},
48+
{__LINE__, "overflow", std::to_string(std::numeric_limits<int>::max()) + "0", 10, Error::OUT_OF_RANGE_INTEGER},
49+
{__LINE__, "negative (dec)", "-10", 10, -10},
50+
{__LINE__, "negative (hex)", "-a", 16, -10},
51+
{__LINE__, "lower case", "a", 16, 10},
52+
{__LINE__, "upper case", "A", 16, 10},
53+
{__LINE__, "underflow", std::to_string(std::numeric_limits<int>::min()) + "0", 10, Error::OUT_OF_RANGE_INTEGER},
54+
}));
55+
// clang-format on
56+
57+
CAPTURE(test_case.line);
58+
CAPTURE(test_case.name);
59+
CAPTURE(test_case.argument);
60+
CAPTURE(test_case.base);
61+
62+
const auto result = parse_int(test_case.argument, test_case.base);
63+
if (std::holds_alternative<int>(test_case.expected)) {
64+
const int& expected = std::get<int>(test_case.expected);
65+
REQUIRE(result);
66+
REQUIRE(*result == expected);
67+
} else {
68+
assert(std::holds_alternative<Error::Code>(test_case.expected));
69+
const Error::Code& expected = std::get<Error::Code>(test_case.expected);
70+
REQUIRE(!result);
71+
REQUIRE(result.error().code == expected);
72+
}
73+
}
74+
75+
// This test case is similar to the one above, except that negative numbers are
76+
// not supported, and the underflow and overflow values are different.
77+
TEST_CASE("parse_uint64") {
78+
struct TestCase {
79+
int line;
80+
std::string name;
81+
std::string argument;
82+
int base;
83+
std::variant<std::uint64_t, Error::Code> expected;
84+
};
85+
86+
// clang-format off
87+
auto test_case = GENERATE(values<TestCase>({
88+
{__LINE__, "zero (dec)", "0", 10, UINT64_C(0)},
89+
{__LINE__, "zeros (dec)", "000", 10, UINT64_C(0)},
90+
{__LINE__, "zero (hex)", "0", 16, UINT64_C(0)},
91+
{__LINE__, "zeros (hex)", "000", 16, UINT64_C(0)},
92+
{__LINE__, "leading whitespace (dec 1)", " 42", 10, Error::INVALID_INTEGER},
93+
{__LINE__, "leading whitespace (dec 2)", "\t42", 10, Error::INVALID_INTEGER},
94+
{__LINE__, "leading whitespace (dec 3)", "\n42", 10, Error::INVALID_INTEGER},
95+
{__LINE__, "trailing whitespace (dec 1)", "42 ", 10, Error::INVALID_INTEGER},
96+
{__LINE__, "trailing whitespace (dec 2)", "42\t", 10, Error::INVALID_INTEGER},
97+
{__LINE__, "trailing whitespace (dec 3)", "42\n", 10, Error::INVALID_INTEGER},
98+
{__LINE__, "leading whitespace (hex 1)", " 42", 16, Error::INVALID_INTEGER},
99+
{__LINE__, "leading whitespace (hex 2)", "\t42", 16, Error::INVALID_INTEGER},
100+
{__LINE__, "leading whitespace (hex 3)", "\n42", 16, Error::INVALID_INTEGER},
101+
{__LINE__, "trailing whitespace (hex 1)", "42 ", 16, Error::INVALID_INTEGER},
102+
{__LINE__, "trailing whitespace (hex 2)", "42\t", 16, Error::INVALID_INTEGER},
103+
{__LINE__, "trailing whitespace (hex 3)", "42\n", 16, Error::INVALID_INTEGER},
104+
{__LINE__, "no hex prefix", "0xbeef", 16, Error::INVALID_INTEGER},
105+
{__LINE__, "dec rejects hex", "42beef", 10, Error::INVALID_INTEGER},
106+
{__LINE__, "hex accepts hex", "42beef", 16, UINT64_C(0x42beef)},
107+
{__LINE__, "no trailing nonsense (dec)", "42xyz", 10, Error::INVALID_INTEGER},
108+
{__LINE__, "no trailing nonsense (hex)", "42xyz", 16, Error::INVALID_INTEGER},
109+
{__LINE__, "no leading nonsense (dec)", "xyz42", 10, Error::INVALID_INTEGER},
110+
{__LINE__, "no leading nonsense (hex)", "xyz42", 16, Error::INVALID_INTEGER},
111+
{__LINE__, "overflow", std::to_string(std::numeric_limits<std::uint64_t>::max()) + "0", 10, Error::OUT_OF_RANGE_INTEGER},
112+
{__LINE__, "negative (dec)", "-10", 10, Error::INVALID_INTEGER},
113+
{__LINE__, "negative (hex)", "-a", 16, Error::INVALID_INTEGER},
114+
{__LINE__, "lower case", "a", 16, UINT64_C(10)},
115+
{__LINE__, "upper case", "A", 16, UINT64_C(10)},
116+
}));
117+
// clang-format on
118+
119+
CAPTURE(test_case.line);
120+
CAPTURE(test_case.name);
121+
CAPTURE(test_case.argument);
122+
CAPTURE(test_case.base);
123+
124+
const auto result = parse_uint64(test_case.argument, test_case.base);
125+
if (std::holds_alternative<std::uint64_t>(test_case.expected)) {
126+
const std::uint64_t& expected = std::get<std::uint64_t>(test_case.expected);
127+
REQUIRE(result);
128+
REQUIRE(*result == expected);
129+
} else {
130+
assert(std::holds_alternative<Error::Code>(test_case.expected));
131+
const Error::Code& expected = std::get<Error::Code>(test_case.expected);
132+
REQUIRE(!result);
133+
REQUIRE(result.error().code == expected);
134+
}
135+
}

‎test/test_tracer.cpp‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,11 +1042,30 @@ TEST_CASE("128-bit trace IDs") {
10421042
trace_id = span->trace_id();
10431043
}
10441044

1045+
SECTION("are, for W3C, extracted preferentially from traceparent") {
1046+
auto tid = GENERATE(values<std::string>({"decade", "deadbeefdeadbeed"}));
1047+
std::unordered_map<std::string, std::string> headers;
1048+
headers["traceparent"] =
1049+
"00-deadbeefdeadbeefcafebabecafebabe-0000000000000001-01";
1050+
// The _dd.p.tid value below is either malformed or inconsistent with the
1051+
// trace ID in the traceparent.
1052+
// It will be ignored, and the resulting _dd.p.tid value will be consistent
1053+
// with the higher part of the trace ID in traceparent: "deadbeefdeadbeef".
1054+
headers["tracestate"] = "dd=t.tid:" + tid;
1055+
MockDictReader reader{headers};
1056+
const auto span = tracer.extract_span(reader);
1057+
CAPTURE(logger->entries);
1058+
REQUIRE(logger->error_count() == 0);
1059+
REQUIRE(span);
1060+
REQUIRE(hex(span->trace_id().high) == "deadbeefdeadbeef");
1061+
trace_id = span->trace_id();
1062+
}
1063+
10451064
SECTION("are extracted from Datadog (_dd.p.tid)") {
10461065
std::unordered_map<std::string, std::string> headers;
10471066
headers["x-datadog-trace-id"] = "4";
10481067
headers["x-datadog-parent-id"] = "42";
1049-
headers["x-datadog-tags"] = "_dd.p.tid=beef";
1068+
headers["x-datadog-tags"] = "_dd.p.tid=000000000000beef";
10501069
MockDictReader reader{headers};
10511070
const auto span = tracer.extract_span(reader);
10521071
CAPTURE(logger->entries);
@@ -1094,7 +1113,9 @@ TEST_CASE(
10941113

10951114
auto test_case = GENERATE(values<TestCase>(
10961115
{{__LINE__, "invalid _dd.p.tid", "noodle", "malformed_tid "},
1097-
{__LINE__, "_dd.p.tid inconsistent with trace ID", "adfeed",
1116+
{__LINE__, "short _dd.p.tid", "beef", "malformed_tid "},
1117+
{__LINE__, "long _dd.p.tid", "000000000000000000beef", "malformed_tid "},
1118+
{__LINE__, "_dd.p.tid inconsistent with trace ID", "0000000000adfeed",
10981119
"inconsistent_tid "}}));
10991120

11001121
CAPTURE(test_case.line);

0 commit comments

Comments
 (0)