From 298d4e27e26138d8a97b135023ba8022ffed2c5e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 5 Jul 2023 15:50:19 -0400 Subject: [PATCH 01/11] don't propagate invalid _dd.p.tid --- src/datadog/tracer.cpp | 6 ++++-- test/test_tracer.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 109ee5fe..7b34278d 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -55,10 +55,12 @@ void handle_trace_tags(StringView trace_tags, ExtractedData& result, "Datadog style from the " "\"_dd.p.tid\" trace tag: ")); span_tags[tags::internal::propagation_error] = "decoding_error"; + continue; } - // Note that this assumes the lower 64 bits of the trace ID have already - // been extracted (i.e. we look for X-Datadog-Trace-ID first). + if (result.trace_id) { + // Note that this assumes the lower 64 bits of the trace ID have already + // been extracted (i.e. we look for X-Datadog-Trace-ID first). result.trace_id->high = *high; } } diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 600ea618..588aea3e 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -24,6 +24,7 @@ #include "matchers.h" #include "mocks/collectors.h" #include "mocks/dict_readers.h" +#include "mocks/dict_writers.h" #include "mocks/loggers.h" #include "test.h" @@ -927,6 +928,33 @@ TEST_CASE("span extraction") { headers["x-datadog-tags"] = header_value; REQUIRE(tracer.extract_span(reader)); } + + SECTION("_dd.p.tid is not propagated when it is invalid") { + const std::string header_value = + "_dd.p.foobar=hello,_dd.p.tid=invalidhex"; + REQUIRE(decode_tags(header_value)); + headers["x-datadog-tags"] = header_value; + + auto maybe_span = tracer.extract_span(reader); + REQUIRE(maybe_span); + auto& span = *maybe_span; + + MockDictWriter writer; + span.inject(writer); + // Expect a valid "x-datadog-tags" header, and it will contain + // "_dd.p.foobar", but not "_dd.p.tid". + REQUIRE(writer.items.count("x-datadog-tags") == 1); + const std::string& injected_header_value = + writer.items.find("x-datadog-tags")->second; + const auto decoded_tags = decode_tags(injected_header_value); + REQUIRE(decoded_tags); + CAPTURE(*decoded_tags); + const std::unordered_multimap tags{ + decoded_tags->begin(), decoded_tags->end()}; + REQUIRE(tags.count("_dd.p.foobar") == 1); + REQUIRE(tags.find("_dd.p.foobar")->second == "hello"); + REQUIRE(tags.count("_dd.p.tid") == 0); + } } } From 89596876b47675939d85f83d07025683660f3805 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 5 Jul 2023 16:45:44 -0400 Subject: [PATCH 02/11] "_dd.propagation_error:malformed_tid xxxxx" --- src/datadog/tracer.cpp | 2 +- test/test_tracer.cpp | 58 +++++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 7b34278d..56a7c2a8 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -54,7 +54,7 @@ void handle_trace_tags(StringView trace_tags, ExtractedData& result, error->with_prefix("Unable to parse high bits of the trace ID in " "Datadog style from the " "\"_dd.p.tid\" trace tag: ")); - span_tags[tags::internal::propagation_error] = "decoding_error"; + span_tags[tags::internal::propagation_error] = "malformed_tid " + value; continue; } diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 588aea3e..c6cc343a 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -929,31 +929,49 @@ TEST_CASE("span extraction") { REQUIRE(tracer.extract_span(reader)); } - SECTION("_dd.p.tid is not propagated when it is invalid") { + SECTION("invalid _dd.p.tid") { const std::string header_value = "_dd.p.foobar=hello,_dd.p.tid=invalidhex"; REQUIRE(decode_tags(header_value)); headers["x-datadog-tags"] = header_value; - auto maybe_span = tracer.extract_span(reader); - REQUIRE(maybe_span); - auto& span = *maybe_span; - - MockDictWriter writer; - span.inject(writer); - // Expect a valid "x-datadog-tags" header, and it will contain - // "_dd.p.foobar", but not "_dd.p.tid". - REQUIRE(writer.items.count("x-datadog-tags") == 1); - const std::string& injected_header_value = - writer.items.find("x-datadog-tags")->second; - const auto decoded_tags = decode_tags(injected_header_value); - REQUIRE(decoded_tags); - CAPTURE(*decoded_tags); - const std::unordered_multimap tags{ - decoded_tags->begin(), decoded_tags->end()}; - REQUIRE(tags.count("_dd.p.foobar") == 1); - REQUIRE(tags.find("_dd.p.foobar")->second == "hello"); - REQUIRE(tags.count("_dd.p.tid") == 0); + SECTION("is not propagated") { + auto maybe_span = tracer.extract_span(reader); + REQUIRE(maybe_span); + auto& span = *maybe_span; + + MockDictWriter writer; + span.inject(writer); + // Expect a valid "x-datadog-tags" header, and it will contain + // "_dd.p.foobar", but not "_dd.p.tid". + REQUIRE(writer.items.count("x-datadog-tags") == 1); + const std::string& injected_header_value = + writer.items.find("x-datadog-tags")->second; + const auto decoded_tags = decode_tags(injected_header_value); + REQUIRE(decoded_tags); + CAPTURE(*decoded_tags); + const std::unordered_multimap tags{ + decoded_tags->begin(), decoded_tags->end()}; + REQUIRE(tags.count("_dd.p.foobar") == 1); + REQUIRE(tags.find("_dd.p.foobar")->second == "hello"); + REQUIRE(tags.count("_dd.p.tid") == 0); + } + + SECTION("is noted in an error tag value") { + { + auto maybe_span = tracer.extract_span(reader); + REQUIRE(maybe_span); + } + // Now that the span is destroyed, it will have been sent to the + // collector. + // We can inspect the `SpanData` in the collector to verify that the + // `tags::internal::propagation_error` ("_dd.propagation_error") tag + // is set to the expected value. + const SpanData& span = collector->first_span(); + REQUIRE(span.tags.count(tags::internal::propagation_error) == 1); + REQUIRE(span.tags.find(tags::internal::propagation_error)->second == + "malformed_tid invalidhex"); + } } } } From f42066c9662c894f625250b5b9a51e2791a9f24a Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 14:06:26 -0400 Subject: [PATCH 03/11] 128-bit trace IDs contain unix timestamp --- src/datadog/id_generator.cpp | 11 +++++++++-- src/datadog/id_generator.h | 5 ++++- src/datadog/tracer.cpp | 2 +- test/test_span.cpp | 8 ++++---- test/test_tracer.cpp | 30 +++++++++++++++++++++++------- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/datadog/id_generator.cpp b/src/datadog/id_generator.cpp index b418ec8f..eb3b517c 100644 --- a/src/datadog/id_generator.cpp +++ b/src/datadog/id_generator.cpp @@ -1,6 +1,7 @@ #include "id_generator.h" #include +#include #include "random.h" @@ -15,11 +16,17 @@ class DefaultIDGenerator : public IDGenerator { explicit DefaultIDGenerator(bool trace_id_128_bit) : trace_id_128_bit_(trace_id_128_bit) {} - TraceID trace_id() const override { + TraceID trace_id(const TimePoint& start) const override { TraceID result; result.low = random_uint64(); if (trace_id_128_bit_) { - result.high = random_uint64(); + // Highest 32 bits contain a unix timestamp (the trace start time). + const auto since_epoch = start.wall.time_since_epoch(); + const auto seconds = + std::chrono::duration_cast(since_epoch).count(); + // The farthest we'll go back is the unix epoch. + const std::uint64_t unsigned_seconds = seconds < 0 ? 0 : seconds; + result.high = unsigned_seconds << 32; } else { // In 64-bit mode, zero the most significant bit for compatibility with // older tracers that can't accept values above diff --git a/src/datadog/id_generator.h b/src/datadog/id_generator.h index 32ffa2ec..834e8bf2 100644 --- a/src/datadog/id_generator.h +++ b/src/datadog/id_generator.h @@ -12,6 +12,7 @@ #include #include +#include "clock.h" #include "trace_id.h" namespace datadog { @@ -21,8 +22,10 @@ class IDGenerator { public: virtual ~IDGenerator() = default; + // Generate a span ID. virtual std::uint64_t span_id() const = 0; - virtual TraceID trace_id() const = 0; + // Generate a trace ID for a trace having the specified `start` time. + virtual TraceID trace_id(const TimePoint& start) const = 0; }; std::shared_ptr default_id_generator(bool trace_id_128_bit); diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 56a7c2a8..bb6b2f3f 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -271,7 +271,7 @@ Span Tracer::create_span(const SpanConfig& config) { auto span_data = std::make_unique(); span_data->apply_config(*defaults_, config, clock_); std::vector> trace_tags; - span_data->trace_id = generator_->trace_id(); + span_data->trace_id = generator_->trace_id(span_data->start); if (span_data->trace_id.high) { trace_tags.emplace_back(tags::internal::trace_id_high, hex(span_data->trace_id.high)); diff --git a/test/test_span.cpp b/test/test_span.cpp index e3948c7d..a20b1bc4 100644 --- a/test/test_span.cpp +++ b/test/test_span.cpp @@ -373,7 +373,7 @@ TEST_CASE("injection") { struct Generator : public IDGenerator { const std::uint64_t id; explicit Generator(std::uint64_t id) : id(id) {} - TraceID trace_id() const override { return TraceID(id); } + TraceID trace_id(const TimePoint&) const override { return TraceID(id); } std::uint64_t span_id() const override { return id; } }; Tracer tracer{*finalized_config, std::make_shared(42)}; @@ -478,7 +478,7 @@ TEST_CASE("injecting W3C traceparent header") { struct Generator : public IDGenerator { const std::uint64_t id; explicit Generator(std::uint64_t id) : id(id) {} - TraceID trace_id() const override { return TraceID(id); } + TraceID trace_id(const TimePoint&) const override { return TraceID(id); } std::uint64_t span_id() const override { return id; } }; Tracer tracer{*finalized_config, @@ -515,7 +515,7 @@ TEST_CASE("injecting W3C traceparent header") { struct Generator : public IDGenerator { const std::uint64_t id; explicit Generator(std::uint64_t id) : id(id) {} - TraceID trace_id() const override { return TraceID(id); } + TraceID trace_id(const TimePoint&) const override { return TraceID(id); } std::uint64_t span_id() const override { return id; } }; Tracer tracer{*finalized_config, std::make_shared(expected_id)}; @@ -715,7 +715,7 @@ TEST_CASE("128-bit trace ID injection") { public: explicit MockIDGenerator(TraceID trace_id) : trace_id_(trace_id) {} - TraceID trace_id() const override { return trace_id_; } + TraceID trace_id(const TimePoint&) const override { return trace_id_; } // `span_id` won't be called, because root spans use the lower part of // `trace_id` for the span ID. std::uint64_t span_id() const override { return 42; } diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index c6cc343a..36a7d378 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -19,6 +19,8 @@ #include #include +#include +#include #include #include "matchers.h" @@ -998,7 +1000,16 @@ TEST_CASE("report hostname") { } } -TEST_CASE("create 128-bit trace IDs") { +TEST_CASE("128-bit trace IDs") { + // Use a clock that always returns a hard-coded `TimePoint`. + // May 6, 2010 14:45:13 America/New_York + const std::time_t flash_crash = 1273171513; + const Clock clock = [flash_crash]() { + TimePoint result; + result.wall = std::chrono::system_clock::from_time_t(flash_crash); + return result; + }; + TracerConfig config; config.defaults.service = "testsvc"; config.trace_id_128_bit = true; @@ -1012,12 +1023,17 @@ TEST_CASE("create 128-bit trace IDs") { config.extraction_styles.push_back(PropagationStyle::B3); const auto finalized = finalize_config(config); REQUIRE(finalized); - Tracer tracer{*finalized}; + Tracer tracer{*finalized, clock}; SECTION("are generated") { + // Specifically, verify that the high 64 bits of the generated trace ID + // contain the unix start time of the trace shifted up 32 bits. + // + // Due to the definition of `clock`, above, that unix time will be + // `flash_crash`. const auto span = tracer.create_span(); - // The chance that it's zero is ~2**(-64), which I'm willing to neglect. - REQUIRE(span.trace_id().high != 0); + const std::uint64_t expected = std::uint64_t(flash_crash) << 32; + REQUIRE(span.trace_id().high == expected); } SECTION("result in _dd.p.tid trace tag being sent to collector") { @@ -1037,7 +1053,7 @@ TEST_CASE("create 128-bit trace IDs") { REQUIRE(*high == generated_id.high); } - SECTION("extracted from W3C") { + SECTION("are extracted from W3C") { std::unordered_map headers; headers["traceparent"] = "00-deadbeefdeadbeefcafebabecafebabe-0000000000000001-01"; @@ -1049,7 +1065,7 @@ TEST_CASE("create 128-bit trace IDs") { REQUIRE(hex(span->trace_id().high) == "deadbeefdeadbeef"); } - SECTION("extracted from Datadog (_dd.p.tid)") { + SECTION("are extracted from Datadog (_dd.p.tid)") { std::unordered_map headers; headers["x-datadog-trace-id"] = "4"; headers["x-datadog-parent-id"] = "42"; @@ -1063,7 +1079,7 @@ TEST_CASE("create 128-bit trace IDs") { "000000000000beef0000000000000004"); } - SECTION("extracted from B3") { + SECTION("are extracted from B3") { std::unordered_map headers; headers["x-b3-traceid"] = "deadbeefdeadbeefcafebabecafebabe"; headers["x-b3-spanid"] = "42"; From caa0fb02f2187a7bbb50d5be83be30a8a1f99233 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 14:43:24 -0400 Subject: [PATCH 04/11] always include _dd.p.tid with 128-bit trace IDs --- src/datadog/tracer.cpp | 5 +++++ test/test_tracer.cpp | 34 +++++++++++++++++----------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index bb6b2f3f..65e517c7 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -394,6 +394,11 @@ Expected Tracer::extract_span(const DictReader& reader, span_data->trace_id = *trace_id; span_data->parent_id = *parent_id; + if (span_data->trace_id.high) { + trace_tags.emplace_back(tags::internal::trace_id_high, + hex(span_data->trace_id.high)); + } + Optional sampling_decision; if (sampling_priority) { SamplingDecision decision; diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 36a7d378..a0b84728 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -1024,6 +1024,7 @@ TEST_CASE("128-bit trace IDs") { const auto finalized = finalize_config(config); REQUIRE(finalized); Tracer tracer{*finalized, clock}; + TraceID trace_id; // used below the following SECTIONs SECTION("are generated") { // Specifically, verify that the high 64 bits of the generated trace ID @@ -1034,23 +1035,7 @@ TEST_CASE("128-bit trace IDs") { const auto span = tracer.create_span(); const std::uint64_t expected = std::uint64_t(flash_crash) << 32; REQUIRE(span.trace_id().high == expected); - } - - SECTION("result in _dd.p.tid trace tag being sent to collector") { - TraceID generated_id; - { - const auto span = tracer.create_span(); - generated_id = span.trace_id(); - } - CAPTURE(logger->entries); - REQUIRE(logger->error_count() == 0); - REQUIRE(collector->span_count() == 1); - const auto& span = collector->first_span(); - const auto found = span.tags.find(tags::internal::trace_id_high); - REQUIRE(found != span.tags.end()); - const auto high = parse_uint64(found->second, 16); - REQUIRE(high); - REQUIRE(*high == generated_id.high); + trace_id = span.trace_id(); } SECTION("are extracted from W3C") { @@ -1063,6 +1048,7 @@ TEST_CASE("128-bit trace IDs") { REQUIRE(logger->error_count() == 0); REQUIRE(span); REQUIRE(hex(span->trace_id().high) == "deadbeefdeadbeef"); + trace_id = span->trace_id(); } SECTION("are extracted from Datadog (_dd.p.tid)") { @@ -1077,6 +1063,7 @@ TEST_CASE("128-bit trace IDs") { REQUIRE(span); REQUIRE(span->trace_id().hex_padded() == "000000000000beef0000000000000004"); + trace_id = span->trace_id(); } SECTION("are extracted from B3") { @@ -1089,5 +1076,18 @@ TEST_CASE("128-bit trace IDs") { REQUIRE(logger->error_count() == 0); REQUIRE(span); REQUIRE(hex(span->trace_id().high) == "deadbeefdeadbeef"); + trace_id = span->trace_id(); } + + // For any 128-bit trace ID, the _dd.p.tid trace tag is always sent to the + // collector. + CAPTURE(logger->entries); + REQUIRE(logger->error_count() == 0); + REQUIRE(collector->span_count() == 1); + const auto& span = collector->first_span(); + const auto found = span.tags.find(tags::internal::trace_id_high); + REQUIRE(found != span.tags.end()); + const auto high = parse_uint64(found->second, 16); + REQUIRE(high); + REQUIRE(*high == trace_id.high); } From 4c286ea1167d93612abfacd269b40d3f0d9a567e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 15:00:48 -0400 Subject: [PATCH 05/11] manual.keep and manual.drop are not special --- src/datadog/tags.cpp | 2 -- src/datadog/tags.h | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/datadog/tags.cpp b/src/datadog/tags.cpp index 3d6c8052..6a02d569 100644 --- a/src/datadog/tags.cpp +++ b/src/datadog/tags.cpp @@ -11,8 +11,6 @@ const std::string service_name = "service.name"; const std::string span_type = "span.type"; const std::string operation_name = "operation"; const std::string resource_name = "resource.name"; -const std::string manual_keep = "manual.keep"; -const std::string manual_drop = "manual.drop"; const std::string version = "version"; namespace internal { diff --git a/src/datadog/tags.h b/src/datadog/tags.h index 4ce6b04e..f2b8d12b 100644 --- a/src/datadog/tags.h +++ b/src/datadog/tags.h @@ -16,8 +16,6 @@ extern const std::string service_name; extern const std::string span_type; extern const std::string operation_name; extern const std::string resource_name; -extern const std::string manual_keep; -extern const std::string manual_drop; extern const std::string version; namespace internal { From 729f72bf15860df3e5b56be7dccaede2310ddfce Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 15:44:13 -0400 Subject: [PATCH 06/11] note when _dd.p.tid disagrees with trace ID --- src/datadog/tracer.cpp | 20 ++++++++++++++-- src/datadog/w3c_propagation.cpp | 6 ----- test/test_tracer.cpp | 42 ++++++++++++++++++++++++++------- 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 65e517c7..bfaec510 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -395,8 +395,24 @@ Expected Tracer::extract_span(const DictReader& reader, span_data->parent_id = *parent_id; if (span_data->trace_id.high) { - trace_tags.emplace_back(tags::internal::trace_id_high, - hex(span_data->trace_id.high)); + // The trace ID has some bits set in the higher 64 bits. Set the + // corresponding `trace_id_high` tag, so that the Datadog backend is aware + // of those bits. + // + // First, though, if the `trace_id_high` tag is already set and has a value + // inconsistent with the trace ID, tag an error. + const std::string hex_high = hex(span_data->trace_id.high); + const auto extant = std::find_if( + trace_tags.begin(), trace_tags.end(), [&](const auto& pair) { + return pair.first == tags::internal::trace_id_high; + }); + if (extant == trace_tags.end()) { + trace_tags.emplace_back(tags::internal::trace_id_high, hex_high); + } else if (extant->second != hex_high) { + span_data->tags[tags::internal::propagation_error] = + "inconsistent_tid " + extant->second; + extant->second = hex_high; + } } Optional sampling_decision; diff --git a/src/datadog/w3c_propagation.cpp b/src/datadog/w3c_propagation.cpp index afa265b5..2957f735 100644 --- a/src/datadog/w3c_propagation.cpp +++ b/src/datadog/w3c_propagation.cpp @@ -214,12 +214,6 @@ void parse_datadog_tracestate(ExtractedData& result, StringView datadog_value) { const auto tag_suffix = key.substr(2); std::string tag_name = "_dd.p."; append(tag_name, tag_suffix); - // The "_dd.p.tid" trace tag is ignored in this context, since its value - // is better inferred from the higher 64 bits of the trace ID. - if (tag_name == "_dd.p.tid") { - pair_begin = pair_end == end ? end : pair_end + 1; - continue; - } // The tag value was encoded with all '=' replaced by '~'. Undo that // transformation. std::string decoded_value{value}; diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index a0b84728..3e95ab94 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -807,15 +807,6 @@ TEST_CASE("span extraction") { nullopt, // expected_additional_w3c_tracestate "x:wow;y:wow"}, // expected_additional_datadog_w3c_tracestate - {__LINE__, "_dd.p.tid trace tag is ignored", - traceparent_drop, // traceparent - "dd=t.tid:deadbeef;t.foo:bar", // tracestate - 0, // expected_sampling_priority - nullopt, // expected_origin - {{"_dd.p.foo", "bar"}}, // expected_trace_tags - nullopt, // expected_additional_w3c_tracestate - nullopt}, // expected_additional_datadog_w3c_tracestate - {__LINE__, "traceparent and tracestate sampling agree (1/4)", traceparent_drop, // traceparent "dd=s:0", // tracestate @@ -1091,3 +1082,36 @@ TEST_CASE("128-bit trace IDs") { REQUIRE(high); REQUIRE(*high == trace_id.high); } + +TEST_CASE("_dd.p.tid inconsistent with trace ID results in error tag") { + TracerConfig config; + config.defaults.service = "testsvc"; + config.trace_id_128_bit = true; + const auto collector = std::make_shared(); + config.collector = collector; + const auto logger = std::make_shared(); + config.logger = logger; + config.extraction_styles.clear(); + config.extraction_styles.push_back(PropagationStyle::W3C); + const auto finalized = finalize_config(config); + REQUIRE(finalized); + Tracer tracer{*finalized}; + + std::unordered_map headers; + headers["traceparent"] = + "00-deadbeefdeadbeefcafebabecafebabe-0000000000000001-01"; + headers["tracestate"] = "dd=t.tid:adfeed"; + MockDictReader reader{headers}; + CAPTURE(logger->entries); + { + const auto span = tracer.extract_span(reader); + REQUIRE(span); + } + + REQUIRE(logger->error_count() == 0); + REQUIRE(collector->span_count() == 1); + const auto& span = collector->first_span(); + const auto found = span.tags.find(tags::internal::propagation_error); + REQUIRE(found != span.tags.end()); + REQUIRE(found->second == "inconsistent_tid adfeed"); +} From 98ce7a5b8a90a65f6cf7e44dc81390dd48251a31 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 16:06:58 -0400 Subject: [PATCH 07/11] note when _dd.p.tid cannot be parsed --- src/datadog/tracer.cpp | 8 ++++++-- test/test_tracer.cpp | 20 +++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index bfaec510..26c489a1 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -399,8 +399,8 @@ Expected Tracer::extract_span(const DictReader& reader, // corresponding `trace_id_high` tag, so that the Datadog backend is aware // of those bits. // - // First, though, if the `trace_id_high` tag is already set and has a value - // inconsistent with the trace ID, tag an error. + // First, though, if the `trace_id_high` tag is already set and has a bogus + // value or a value inconsistent with the trace ID, tag an error. const std::string hex_high = hex(span_data->trace_id.high); const auto extant = std::find_if( trace_tags.begin(), trace_tags.end(), [&](const auto& pair) { @@ -408,6 +408,10 @@ Expected Tracer::extract_span(const DictReader& reader, }); if (extant == trace_tags.end()) { trace_tags.emplace_back(tags::internal::trace_id_high, hex_high); + } else if (!parse_uint64(extant->second, 16)) { + span_data->tags[tags::internal::propagation_error] = + "malformed_tid " + extant->second; + extant->second = hex_high; } else if (extant->second != hex_high) { span_data->tags[tags::internal::propagation_error] = "inconsistent_tid " + extant->second; diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 3e95ab94..1e498fde 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -1083,7 +1083,20 @@ TEST_CASE("128-bit trace IDs") { REQUIRE(*high == trace_id.high); } -TEST_CASE("_dd.p.tid inconsistent with trace ID results in error tag") { +TEST_CASE( + "_dd.p.tid invalid or inconsistent with trace ID results in error tag") { + struct TestCase { + int line; + std::string name; + std::string tid_tag_value; + std::string expected_error_prefix; + }; + + auto test_case = GENERATE(values( + {{__LINE__, "invalid _dd.p.tid", "noodle", "malformed_tid "}, + {__LINE__, "_dd.p.tid inconsistent with trace ID", "adfeed", + "inconsistent_tid "}})); + TracerConfig config; config.defaults.service = "testsvc"; config.trace_id_128_bit = true; @@ -1100,7 +1113,7 @@ TEST_CASE("_dd.p.tid inconsistent with trace ID results in error tag") { std::unordered_map headers; headers["traceparent"] = "00-deadbeefdeadbeefcafebabecafebabe-0000000000000001-01"; - headers["tracestate"] = "dd=t.tid:adfeed"; + headers["tracestate"] = "dd=t.tid:" + test_case.tid_tag_value; MockDictReader reader{headers}; CAPTURE(logger->entries); { @@ -1113,5 +1126,6 @@ TEST_CASE("_dd.p.tid inconsistent with trace ID results in error tag") { const auto& span = collector->first_span(); const auto found = span.tags.find(tags::internal::propagation_error); REQUIRE(found != span.tags.end()); - REQUIRE(found->second == "inconsistent_tid adfeed"); + REQUIRE(found->second == + test_case.expected_error_prefix + test_case.tid_tag_value); } From 3d0e453ef9705ca4423ee0b9a07c2b7935f7c9bd Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 16:32:46 -0400 Subject: [PATCH 08/11] work around the poorly named "-Wunused-lambda-capture" --- test/test_tracer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 1e498fde..a99516c8 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -995,7 +995,7 @@ TEST_CASE("128-bit trace IDs") { // Use a clock that always returns a hard-coded `TimePoint`. // May 6, 2010 14:45:13 America/New_York const std::time_t flash_crash = 1273171513; - const Clock clock = [flash_crash]() { + const Clock clock = []() { TimePoint result; result.wall = std::chrono::system_clock::from_time_t(flash_crash); return result; From 1b6c4fbb544e4d3f9e5121c67f60bb5af66b2f56 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 16:43:07 -0400 Subject: [PATCH 09/11] capture unit test case context --- test/test_tracer.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index a99516c8..6cb3764e 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -1097,6 +1097,9 @@ TEST_CASE( {__LINE__, "_dd.p.tid inconsistent with trace ID", "adfeed", "inconsistent_tid "}})); + CAPTURE(test_case.line); + CAPTURE(test_case.name); + TracerConfig config; config.defaults.service = "testsvc"; config.trace_id_128_bit = true; From 08433f955d94692f298bea02b4f8a0998978c4b7 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 17:24:19 -0400 Subject: [PATCH 10/11] work around a spurious compiler warning --- CMakeLists.txt | 4 ++++ test/test_tracer.cpp | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8fe4e9eb..d208aa48 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -21,6 +21,10 @@ set(CMAKE_CXX_EXTENSIONS OFF) # . add_compile_options(-Wno-error=free-nonheap-object) +# This warning has a false positive with clang. See +# . +add_compile_options(-Wno-error=unused-lambda-capture) + # If we're building with clang, then use the libc++ version of the standard # library instead of libstdc++. Better coverage of build configurations. # diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 6cb3764e..d8bdb293 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -995,7 +995,7 @@ TEST_CASE("128-bit trace IDs") { // Use a clock that always returns a hard-coded `TimePoint`. // May 6, 2010 14:45:13 America/New_York const std::time_t flash_crash = 1273171513; - const Clock clock = []() { + const Clock clock = [flash_crash]() { TimePoint result; result.wall = std::chrono::system_clock::from_time_t(flash_crash); return result; From 7c1144c63882a85831d4d81be6761ba24488738c Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 6 Jul 2023 18:30:05 -0400 Subject: [PATCH 11/11] Permit unused-lambda-capture on Clang only. It doesn't exist in GCC. --- CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d208aa48..7471f1aa 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,7 +23,9 @@ add_compile_options(-Wno-error=free-nonheap-object) # This warning has a false positive with clang. See # . -add_compile_options(-Wno-error=unused-lambda-capture) +if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang") + add_compile_options(-Wno-error=unused-lambda-capture) +endif() # If we're building with clang, then use the libc++ version of the standard # library instead of libstdc++. Better coverage of build configurations.