[SDK] Enforce Span Status transition rules (#4547)

Author: denizariyan

CHANGELOG.md

@@ -325,6 +325,18 @@ Increment the:
   metric exporter
   [#4365](https://github.com/open-telemetry/opentelemetry-cpp/pull/4365)
 
+* [SDK] Enforce span status transition rules defined in the OpenTelemetry
+  specification in `Span::SetStatus`.
+  [#4546](https://github.com/open-telemetry/opentelemetry-cpp/issues/4546)
+  * Previously every `SetStatus` call overwrote the recorded status. It now
+    follows the specification, which changes behavior for users in three ways:
+    * A call with `StatusCode::kUnset` is ignored.
+    * Once the status is `kOk`, it is final. So a later call can no longer
+      change an explicit `kOk`.
+    * The description is kept only for `kError`; it is dropped for `kOk` and
+      `kUnset`.
+  * `ETWSpan::SetStatus` in the ETW exporter follows the same rules.
+
 Important changes:
 
 * [API] Never set a null global provider or propagator

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h

@@ -35,6 +35,7 @@
 #include "opentelemetry/sdk/common/empty_attributes.h"
 #include "opentelemetry/sdk/trace/exporter.h"
 #include "opentelemetry/sdk/trace/samplers/always_on.h"
+#include "opentelemetry/sdk/trace/span_status.h"
 
 #include "opentelemetry/exporters/etw/etw_config.h"
 #include "opentelemetry/exporters/etw/etw_fields.h"
@@ -952,8 +953,15 @@ class Span : public opentelemetry::trace::Span
   void SetStatus(opentelemetry::trace::StatusCode code,
                  nostd::string_view description) noexcept override
   {
-    status_code_        = code;
-    status_description_ = description.data();
+    const auto transition =
+        sdk::trace::detail::ApplyStatusTransition(status_code_, code, description);
+    if (!transition.accepted)
+    {
+      return;
+    }
+
+    status_code_ = transition.code;
+    status_description_.assign(transition.description.data(), transition.description.size());
   }
 
   opentelemetry::trace::StatusCode GetStatus() { return status_code_; }

exporters/etw/test/etw_tracer_test.cc

@@ -4,6 +4,7 @@
 #ifdef _WIN32
 
 #  include <gtest/gtest.h>
+#  include <cstdint>
 #  include <map>
 #  include <string>
 #  include "opentelemetry//sdk/trace/sampler.h"
@@ -580,6 +581,86 @@ TEST(ETWTracer, EndWithCustomTime)
 
 }
 
+/**
+ * Reads back the status fields the ETW exporter puts on the "Span" event at End.
+ */
+static void GetEmittedStatus(opentelemetry::trace::Span &span, uint32_t &code, std::string &message)
+{
+  exporter::etw::Properties evt;
+  static_cast<exporter::etw::Span &>(span).UpdateStatus(evt);
+  const exporter::etw::PropertyVariant &code_value = evt[ETW_FIELD_STATUSCODE];
+  const exporter::etw::PropertyVariant &message_value = evt[ETW_FIELD_STATUSMESSAGE];
+  code = nostd::get<uint32_t>(code_value);
+  message = nostd::get<std::string>(message_value);
+}
+
+TEST(ETWTracer, SpanSetStatusOkIsFinal)
+{
+  exporter::etw::TracerProvider tp;
+  auto tracer = tp.GetTracer("SpanSetStatus");
+  auto span = tracer->StartSpan("span 1");
+  span->SetStatus(opentelemetry::trace::StatusCode::kOk, "");
+  span->SetStatus(opentelemetry::trace::StatusCode::kError, "later error");
+  span->SetStatus(opentelemetry::trace::StatusCode::kUnset, "");
+
+  uint32_t code = 0;
+  std::string message;
+  GetEmittedStatus(*span, code, message);
+  EXPECT_EQ(code, static_cast<uint32_t>(opentelemetry::trace::StatusCode::kOk));
+  EXPECT_EQ(message, "");
+  span->End();
+}
+
+TEST(ETWTracer, SpanSetStatusUnsetIsIgnored)
+{
+  exporter::etw::TracerProvider tp;
+  auto tracer = tp.GetTracer("SpanSetStatus");
+  auto span = tracer->StartSpan("span 1");
+  span->SetStatus(opentelemetry::trace::StatusCode::kError, "the real failure");
+  span->SetStatus(opentelemetry::trace::StatusCode::kUnset, "");
+
+  uint32_t code = 0;
+  std::string message;
+  GetEmittedStatus(*span, code, message);
+  EXPECT_EQ(code, static_cast<uint32_t>(opentelemetry::trace::StatusCode::kError));
+  EXPECT_EQ(message, "the real failure");
+  span->End();
+}
+
+TEST(ETWTracer, SpanSetStatusDescriptionIgnoredForOk)
+{
+  exporter::etw::TracerProvider tp;
+  auto tracer = tp.GetTracer("SpanSetStatus");
+  auto span = tracer->StartSpan("span 1");
+  span->SetStatus(opentelemetry::trace::StatusCode::kError, "transient");
+  span->SetStatus(opentelemetry::trace::StatusCode::kOk, "ignored description");
+
+  uint32_t code = 0;
+  std::string message;
+  GetEmittedStatus(*span, code, message);
+  EXPECT_EQ(code, static_cast<uint32_t>(opentelemetry::trace::StatusCode::kOk));
+  EXPECT_EQ(message, "");
+  span->End();
+}
+
+TEST(ETWTracer, SpanSetStatusErrorKeepsDescription)
+{
+  exporter::etw::TracerProvider tp;
+  auto tracer = tp.GetTracer("SpanSetStatus");
+  auto span = tracer->StartSpan("span 1");
+  // The description is not null terminated at its end, so a copy through data() alone overruns it.
+  std::string description = "boomX";
+  span->SetStatus(opentelemetry::trace::StatusCode::kError,
+                  nostd::string_view(description.data(), 4));
+
+  uint32_t code = 0;
+  std::string message;
+  GetEmittedStatus(*span, code, message);
+  EXPECT_EQ(code, static_cast<uint32_t>(opentelemetry::trace::StatusCode::kError));
+  EXPECT_EQ(message, "boom");
+  span->End();
+}
+
 TEST(ETWTracer, ConstructorInitializesToOpenState)
 {
   opentelemetry::exporter::etw::TracerProvider provider;

exporters/otlp/src/otlp_recordable.cc

@@ -213,6 +213,10 @@ void OtlpRecordable::SetStatus(trace::StatusCode code, nostd::string_view descri
   {
     span_.mutable_status()->set_message(description.data(), description.size());
   }
+  else
+  {
+    span_.mutable_status()->clear_message();
+  }
 }
 
 void OtlpRecordable::SetName(nostd::string_view name) noexcept

exporters/otlp/test/otlp_recordable_test.cc

@@ -190,6 +190,18 @@ TEST(OtlpRecordable, SetStatus)
   EXPECT_EQ(rec2.span().status().message(), "");
 }
 
+TEST(OtlpRecordable, SetStatusClearsStaleMessage)
+{
+  OtlpRecordable rec;
+  rec.SetStatus(trace::StatusCode::kError, "boom");
+  EXPECT_EQ(rec.span().status().message(), "boom");
+
+  rec.SetStatus(trace::StatusCode::kOk, "");
+  EXPECT_EQ(rec.span().status().code(),
+            proto::trace::v1::Status_StatusCode(trace::StatusCode::kOk));
+  EXPECT_EQ(rec.span().status().message(), "");
+}
+
 TEST(OtlpRecordable, SetTraceFlags)
 {
   OtlpRecordable rec;

exporters/zipkin/src/recordable.cc

@@ -215,6 +215,10 @@ void Recordable::SetStatus(trace::StatusCode code, nostd::string_view descriptio
     {
       span_["tags"]["error"] = description;
     }
+    else
+    {
+      span_["tags"].erase("error");
+    }
   }
 }
 

exporters/zipkin/test/zipkin_recordable_test.cc

@@ -156,6 +156,19 @@ TEST(ZipkinSpanRecordable, SetStatus)
   }
 }
 
