datadog: fix span error property

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",

source/extensions/tracers/datadog/span.cc

@@ -2,6 +2,7 @@
 
 #include <utility>
 
+#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,28 @@ 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) {
+    // Envoy marks spans as containing errors by setting the "error" tag.
+    // Here we translate into the dd-trace-cpp equivalent.
+    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);
   }

test/extensions/tracers/datadog/span_test.cc

@@ -5,6 +5,7 @@
 #include <utility>
 #include <vector>
 
+#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"
@@ -138,7 +139,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 +155,112 @@ 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. 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
+//   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(Tags.Error));
+  ASSERT_EQ(0, data.tags.count("error.message"));
+  ASSERT_EQ(0, data.tags.count(Tags.ErrorReason));
+}
+
+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(Tags.Error));
+  ASSERT_EQ(0, data.tags.count("error.message"));
+  ASSERT_EQ(0, data.tags.count(Tags.ErrorReason));
+}
+
+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" 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) {
   Span span{std::move(span_)};