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 status that repeats the code already recorded no longer replaces or drops the description recorded with it

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

@@ -76,6 +76,25 @@
 
 logger = logging.getLogger(__name__)
 
+
+def _status_precedence(status_code: StatusCode) -> int:
+    """Rank ``status_code`` by the order the specification gives, ``Ok > Error > Unset``.
+
+    The enum's own values do not carry that order, so it is stated here. A
+    code with no rank of its own sorts below ``Unset``, so a status the
+    ordering has not been taught about cannot displace one already recorded.
+    """
+    match status_code:
+        case StatusCode.UNSET:
+            return 0
+        case StatusCode.ERROR:
+            return 1
+        case StatusCode.OK:
+            return 2
+        case _:
+            return -1
+
+
 _DEFAULT_OTEL_ATTRIBUTE_COUNT_LIMIT = 128
 _DEFAULT_OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT = 128
 _DEFAULT_OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT = 128
@@ -983,21 +1002,49 @@ 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``, and says an attempt to set ``Unset`` should be ignored. So a
+        code that does not rank above the one already recorded is dropped and
+        ``Ok`` is final. A repeat of the same code is allowed through only
+        when it fills in a description that is still missing, which keeps a
+        message already recorded from being replaced or dropped.
+        """
+        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 ordering has nothing more to say. The only call
+        # left that carries new information is one that supplies a
+        # description where none was recorded.
+        return current.description is None and new_status.description is not None
 
     def __exit__(
         self,

opentelemetry-sdk/tests/trace/test_trace.py

@@ -1351,6 +1351,123 @@ 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 and says an attempt to set
+    # Unset should be ignored. A repeat of a code already recorded may only
+    # fill in a description that is missing, never replace or drop one.
+
+    # (name, calls as (code, description), expected code, expected description)
+    _PRECEDENCE_CASES = [
+        (
+            "unset on its own is ignored",
+            [(StatusCode.UNSET, None)],
+            StatusCode.UNSET,
+            None,
+        ),
+        (
+            "unset is ignored over error",
+            [(StatusCode.ERROR, "boom"), (StatusCode.UNSET, None)],
+            StatusCode.ERROR,
+            "boom",
+        ),
+        (
+            "unset is ignored over ok",
+            [(StatusCode.OK, None), (StatusCode.UNSET, None)],
+            StatusCode.OK,
+            None,
+        ),
+        (
+            "error overrides unset",
+            [(StatusCode.ERROR, "boom")],
+            StatusCode.ERROR,
+            "boom",
+        ),
+        (
+            "ok overrides unset",
+            [(StatusCode.OK, None)],
+            StatusCode.OK,
+            None,
+        ),
+        (
+            "ok overrides error",
+            [(StatusCode.ERROR, "boom"), (StatusCode.OK, None)],
+            StatusCode.OK,
+            None,
+        ),
+        (
+            "error does not override ok",
+            [(StatusCode.OK, None), (StatusCode.ERROR, "boom")],
+            StatusCode.OK,
+            None,
+        ),
+        (
+            "ok does not override ok",
+            [(StatusCode.OK, None), (StatusCode.OK, None)],
+            StatusCode.OK,
+            None,
+        ),
+        (
+            "a bare error does not drop a description",
+            [
+                (StatusCode.ERROR, "connection refused to db-1"),
+                (StatusCode.ERROR, None),
+            ],
+            StatusCode.ERROR,
+            "connection refused to db-1",
+        ),
+        (
+            "a described error does not replace a description",
+            [(StatusCode.ERROR, "first"), (StatusCode.ERROR, "second")],
+            StatusCode.ERROR,
+            "first",
+        ),
+        (
+            "a description fills in where none was recorded",
+            [(StatusCode.ERROR, None), (StatusCode.ERROR, "boom")],
+            StatusCode.ERROR,
+            "boom",
+        ),
+        (
+            "a bare error lands when there is nothing to keep",
+            [(StatusCode.ERROR, None)],
+            StatusCode.ERROR,
+            None,
+        ),
+        (
+            "ok stays final over a longer run",
+            [
+                (StatusCode.ERROR, "boom"),
+                (StatusCode.UNSET, None),
+                (StatusCode.OK, None),
+                (StatusCode.ERROR, "late"),
+            ],
+            StatusCode.OK,
+            None,
+        ),
+    ]
+
+    @staticmethod
+    def _span():
+        return trace.TracerProvider().get_tracer(__name__).start_span("root")
+
+    def test_status_precedence_with_a_status_instance(self):
+        for name, calls, code, description in self._PRECEDENCE_CASES:
+            with self.subTest(name):
+                span = self._span()
+                for call_code, call_description in calls:
+                    span.set_status(trace_api.status.Status(call_code, call_description))
+                self.assertIs(span.status.status_code, code)
+                self.assertEqual(span.status.description, description)
+
+    def test_status_precedence_with_the_statuscode_overload(self):
+        for name, calls, code, description in self._PRECEDENCE_CASES:
+            with self.subTest(name):
+                span = self._span()
+                for call_code, call_description in calls:
+                    span.set_status(call_code, call_description)
+                self.assertIs(span.status.status_code, code)
+                self.assertEqual(span.status.description, description)
+
     def test_record_exception_fqn(self):
         span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
         exception = DummyError("error")