Apply the specified status precedence in Span.set_status

.changelog/5662.fixed

@@ -0,0 +1 @@
+`opentelemetry-sdk`: apply the specified status precedence in `Span.set_status` so `Ok > Error > Unset` is enforced explicitly, a `Unset` status is always ignored, and a bare `Status(ERROR)` no longer drops a description already recorded

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -76,6 +76,14 @@
 
 logger = logging.getLogger(__name__)
 
+# The specification orders the status codes ``Ok > Error > Unset``. The enum's
+# own values do not carry that order, so it is stated explicitly here.
+_STATUS_PRECEDENCE = {
+    StatusCode.UNSET: 0,
+    StatusCode.ERROR: 1,
+    StatusCode.OK: 2,
+}
+
 _DEFAULT_OTEL_ATTRIBUTE_COUNT_LIMIT = 128
 _DEFAULT_OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT = 128
 _DEFAULT_OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT = 128
@@ -983,21 +991,48 @@ def set_status(
         status: Status | StatusCode,
         description: str | None = None,
     ) -> None:
-        # Ignore future calls if status is already set to OK
-        # Ignore calls to set to StatusCode.UNSET
         if isinstance(status, Status):
-            if self._status and self._status.status_code is StatusCode.OK or status.status_code is StatusCode.UNSET:
-                return
             if description is not None:
                 logger.warning(
                     "Description %s ignored. Use either `Status` or `(StatusCode, Description)`",
                     description,
                 )
-            self._status = status
+            new_status = status
         elif isinstance(status, StatusCode):
-            if self._status and self._status.status_code is StatusCode.OK or status is StatusCode.UNSET:
-                return
-            self._status = Status(status, description)
+            new_status = Status(status, description)
+        else:
+            return
+
+        if self._accepts_status(new_status):
+            self._status = new_status
+
+    def _accepts_status(self, new_status: Status) -> bool:
+        """Decide whether ``new_status`` may replace the status already recorded.
+
+        The specification gives the status codes a total order, ``Ok > Error >
+        Unset``, says an attempt to set ``Unset`` should be ignored, and says
+        the value of the last call is the one recorded. So a code that ranks
+        below the one already recorded is dropped, ``Ok`` is final, and within
+        the same code the later call wins.
+        """
+        if new_status.status_code is StatusCode.UNSET:
+            return False
+
+        current = self._status
+        if current is None:
+            return True
+        if current.status_code is StatusCode.OK:
+            return False
+
+        current_rank = _STATUS_PRECEDENCE[current.status_code]
+        new_rank = _STATUS_PRECEDENCE[new_status.status_code]
+        if new_rank != current_rank:
+            return new_rank > current_rank
+
+        # Same code, so the last call wins, with one exception: a bare status
+        # carries no new information, and letting it through would drop a
+        # description already recorded in favour of nothing.
+        return new_status.description is not None
 
     def __exit__(
         self,

opentelemetry-sdk/tests/trace/test_trace.py

@@ -1350,6 +1350,93 @@ def error_status_test(context):
         error_status_test(trace.TracerProvider().get_tracer(__name__).start_span("root"))
         error_status_test(trace.TracerProvider().get_tracer(__name__).start_as_current_span("root"))
 
+    # --- status precedence -------------------------------------------------
+    # The spec orders the codes Ok > Error > Unset, says an attempt to set
+    # Unset should be ignored, and says the last call is the one recorded.
+
+    @staticmethod
+    def _span():
+        return trace.TracerProvider().get_tracer(__name__).start_span("root")
+
+    def test_unset_is_ignored_from_unset(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.UNSET, None))
+        self.assertIs(span.status.status_code, StatusCode.UNSET)
+
+    def test_unset_is_ignored_over_error(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "boom"))
+        span.set_status(trace_api.status.Status(StatusCode.UNSET))
+        self.assertIs(span.status.status_code, StatusCode.ERROR)
+        self.assertEqual(span.status.description, "boom")
+
+    def test_unset_is_ignored_over_ok(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        span.set_status(trace_api.status.Status(StatusCode.UNSET))
+        self.assertIs(span.status.status_code, StatusCode.OK)
+
+    def test_error_overrides_unset(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "boom"))
+        self.assertIs(span.status.status_code, StatusCode.ERROR)
+        self.assertEqual(span.status.description, "boom")
+
+    def test_ok_overrides_unset(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        self.assertIs(span.status.status_code, StatusCode.OK)
+
+    def test_ok_overrides_error(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "boom"))
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        self.assertIs(span.status.status_code, StatusCode.OK)
+        self.assertIsNone(span.status.description)
+
+    def test_error_does_not_override_ok(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "boom"))
+        self.assertIs(span.status.status_code, StatusCode.OK)
+        self.assertIsNone(span.status.description)
+
+    def test_ok_does_not_override_ok(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        span.set_status(trace_api.status.Status(StatusCode.OK))
+        self.assertIs(span.status.status_code, StatusCode.OK)
+
+    def test_bare_status_does_not_drop_existing_description(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "connection refused to db-1"))
+        span.set_status(trace_api.status.Status(StatusCode.ERROR))
+        self.assertIs(span.status.status_code, StatusCode.ERROR)
+        self.assertEqual(span.status.description, "connection refused to db-1")
+
+    def test_described_status_still_replaces_described_status(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "first"))
+        span.set_status(trace_api.status.Status(StatusCode.ERROR, "second"))
+        self.assertEqual(span.status.description, "second")
+
+    def test_bare_status_lands_when_there_is_no_description_to_keep(self):
+        span = self._span()
+        span.set_status(trace_api.status.Status(StatusCode.ERROR))
+        self.assertIs(span.status.status_code, StatusCode.ERROR)
+        self.assertIsNone(span.status.description)
+
+    def test_precedence_holds_for_the_statuscode_overload(self):
+        span = self._span()
+        span.set_status(StatusCode.ERROR, "boom")
+        span.set_status(StatusCode.UNSET)
+        self.assertIs(span.status.status_code, StatusCode.ERROR)
+        self.assertEqual(span.status.description, "boom")
+        span.set_status(StatusCode.OK)
+        self.assertIs(span.status.status_code, StatusCode.OK)
+        span.set_status(StatusCode.ERROR, "late")
+        self.assertIs(span.status.status_code, StatusCode.OK)
+
     def test_record_exception_fqn(self):
         span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
         exception = DummyError("error")