From 5e4e95d654d87633bd862f5fdfa58a489c9c6c30 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Thu, 16 Oct 2025 14:16:22 +0200 Subject: [PATCH 1/2] feat: send knuth sampling rates tags Changes: - Set `_dd.p.ksr` tag with the knuth sampling rates computed. - Remove remaining devs from sampling delegation. - Add unit tests. --- src/datadog/tags.cpp | 2 +- src/datadog/tags.h | 2 +- src/datadog/trace_segment.cpp | 12 +++++------ test/test_span.cpp | 40 ++++++++++++++++++++--------------- test/test_trace_segment.cpp | 22 ++++++++++++++----- test/test_tracer.cpp | 31 ++++++++++++++++++++++++--- 6 files changed, 75 insertions(+), 34 deletions(-) diff --git a/src/datadog/tags.cpp b/src/datadog/tags.cpp index 63d86f86..df73dec9 100644 --- a/src/datadog/tags.cpp +++ b/src/datadog/tags.cpp @@ -29,10 +29,10 @@ const std::string trace_id_high = "_dd.p.tid"; const std::string process_id = "process_id"; const std::string language = "language"; const std::string runtime_id = "runtime-id"; -const std::string sampling_decider = "_dd.is_sampling_decider"; const std::string w3c_parent_id = "_dd.parent_id"; const std::string trace_source = "_dd.p.ts"; const std::string apm_enabled = "_dd.apm.enabled"; +const std::string ksr = "_dd.p.ksr"; } // namespace internal diff --git a/src/datadog/tags.h b/src/datadog/tags.h index a6eb1751..9c7ea231 100644 --- a/src/datadog/tags.h +++ b/src/datadog/tags.h @@ -37,10 +37,10 @@ extern const std::string trace_id_high; extern const std::string process_id; extern const std::string language; extern const std::string runtime_id; -extern const std::string sampling_decider; extern const std::string w3c_parent_id; extern const std::string trace_source; // _dd.p.ts extern const std::string apm_enabled; // _dd.apm.enabled +extern const std::string ksr; // _dd.p.ksr } // namespace internal diff --git a/src/datadog/trace_segment.cpp b/src/datadog/trace_segment.cpp index cbf4e067..3cc9ebc8 100644 --- a/src/datadog/trace_segment.cpp +++ b/src/datadog/trace_segment.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -226,13 +227,6 @@ void TraceSegment::span_finished() { } } - if (decision.origin == SamplingDecision::Origin::DELEGATED && - local_root.parent_id == 0) { - // Convey the fact that, even though we are the root service, we delegated - // the sampling decision and so are not the "sampling decider." - local_root.tags[tags::internal::sampling_decider] = "0"; - } - // RFC seems to only mandate that this be set if the trace is kept. // However, system-tests expect this to be always be set. // Add it all the time; can't hurt @@ -292,6 +286,10 @@ void TraceSegment::make_sampling_decision_if_null() { sampling_decision_ = trace_sampler_->decide(local_root); update_decision_maker_trace_tag(); + + trace_tags_.emplace_back( + tags::internal::ksr, + std::to_string(*sampling_decision_->configured_rate)); } void TraceSegment::update_decision_maker_trace_tag() { diff --git a/test/test_span.cpp b/test/test_span.cpp index 5cbcb364..e49b374c 100644 --- a/test/test_span.cpp +++ b/test/test_span.cpp @@ -499,7 +499,9 @@ TEST_SPAN("injection") { REQUIRE(writer.items.empty() == false); // Consume the span that MUST be kept for service liveness. - { apm_disabled_tracer.create_span(); } + { + apm_disabled_tracer.create_span(); + } auto span = apm_disabled_tracer.create_span(); @@ -784,8 +786,8 @@ TEST_SPAN("injecting W3C tracestate header") { {"x-datadog-parent-id", "1"}, {"x-datadog-origin", "France"}, }, - // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;o:France"}, + // The "s:-1" and "t.ksr:0.000000" comes from the 0% sample rate. + "dd=s:-1;p:$parent_id;o:France;t.ksr:0.000000"}, {__LINE__, "trace tags", @@ -794,8 +796,8 @@ TEST_SPAN("injecting W3C tracestate header") { {"x-datadog-parent-id", "1"}, {"x-datadog-tags", "_dd.p.foo=x,_dd.p.bar=y,ignored=wrong_prefix"}, }, - // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;t.foo:x;t.bar:y"}, + // The "s:-1" and "t.ksr:0.000000" comes from the 0% sample rate. + "dd=s:-1;p:$parent_id;t.foo:x;t.bar:y;t.ksr:0.000000"}, {__LINE__, "extra fields", @@ -815,15 +817,18 @@ TEST_SPAN("injecting W3C tracestate header") { // The "s:0" comes from the sampling decision in `traceparent_drop`. "dd=s:0;p:$parent_id;o:France;t.foo:x;t.bar:y;foo:bar;boing:boing"}, - {__LINE__, - "replace invalid characters in origin", - { - {"x-datadog-trace-id", "1"}, - {"x-datadog-parent-id", "1"}, - {"x-datadog-origin", "France, is a country=nation; so is 台北."}, - }, - // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;o:France_ is a country~nation_ so is ______."}, + { + __LINE__, + "replace invalid characters in origin", + { + {"x-datadog-trace-id", "1"}, + {"x-datadog-parent-id", "1"}, + {"x-datadog-origin", "France, is a country=nation; so is 台北."}, + }, + // The "s:-1" comes from the 0% sample rate. + "dd=s:-1;p:$parent_id;o:France_ is a country~nation_ so is " + "______.;t.ksr:0.000000", + }, {__LINE__, "replace invalid characters in trace tag key", @@ -833,7 +838,7 @@ TEST_SPAN("injecting W3C tracestate header") { {"x-datadog-tags", "_dd.p.a;d台北x =foo,_dd.p.ok=bar"}, }, // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;t.a_d______x_:foo;t.ok:bar"}, + "dd=s:-1;p:$parent_id;t.a_d______x_:foo;t.ok:bar;t.ksr:0.000000"}, {__LINE__, "replace invalid characters in trace tag value", @@ -843,7 +848,8 @@ TEST_SPAN("injecting W3C tracestate header") { {"x-datadog-tags", "_dd.p.wacky=hello fr~d; how are คุณ?"}, }, // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;t.wacky:hello fr_d_ how are _________?"}, + "dd=s:-1;p:$parent_id;t.wacky:hello fr_d_ how are " + "_________?;t.ksr:0.000000"}, {__LINE__, "replace equal signs with tildes in trace tag value", @@ -853,7 +859,7 @@ TEST_SPAN("injecting W3C tracestate header") { {"x-datadog-tags", "_dd.p.base64_thingy=d2Fra2EhIHdhaw=="}, }, // The "s:-1" comes from the 0% sample rate. - "dd=s:-1;p:$parent_id;t.base64_thingy:d2Fra2EhIHdhaw~~"}, + "dd=s:-1;p:$parent_id;t.base64_thingy:d2Fra2EhIHdhaw~~;t.ksr:0.000000"}, {__LINE__, "oversized origin truncates it and subsequent fields", diff --git a/test/test_trace_segment.cpp b/test/test_trace_segment.cpp index c3f3a3fd..21ec41c3 100644 --- a/test/test_trace_segment.cpp +++ b/test/test_trace_segment.cpp @@ -236,7 +236,9 @@ TEST_CASE("TraceSegment finalization of spans") { {"x-datadog-sampling-priority", std::to_string(sampling_priority)}, }; MockDictReader reader{headers}; - { auto span = tracer.extract_span(reader); } + { + auto span = tracer.extract_span(reader); + } REQUIRE(collector->span_count() == 1); REQUIRE(collector->first_span().numeric_tags.at( tags::internal::sampling_priority) == sampling_priority); @@ -310,6 +312,7 @@ TEST_CASE("TraceSegment finalization of spans") { REQUIRE_THAT(span.tags, ContainsSubset(filtered)); // "_dd.p.dm" will be added, because we made a sampling decision. REQUIRE(span.tags.count("_dd.p.dm") == 1); + REQUIRE(span.tags.count("_dd.p.ksr") == 1); } SECTION("rate tags") { @@ -324,6 +327,7 @@ TEST_CASE("TraceSegment finalization of spans") { REQUIRE(collector->span_count() == 1); const auto& span = collector->first_span(); REQUIRE(span.numeric_tags.at(tags::internal::agent_sample_rate) == 1.0); + REQUIRE(span.tags.at(tags::internal::ksr) == "1.000000"); } SECTION( @@ -343,7 +347,11 @@ TEST_CASE("TraceSegment finalization of spans") { auto span = tracer.create_span(); (void)span; } - REQUIRE(collector_response->span_count() == 1); + { + REQUIRE(collector_response->span_count() == 1); + const auto& span = collector_response->first_span(); + CHECK(span.tags.at(tags::internal::ksr) == "1.000000"); + } collector_response->chunks.clear(); // Second trace will use the rate from `collector->response`. @@ -351,9 +359,12 @@ TEST_CASE("TraceSegment finalization of spans") { auto span = tracer.create_span(); (void)span; } - REQUIRE(collector_response->span_count() == 1); - const auto& span = collector_response->first_span(); - REQUIRE(span.numeric_tags.at(tags::internal::agent_sample_rate) == 1.0); + { + REQUIRE(collector_response->span_count() == 1); + const auto& span = collector_response->first_span(); + CHECK(span.numeric_tags.at(tags::internal::agent_sample_rate) == 1.0); + CHECK(span.tags.at(tags::internal::ksr) == "1.000000"); + } } SECTION("rules (implicit and explicit)") { @@ -383,6 +394,7 @@ TEST_CASE("TraceSegment finalization of spans") { const auto& span = collector->first_span(); REQUIRE(span.numeric_tags.at(tags::internal::rule_sample_rate) == sample_rate); + CHECK(span.tags.at(tags::internal::ksr) == std::to_string(sample_rate)); if (sample_rate == 1.0) { REQUIRE(span.numeric_tags.at( tags::internal::rule_limiter_sample_rate) == 1.0); diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index d5ff0cc2..8254cda2 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -995,11 +995,13 @@ TEST_TRACER("span extraction") { __LINE__, "origin, trace tags, parent, and extra fields", traceparent_drop, // traceparent - "dd=o:France;p:00000000000d69ac;t.foo:thing1;t.bar:thing2;x:wow;y:" + "dd=o:France;p:00000000000d69ac;t.ksr:0.728;t.foo:thing1;t.bar:" + "thing2;x:wow;y:" "wow", // tracestate 0, // expected_sampling_priority "France", // expected_origin { + {"_dd.p.ksr", "0.728"}, {"_dd.p.foo", "thing1"}, {"_dd.p.bar", "thing2"}, }, // expected_trace_tags @@ -1844,7 +1846,9 @@ TEST_TRACER("APM tracing disabled") { auto finalized_config = finalize_config(config, clock); REQUIRE(finalized_config); Tracer tracer{*finalized_config}; - { auto root1 = tracer.create_span(); } + { + auto root1 = tracer.create_span(); + } REQUIRE(collector->chunks.size() == 1); REQUIRE(collector->chunks.front().size() == 1); @@ -1923,7 +1927,9 @@ TEST_TRACER("APM tracing disabled") { // When APM Tracing is disabled, we allow one trace per second for service // liveness. To ensure consistency, consume the limiter slot. - { tracer.create_span(); } + { + tracer.create_span(); + } collector->chunks.clear(); // Case 1: extracted context with priority, but no `_dd.p.ts` → depends if @@ -2034,3 +2040,22 @@ TEST_TRACER("APM tracing disabled") { } } } + +TEST_TRACER("_dd.p.ksr is NOT set when overriding the sampling decision") { + const auto collector = std::make_shared(); + + TracerConfig config; + config.collector = collector; + auto finalized_config = finalize_config(config); + REQUIRE(finalized_config); + + Tracer tracer{*finalized_config}; + + { + auto span = tracer.create_span(); + span.trace_segment().override_sampling_priority(10); + } + + auto c = collector->first_span(); + CHECK(c.tags.count(tags::internal::ksr) == 0); +} From d501c4f12258bf6a84e9192022bb7222b65ae80f Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Mon, 20 Oct 2025 04:58:26 -0400 Subject: [PATCH 2/2] fix format --- test/test_span.cpp | 4 +--- test/test_trace_segment.cpp | 4 +--- test/test_tracer.cpp | 8 ++------ 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/test/test_span.cpp b/test/test_span.cpp index e49b374c..bce0d27d 100644 --- a/test/test_span.cpp +++ b/test/test_span.cpp @@ -499,9 +499,7 @@ TEST_SPAN("injection") { REQUIRE(writer.items.empty() == false); // Consume the span that MUST be kept for service liveness. - { - apm_disabled_tracer.create_span(); - } + { apm_disabled_tracer.create_span(); } auto span = apm_disabled_tracer.create_span(); diff --git a/test/test_trace_segment.cpp b/test/test_trace_segment.cpp index 21ec41c3..bf7e6108 100644 --- a/test/test_trace_segment.cpp +++ b/test/test_trace_segment.cpp @@ -236,9 +236,7 @@ TEST_CASE("TraceSegment finalization of spans") { {"x-datadog-sampling-priority", std::to_string(sampling_priority)}, }; MockDictReader reader{headers}; - { - auto span = tracer.extract_span(reader); - } + { auto span = tracer.extract_span(reader); } REQUIRE(collector->span_count() == 1); REQUIRE(collector->first_span().numeric_tags.at( tags::internal::sampling_priority) == sampling_priority); diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 8254cda2..d0a52260 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -1846,9 +1846,7 @@ TEST_TRACER("APM tracing disabled") { auto finalized_config = finalize_config(config, clock); REQUIRE(finalized_config); Tracer tracer{*finalized_config}; - { - auto root1 = tracer.create_span(); - } + { auto root1 = tracer.create_span(); } REQUIRE(collector->chunks.size() == 1); REQUIRE(collector->chunks.front().size() == 1); @@ -1927,9 +1925,7 @@ TEST_TRACER("APM tracing disabled") { // When APM Tracing is disabled, we allow one trace per second for service // liveness. To ensure consistency, consume the limiter slot. - { - tracer.create_span(); - } + { tracer.create_span(); } collector->chunks.clear(); // Case 1: extracted context with priority, but no `_dd.p.ts` → depends if