+TEST(ZipkinSpanRecordable, SetStatusClearsStaleErrorTag)
+{
+  zipkin::Recordable rec;
+
+  rec.SetStatus(trace::StatusCode::kError, "boom");
+  EXPECT_EQ(rec.span()["tags"]["error"], "boom");
+
+  rec.SetStatus(trace::StatusCode::kOk, "");
+  const json j_span = {{"tags", {{"otel.status_code", trace::StatusCode::kOk}}}};
+  EXPECT_EQ(rec.span(), j_span);
+  EXPECT_FALSE(rec.span()["tags"].contains("error"));
+}
+
 TEST(ZipkinSpanRecordable, SetSpanKind)
 {
   json j_json_client = {{"kind", "CLIENT"}};

sdk/include/opentelemetry/sdk/trace/span_status.h

@@ -0,0 +1,62 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#pragma once
+
+#include "opentelemetry/nostd/string_view.h"
+#include "opentelemetry/trace/span_metadata.h"
+#include "opentelemetry/version.h"
+
+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace sdk
+{
+namespace trace
+{
+namespace detail
+{
+
+/**
+ * Outcome of applying the span status transition rules to a SetStatus call.
+ */
+struct StatusTransition
+{
+  /// Whether the call changes the status. When false, the SetStatus call must be ignored.
+  bool accepted{false};
+
+  /// Status code to record.
+  opentelemetry::trace::StatusCode code{opentelemetry::trace::StatusCode::kUnset};
+
+  /// Description to record.
+  nostd::string_view description{""};
+};
+
+/**
+ * @brief Applies the span status transition rules from the trace API specification.
+ *
+ * @param current Status code recorded so far, kUnset if SetStatus was never called.
+ * @param code Status code passed to SetStatus.
+ * @param description Description passed to SetStatus.
+ */
+inline StatusTransition ApplyStatusTransition(opentelemetry::trace::StatusCode current,
+                                              opentelemetry::trace::StatusCode code,
+                                              nostd::string_view description) noexcept
+{
+  const auto is_already_ok{current == opentelemetry::trace::StatusCode::kOk};
+  const auto is_unset{code == opentelemetry::trace::StatusCode::kUnset};
+
+  if (is_already_ok || is_unset)
+  {
+    return StatusTransition{false, current, nostd::string_view{""}};
+  }
+
+  const auto keep_description{(code == opentelemetry::trace::StatusCode::kError) &&
+                              !description.empty()};
+  const auto effective_description{keep_description ? description : nostd::string_view{""}};
+
+  return StatusTransition{true, code, effective_description};
+}
+
+}  // namespace detail
+}  // namespace trace
+}  // namespace sdk
+OPENTELEMETRY_END_NAMESPACE

sdk/src/trace/span.cc

@@ -8,6 +8,7 @@
 #include "opentelemetry/nostd/function_ref.h"
 #include "opentelemetry/sdk/trace/processor.h"
 #include "opentelemetry/sdk/trace/recordable.h"
+#include "opentelemetry/sdk/trace/span_status.h"
 #include "opentelemetry/trace/span_id.h"
 #include "opentelemetry/trace/span_metadata.h"
 #include "opentelemetry/trace/trace_flags.h"
@@ -201,7 +202,15 @@ void Span::SetStatus(opentelemetry::trace::StatusCode code, nostd::string_view d
   {
     return;
   }
-  recordable_->SetStatus(code, description);
+
+  const auto transition = detail::ApplyStatusTransition(status_code_, code, description);
+  if (!transition.accepted)
+  {
+    return;
+  }
+
+  status_code_ = transition.code;
+  recordable_->SetStatus(transition.code, transition.description);
 }
 
 void Span::UpdateName(nostd::string_view name) noexcept

sdk/src/trace/span.h

@@ -16,6 +16,7 @@
 #include "opentelemetry/trace/span.h"
 #include "opentelemetry/trace/span_context.h"
 #include "opentelemetry/trace/span_context_kv_iterable.h"
+#include "opentelemetry/trace/span_metadata.h"
 #include "opentelemetry/trace/span_startoptions.h"
 #include "opentelemetry/version.h"
 
@@ -83,6 +84,7 @@ class Span final : public opentelemetry::trace::Span
   std::unique_ptr<Recordable> recordable_;
   opentelemetry::common::SteadyTimestamp start_steady_time;
   opentelemetry::trace::SpanContext span_context_;
+  opentelemetry::trace::StatusCode status_code_{opentelemetry::trace::StatusCode::kUnset};
   bool has_ended_{false};
 };
 }  // namespace trace