diff --git a/.changelog/5662.fixed b/.changelog/5662.fixed new file mode 100644 index 0000000000..b0f75e9c76 --- /dev/null +++ b/.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 diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 4555a2817f..9dc3a96a34 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/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, diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 574082917a..b0f8b467f6 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/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")