From ad4a1a098ea80213faa58ced95df2470d69f099f Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Thu, 3 Oct 2024 14:40:07 -0700 Subject: [PATCH 1/3] test: Add unit test to assert that when a span has an overridden service name that does not match the tracer default, then no "version" tag is added --- test/test_tracer.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index dd2ef684..61c6f59a 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -90,6 +90,18 @@ TEST_CASE("tracer span defaults") { REQUIRE(overrides.name != config.name); REQUIRE(overrides.tags != config.tags); + // Test behaviors when the config overrides the service but leaves other + // fields empty + SpanConfig overrides_with_empty_values; + overrides_with_empty_values.service = "barsvc"; + + REQUIRE(overrides_with_empty_values.service != config.service); + REQUIRE(overrides_with_empty_values.service_type != config.service_type); + REQUIRE(overrides_with_empty_values.environment != config.environment); + REQUIRE(overrides_with_empty_values.version != config.version); + REQUIRE(overrides_with_empty_values.name != config.name); + REQUIRE(overrides_with_empty_values.tags != config.tags); + // Some of the sections below create a span from extracted trace context. const std::unordered_map headers{ {"x-datadog-trace-id", "123"}, {"x-datadog-parent-id", "456"}}; @@ -117,6 +129,9 @@ TEST_CASE("tracer span defaults") { REQUIRE(root.version() == config.version); REQUIRE(root.name == config.name); REQUIRE_THAT(root.tags, ContainsSubset(*config.tags)); + + REQUIRE(root.tags.count(tags::version) == 1); + REQUIRE(root.tags.find(tags::version)->second == config.version); } SECTION("can be overridden in a root span") { @@ -141,6 +156,9 @@ TEST_CASE("tracer span defaults") { REQUIRE(root.version() == overrides.version); REQUIRE(root.name == overrides.name); REQUIRE_THAT(root.tags, ContainsSubset(overrides.tags)); + + REQUIRE(root.tags.count(tags::version) == 1); + REQUIRE(root.tags.find(tags::version)->second == overrides.version); } SECTION("are honored in an extracted span") { @@ -165,6 +183,9 @@ TEST_CASE("tracer span defaults") { REQUIRE(span.version() == config.version); REQUIRE(span.name == config.name); REQUIRE_THAT(span.tags, ContainsSubset(*config.tags)); + + REQUIRE(span.tags.count(tags::version) == 1); + REQUIRE(span.tags.find(tags::version)->second == config.version); } SECTION("can be overridden in an extracted span") { @@ -189,6 +210,9 @@ TEST_CASE("tracer span defaults") { REQUIRE(span.version() == overrides.version); REQUIRE(span.name == overrides.name); REQUIRE_THAT(span.tags, ContainsSubset(overrides.tags)); + + REQUIRE(span.tags.count(tags::version) == 1); + REQUIRE(span.tags.find(tags::version)->second == overrides.version); } SECTION("are honored in a child span") { @@ -216,6 +240,9 @@ TEST_CASE("tracer span defaults") { REQUIRE(child.version() == config.version); REQUIRE(child.name == config.name); REQUIRE_THAT(child.tags, ContainsSubset(*config.tags)); + + REQUIRE(child.tags.count(tags::version) == 1); + REQUIRE(child.tags.find(tags::version)->second == config.version); } SECTION("can be overridden in a child span") { @@ -243,6 +270,40 @@ TEST_CASE("tracer span defaults") { REQUIRE(child.version() == overrides.version); REQUIRE(child.name == overrides.name); REQUIRE_THAT(child.tags, ContainsSubset(overrides.tags)); + + REQUIRE(child.tags.count(tags::version) == 1); + REQUIRE(child.tags.find(tags::version)->second == overrides.version); + } + + SECTION("can be overridden in a child span with empty values") { + { + auto parent = tracer.create_span(); + auto child = parent.create_child(overrides_with_empty_values); + (void)child; + } + REQUIRE(logger->error_count() == 0); + + // Get the finished span from the collector and verify that its + // properties have the configured default values. + REQUIRE(collector->chunks.size() == 1); + const auto& chunk = collector->chunks.front(); + // One span for the parent, and another for the child. + REQUIRE(chunk.size() == 2); + // The parent will be first, so the child is last. + auto& child_ptr = chunk.back(); + REQUIRE(child_ptr); + const auto& child = *child_ptr; + + REQUIRE(child.service == + overrides_with_empty_values.service); // only service is set + REQUIRE(child.service_type == config.service_type); + REQUIRE(child.environment() == config.environment); + REQUIRE(child.version() == nullopt); // version is not inherited since the + // service name is different + REQUIRE(child.name == config.name); + REQUIRE_THAT(child.tags, ContainsSubset(*config.tags)); + + REQUIRE(child.tags.count(tags::version) == 0); } } From c544d033175bf2a361a32ab077b47856ef0da81d Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Wed, 2 Oct 2024 16:04:38 -0700 Subject: [PATCH 2/3] feat: Implement consistency for "version" tag - Apply version tag to a span only if its service name matches the default service name Issues: APMAPI-517 --- src/datadog/span_data.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/datadog/span_data.cpp b/src/datadog/span_data.cpp index b1e1216f..b06b50c0 100644 --- a/src/datadog/span_data.cpp +++ b/src/datadog/span_data.cpp @@ -47,7 +47,8 @@ void SpanData::apply_config(const SpanDefaults& defaults, if (!environment.empty()) { tags.insert_or_assign(tags::environment, environment); } - std::string version = config.version.value_or(defaults.version); + std::string version = config.version.value_or( + service.compare(defaults.service) == 0 ? defaults.version : ""); if (!version.empty()) { tags.insert_or_assign(tags::version, version); } From ba34e04970e02b49ce1f90824351b98c79c6d58f Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Sun, 6 Oct 2024 11:19:22 -0400 Subject: [PATCH 3/3] code review from dmehala - Make the logic used to apply `version` tag more explicit. - Removed unused variable in test. --- src/datadog/span_data.cpp | 20 ++++++++++++++------ test/test_tracer.cpp | 3 +-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/datadog/span_data.cpp b/src/datadog/span_data.cpp index b06b50c0..7ed4ebb2 100644 --- a/src/datadog/span_data.cpp +++ b/src/datadog/span_data.cpp @@ -37,7 +37,19 @@ Optional SpanData::version() const { void SpanData::apply_config(const SpanDefaults& defaults, const SpanConfig& config, const Clock& clock) { - service = config.service.value_or(defaults.service); + std::string version; + if (config.service) { + service = *config.service; + version = config.version.value_or(""); + } else { + service = defaults.service; + version = defaults.version; + } + + if (!version.empty()) { + tags.insert_or_assign(tags::version, version); + } + name = config.name.value_or(defaults.name); for (const auto& item : defaults.tags) { @@ -47,11 +59,7 @@ void SpanData::apply_config(const SpanDefaults& defaults, if (!environment.empty()) { tags.insert_or_assign(tags::environment, environment); } - std::string version = config.version.value_or( - service.compare(defaults.service) == 0 ? defaults.version : ""); - if (!version.empty()) { - tags.insert_or_assign(tags::version, version); - } + for (const auto& [key, value] : config.tags) { tags.insert_or_assign(key, value); } diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 61c6f59a..98d855cf 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -278,8 +278,7 @@ TEST_CASE("tracer span defaults") { SECTION("can be overridden in a child span with empty values") { { auto parent = tracer.create_span(); - auto child = parent.create_child(overrides_with_empty_values); - (void)child; + parent.create_child(overrides_with_empty_values); } REQUIRE(logger->error_count() == 0);