From 19c3aa0260fa575d7fd691468a682b71a5982ff3 Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:40:39 +0100 Subject: [PATCH 1/6] Keep an existing status description when a bare Status(ERROR) is set Fixes #5661. set_status returns early when the span is already OK and when the incoming status is UNSET, but nothing stops a second ERROR replacing a first one. A bare Status(StatusCode.ERROR) therefore overwrites Status(StatusCode.ERROR, "...") and the description is gone. The status code is unchanged, so the loss is silent. The assignment is now declined when the incoming status carries no description, the recorded one does, and the codes match. A described status still replaces a described status, a bare status still lands when there is nothing to keep, and a different status code still takes precedence as before. Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> --- .../src/opentelemetry/sdk/trace/__init__.py | 10 +++++++ opentelemetry-sdk/tests/trace/test_trace.py | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 14d6573d0cf..d2ff78962b0 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -993,6 +993,16 @@ def set_status( "Description %s ignored. Use either `Status` or `(StatusCode, Description)`", description, ) + # Keep the description already recorded for this status code. A bare + # Status(ERROR) carries no new information, so letting it through + # would drop a specific message in favour of nothing. + if ( + self._status is not None + and status.description is None + and self._status.description is not None + and status.status_code is self._status.status_code + ): + return self._status = status elif isinstance(status, StatusCode): if self._status and self._status.status_code is StatusCode.OK or status is StatusCode.UNSET: diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index b147979d932..d71ce9518e6 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -1350,6 +1350,34 @@ 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")) + def test_bare_status_does_not_drop_existing_description(self): + span = trace.TracerProvider().get_tracer(__name__).start_span("root") + 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 = trace.TracerProvider().get_tracer(__name__).start_span("root") + 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 = trace.TracerProvider().get_tracer(__name__).start_span("root") + span.set_status(trace_api.status.Status(StatusCode.ERROR)) + self.assertIs(span.status.status_code, StatusCode.ERROR) + self.assertIsNone(span.status.description) + + def test_bare_status_of_a_different_code_still_lands(self): + # The guard is per status code: only a repeat of the same code with no + # description is declined, so an ERROR still overwrites a described UNSET. + span = trace.TracerProvider().get_tracer(__name__).start_span("root") + span._status = trace_api.status.Status(StatusCode.UNSET, "pending") + span.set_status(trace_api.status.Status(StatusCode.ERROR)) + self.assertIs(span.status.status_code, StatusCode.ERROR) + self.assertIsNone(span.status.description) + def test_record_exception_fqn(self): span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext)) exception = DummyError("error") From 67ef8e0fb40e2f7bcdc5434f7c1a3152c37b2ad1 Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:42:54 +0100 Subject: [PATCH 2/6] changelog: add fragment for #5662 Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> --- .changelog/5662.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/5662.fixed diff --git a/.changelog/5662.fixed b/.changelog/5662.fixed new file mode 100644 index 00000000000..be2ee2657b9 --- /dev/null +++ b/.changelog/5662.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: keep an existing status description when a bare `Status(ERROR)` is set on a span that already has one From e328d94eb6682138a0e6aecf4c4311d5c8193f44 Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:14:39 +0100 Subject: [PATCH 3/6] Apply the specified status precedence in set_status 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. The SDK already behaved that way, but the rule was spread across two ad-hoc conditions and one of them relied on Python's and/or precedence to read correctly, so it was neither obvious nor covered by tests. State the order explicitly in _STATUS_PRECEDENCE and move the decision into _accepts_status, then pin every transition in the lattice with a test. The enum's own values are Unset=0, Ok=1, Error=2, which is not the specified order, so the map cannot be replaced by a comparison on the enum. Within a single status code the last call still wins, with one exception kept from the original fix: a bare status carries no new information, so letting it through would drop a description already recorded in favour of nothing. --- .changelog/5662.fixed | 2 +- .../src/opentelemetry/sdk/trace/__init__.py | 61 +++++++++----- opentelemetry-sdk/tests/trace/test_trace.py | 79 ++++++++++++++++--- 3 files changed, 113 insertions(+), 29 deletions(-) diff --git a/.changelog/5662.fixed b/.changelog/5662.fixed index be2ee2657b9..1ce4e93f026 100644 --- a/.changelog/5662.fixed +++ b/.changelog/5662.fixed @@ -1 +1 @@ -`opentelemetry-sdk`: keep an existing status description when a bare `Status(ERROR)` is set on a span that already has one +`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 diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index d2ff78962b0..627d7cea2b0 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/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,31 +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, ) - # Keep the description already recorded for this status code. A bare - # Status(ERROR) carries no new information, so letting it through - # would drop a specific message in favour of nothing. - if ( - self._status is not None - and status.description is None - and self._status.description is not None - and status.status_code is self._status.status_code - ): - return - 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, diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index d71ce9518e6..ff6bc6a8497 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -1350,33 +1350,92 @@ 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 = trace.TracerProvider().get_tracer(__name__).start_span("root") + 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 = trace.TracerProvider().get_tracer(__name__).start_span("root") + 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 = trace.TracerProvider().get_tracer(__name__).start_span("root") + 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_bare_status_of_a_different_code_still_lands(self): - # The guard is per status code: only a repeat of the same code with no - # description is declined, so an ERROR still overwrites a described UNSET. - span = trace.TracerProvider().get_tracer(__name__).start_span("root") - span._status = trace_api.status.Status(StatusCode.UNSET, "pending") - span.set_status(trace_api.status.Status(StatusCode.ERROR)) + 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.assertIsNone(span.status.description) + 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)) From 69b6ab66f429fc576c88d991c988161ac38ec430 Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Sat, 19 Sep 2026 02:03:13 +0100 Subject: [PATCH 4/6] Address review: description-absent check, match-based precedence, subTests Three points from the review. A same-code status now has to fill in a description that is missing rather than merely carry one. Previously any described status replaced a described status of the same code, on the reading that the last call wins. The review asked that an already populated description not be overwritten, and that also settles the Error to Error question raised earlier in the thread: a repeat of a code already recorded is ignored unless it adds a message where there was none. One assertion inverts with this, the case that used to expect the second description now expects the first. The precedence map becomes _status_precedence, a small function with a match expression and a default arm. A code the ordering has not been taught about ranks below Unset, so an unknown status cannot displace one already recorded, and it raises nothing at runtime. The twelve transition tests become a table of thirteen cases driven through self.subTest, and the table is now run twice, once through Status instances and once through the StatusCode overload, which covers the overload on every case rather than on one. Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> --- .changelog/5662.fixed | 2 +- .../src/opentelemetry/sdk/trace/__init__.py | 46 +++-- opentelemetry-sdk/tests/trace/test_trace.py | 190 ++++++++++-------- 3 files changed, 140 insertions(+), 98 deletions(-) diff --git a/.changelog/5662.fixed b/.changelog/5662.fixed index 1ce4e93f026..b0f75e9c761 100644 --- a/.changelog/5662.fixed +++ b/.changelog/5662.fixed @@ -1 +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`: 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 c3d01de383d..9dc3a96a346 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -76,13 +76,24 @@ 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, -} + +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 @@ -1010,10 +1021,11 @@ 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. + 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 @@ -1024,15 +1036,15 @@ def _accepts_status(self, new_status: Status) -> bool: if current.status_code is StatusCode.OK: return False - current_rank = _STATUS_PRECEDENCE[current.status_code] - new_rank = _STATUS_PRECEDENCE[new_status.status_code] + 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 + # 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 65a65cff440..b0f8b467f63 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -1352,91 +1352,121 @@ def error_status_test(context): 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. + # 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_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_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)) From dda586e84b4fd1b97ef7575a8498a840f03773db Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:03:08 +0100 Subject: [PATCH 5/6] Simplify _accepts_status and drop _status_precedence Once Unset is ignored and Ok is final, any code that differs from the one recorded is an upgrade, so the rank comparison is not needed. No change in behaviour. Also fixes the article before Unset in the changelog fragment. Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> --- .changelog/5662.fixed | 2 +- .../src/opentelemetry/sdk/trace/__init__.py | 40 ++++--------------- 2 files changed, 8 insertions(+), 34 deletions(-) diff --git a/.changelog/5662.fixed b/.changelog/5662.fixed index b0f75e9c761..e7dee26cb9d 100644 --- a/.changelog/5662.fixed +++ b/.changelog/5662.fixed @@ -1 +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`: apply the specified status precedence in `Span.set_status` so `Ok > Error > Unset` is enforced explicitly, an `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 9dc3a96a346..46f1bd932ee 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -77,24 +77,6 @@ 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 @@ -1020,12 +1002,10 @@ def set_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. + The codes are totally ordered, ``Ok > Error > Unset``, an attempt to set + ``Unset`` is ignored, and ``Ok`` is final. Once those two are handled, a + differing code always outranks the one recorded. A repeat of the same code + gets through only when it fills in a description that is still missing. """ if new_status.status_code is StatusCode.UNSET: return False @@ -1036,15 +1016,9 @@ def _accepts_status(self, new_status: Status) -> bool: 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 + return current.status_code is not new_status.status_code or ( + current.description is None and new_status.description is not None + ) def __exit__( self, From 14480f2000b478c2fa944f18ee169efeb65b1873 Mon Sep 17 00:00:00 2001 From: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> Date: Thu, 24 Sep 2026 23:07:56 +0100 Subject: [PATCH 6/6] Treat an empty status description as absent The spec says an empty description is equivalent to an absent one, so an Error recorded with an empty description can now take a later one, and an empty description never replaces one already recorded. Both transitions are added to the precedence cases. Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com> --- .../src/opentelemetry/sdk/trace/__init__.py | 6 ++++-- opentelemetry-sdk/tests/trace/test_trace.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 46f1bd932ee..e662e1856ce 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -1005,7 +1005,8 @@ def _accepts_status(self, new_status: Status) -> bool: The codes are totally ordered, ``Ok > Error > Unset``, an attempt to set ``Unset`` is ignored, and ``Ok`` is final. Once those two are handled, a differing code always outranks the one recorded. A repeat of the same code - gets through only when it fills in a description that is still missing. + gets through only when it fills in a description that is still missing, + and an empty description counts as missing. """ if new_status.status_code is StatusCode.UNSET: return False @@ -1016,8 +1017,9 @@ def _accepts_status(self, new_status: Status) -> bool: if current.status_code is StatusCode.OK: return False + # An empty description is equivalent to an absent one. return current.status_code is not new_status.status_code or ( - current.description is None and new_status.description is not None + not current.description and bool(new_status.description) ) def __exit__( diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index b0f8b467f63..f5eb7128909 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -1427,6 +1427,18 @@ def error_status_test(context): StatusCode.ERROR, "boom", ), + ( + "an empty description counts as missing", + [(StatusCode.ERROR, ""), (StatusCode.ERROR, "useful detail")], + StatusCode.ERROR, + "useful detail", + ), + ( + "an empty description does not replace one", + [(StatusCode.ERROR, "boom"), (StatusCode.ERROR, "")], + StatusCode.ERROR, + "boom", + ), ( "a bare error lands when there is nothing to keep", [(StatusCode.ERROR, None)],