From f1d74fdb82ca549f1379eb56415776177afb900c Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 2 Jan 2024 22:36:35 -0500 Subject: [PATCH 1/4] write some failing unit tests Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 99 +++++++++++++++++++- 1 file changed, 95 insertions(+), 4 deletions(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 436685b36f3a0..62994e2a55cd3 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -5,6 +5,7 @@ #include #include +#include "source/common/tracing/common_values.h" #include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/span.h" #include "source/extensions/tracers/datadog/time_util.h" @@ -13,6 +14,7 @@ #include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" +#include "absl/types/optional.h" #include "datadog/clock.h" #include "datadog/collector.h" #include "datadog/expected.h" @@ -138,7 +140,6 @@ TEST_F(DatadogTracerSpanTest, SetTag) { span.setTag("foo", "bar"); span.setTag("boom", "bam"); span.setTag("foo", "new"); - span.setTag("resource.name", "vespene gas"); span.finishSpan(); ASSERT_EQ(1, collector_->chunks.size()); @@ -155,14 +156,104 @@ TEST_F(DatadogTracerSpanTest, SetTag) { found = data.tags.find("boom"); ASSERT_NE(data.tags.end(), found); EXPECT_EQ("bam", found->second); +} + +TEST_F(DatadogTracerSpanTest, SetTagResourceName) { + // The "resource.name" tag is special. It doesn't set a tag, but instead sets + // the span's resource name. + + Span span{std::move(span_)}; + span.setTag("resource.name", "vespene gas"); + span.finishSpan(); - // The "resource.name" tag is special. It doesn't set a tag, but instead the - // span's resource name. - found = data.tags.find("resource.name"); + ASSERT_EQ(1, collector_->chunks.size()); + const auto& chunk = collector_->chunks[0]; + ASSERT_EQ(1, chunk.size()); + const auto& data_ptr = chunk[0]; + ASSERT_NE(nullptr, data_ptr); + const datadog::tracing::SpanData& data = *data_ptr; + + const auto found = data.tags.find("resource.name"); ASSERT_EQ(data.tags.end(), found); EXPECT_EQ("vespene gas", data.resource); } +// The "error" and "error.reason" tags are special. +// +// - The "error" tag is only ever set to "true", and doing so indicates that +// an error occurred during the extent of the span. The corresponding notion +// for a Datadog span is to call `.set_error(true)`, and the result is that +// the underlying Datadog span's `error` property will be `1`. +// - The "error.reason" tag is set to some description of the kind of error +// that occurred. It's debatable whether this more closely corresponds to +// Datadog's `.set_error_message(...)` or to `.set_error_type(...)`, but this +// library chooses `.set_error_message(...)`, which has the result of setting +// the "error.message" tag. +// - Note that calling `.set_error_message(...)` causes `.set_error(true)` to +// be called. However, it might be possible for Envoy to set the +// "error.reason" tag without also setting the "error" tag. This library +// chooses to treat all "error.reason" as if they imply a corresponding +// "error", i.e. setting "error.reason" without "error" still implies an +// error. + +TEST_F(DatadogTracerSpanTest, SetTagError) { + Span span{std::move(span_)}; + const auto& Tags = Envoy::Tracing::Tags::get(); + span.setTag(Tags.Error, Tags.True); + span.finishSpan(); + + ASSERT_EQ(1, collector_->chunks.size()); + const auto& chunk = collector_->chunks[0]; + ASSERT_EQ(1, chunk.size()); + const auto& data_ptr = chunk[0]; + ASSERT_NE(nullptr, data_ptr); + const datadog::tracing::SpanData& data = *data_ptr; + + ASSERT_TRUE(data.error); + ASSERT_EQ(0, data.tags.count("error.message")); +} + +TEST_F(DatadogTracerSpanTest, SetTagErrorBogus) { + Span span{std::move(span_)}; + const auto& Tags = Envoy::Tracing::Tags::get(); + // `Tags.True`, which is "true", is the only value accepted for the + // `Tags.Error` ("error") tag. All others are ignored. + span.setTag(Tags.Error, Tags.True); + span.setTag(Tags.Error, "false"); + span.setTag(Tags.Error, "supercalifragilisticexpialidocious"); + span.finishSpan(); + + ASSERT_EQ(1, collector_->chunks.size()); + const auto& chunk = collector_->chunks[0]; + ASSERT_EQ(1, chunk.size()); + const auto& data_ptr = chunk[0]; + ASSERT_NE(nullptr, data_ptr); + const datadog::tracing::SpanData& data = *data_ptr; + + ASSERT_TRUE(data.error); + ASSERT_EQ(0, data.tags.count("error.message")); +} + +TEST_F(DatadogTracerSpanTest, SetTagErrorReason) { + Span span{std::move(span_)}; + const auto& Tags = Envoy::Tracing::Tags::get(); + span.setTag(Tags.ErrorReason, "not enough minerals"); + span.finishSpan(); + + ASSERT_EQ(1, collector_->chunks.size()); + const auto& chunk = collector_->chunks[0]; + ASSERT_EQ(1, chunk.size()); + const auto& data_ptr = chunk[0]; + ASSERT_NE(nullptr, data_ptr); + const datadog::tracing::SpanData& data = *data_ptr; + + // In addition to setting the "error.message" tag, we also have + // `.error == true`. + ASSERT_TRUE(data.error); + ASSERT_EQ(1, data.tags.count("error.message")); + ASSERT_EQ("not enough minerals", data.tags.at("error.message")); +} + TEST_F(DatadogTracerSpanTest, InjectContext) { Span span{std::move(span_)}; From c20311f9284e21eb5244dc45a23ebf52a4d2a2cb Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 2 Jan 2024 22:49:56 -0500 Subject: [PATCH 2/4] special "error" and "error.reason" tags Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 1 + source/extensions/tracers/datadog/span.cc | 24 +++++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 5fa018ef5c9a3..da0862389706e 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -43,6 +43,7 @@ envoy_cc_library( deps = [ "//source/common/config:utility_lib", "//source/common/http:async_client_utility_lib", + "//source/common/tracing:common_values_lib", "//source/common/tracing:null_span_lib", "//source/common/tracing:trace_context_lib", "//source/common/upstream:cluster_update_tracker_lib", diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index 38646f5cf8444..b6b805f26726b 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -2,6 +2,7 @@ #include +#include "source/common/tracing/common_values.h" #include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/time_util.h" @@ -51,14 +52,25 @@ void Span::setTag(absl::string_view name, absl::string_view value) { return; } - // The special "resource.name" tag is a holdover from when the Datadog tracer - // was OpenTracing-based, and so there was no way to set the Datadog resource - // name directly. - // In Envoy, it's still the case that there's no way to set the Datadog - // resource name directly; so, here if the tag name is "resource.name", we - // actually set the resource name instead of setting a tag. + const auto& Tags = Envoy::Tracing::Tags::get(); + if (name == "resource.name") { + // The special "resource.name" tag is a holdover from when the Datadog + // tracer was OpenTracing-based, and so there was no way to set the Datadog + // resource name directly. + // In Envoy, it's still the case that there's no way to set the Datadog + // resource name directly; so, here if the tag name is "resource.name", we + // actually set the resource name instead of setting a tag. span_->set_resource_name(value); + } else if (name == Tags.Error && value == Tags.True) { + // Envoy marks spans as containing errors by setting the "error" tag. + // Here we translate into the dd-trace-cpp equivalent. + span_->set_error(true); + } else if (name == Tags.ErrorReason) { + // Envoy conveys information about an error by setting the "error.reason" + // tag. + // Here we translate into the dd-trace-cpp equivalent. + span_->set_error_message(value); } else { span_->set_tag(name, value); } From b56eb5b18af21d085a28b7361d23d84e35e09256 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 2 Jan 2024 23:24:08 -0500 Subject: [PATCH 3/4] remove unnecessary include Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 62994e2a55cd3..4c4b3a6facf44 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -14,7 +14,6 @@ #include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" -#include "absl/types/optional.h" #include "datadog/clock.h" #include "datadog/collector.h" #include "datadog/expected.h" From 9d0ccef077d4a1c31838fc0348fd1753519057f7 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 3 Jan 2024 05:54:47 -0500 Subject: [PATCH 4/4] ensure that "error.reason" tag is set, and "error" tag is not set Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/span.cc | 7 +++++-- test/extensions/tracers/datadog/span_test.cc | 14 +++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index b6b805f26726b..bc52a58ab8b2d 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -62,15 +62,18 @@ void Span::setTag(absl::string_view name, absl::string_view value) { // resource name directly; so, here if the tag name is "resource.name", we // actually set the resource name instead of setting a tag. span_->set_resource_name(value); - } else if (name == Tags.Error && value == Tags.True) { + } else if (name == Tags.Error) { // Envoy marks spans as containing errors by setting the "error" tag. // Here we translate into the dd-trace-cpp equivalent. - span_->set_error(true); + if (value == Tags.True) { + span_->set_error(true); + } } else if (name == Tags.ErrorReason) { // Envoy conveys information about an error by setting the "error.reason" // tag. // Here we translate into the dd-trace-cpp equivalent. span_->set_error_message(value); + span_->set_tag(name, value); } else { span_->set_tag(name, value); } diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 4c4b3a6facf44..b7c3740ebe251 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -187,7 +187,8 @@ TEST_F(DatadogTracerSpanTest, SetTagResourceName) { // that occurred. It's debatable whether this more closely corresponds to // Datadog's `.set_error_message(...)` or to `.set_error_type(...)`, but this // library chooses `.set_error_message(...)`, which has the result of setting -// the "error.message" tag. +// the "error.message" tag. The "error.reason" tag is also set to the same +// value. // - Note that calling `.set_error_message(...)` causes `.set_error(true)` to // be called. However, it might be possible for Envoy to set the // "error.reason" tag without also setting the "error" tag. This library @@ -209,7 +210,9 @@ TEST_F(DatadogTracerSpanTest, SetTagError) { const datadog::tracing::SpanData& data = *data_ptr; ASSERT_TRUE(data.error); + ASSERT_EQ(0, data.tags.count(Tags.Error)); ASSERT_EQ(0, data.tags.count("error.message")); + ASSERT_EQ(0, data.tags.count(Tags.ErrorReason)); } TEST_F(DatadogTracerSpanTest, SetTagErrorBogus) { @@ -230,7 +233,9 @@ TEST_F(DatadogTracerSpanTest, SetTagErrorBogus) { const datadog::tracing::SpanData& data = *data_ptr; ASSERT_TRUE(data.error); + ASSERT_EQ(0, data.tags.count(Tags.Error)); ASSERT_EQ(0, data.tags.count("error.message")); + ASSERT_EQ(0, data.tags.count(Tags.ErrorReason)); } TEST_F(DatadogTracerSpanTest, SetTagErrorReason) { @@ -246,11 +251,14 @@ TEST_F(DatadogTracerSpanTest, SetTagErrorReason) { ASSERT_NE(nullptr, data_ptr); const datadog::tracing::SpanData& data = *data_ptr; - // In addition to setting the "error.message" tag, we also have - // `.error == true`. + // In addition to setting the "error.message" and "error.reason" tags, we also + // have `.error == true`. But still there is no "error" tag. ASSERT_TRUE(data.error); + ASSERT_EQ(0, data.tags.count(Tags.Error)); ASSERT_EQ(1, data.tags.count("error.message")); ASSERT_EQ("not enough minerals", data.tags.at("error.message")); + ASSERT_EQ(1, data.tags.count(Tags.ErrorReason)); + ASSERT_EQ("not enough minerals", data.tags.at(Tags.ErrorReason)); } TEST_F(DatadogTracerSpanTest, InjectContext) {