Apply the specified status precedence in Span.set_status - #5662
WatchTree-19 wants to merge 7 commits into
Conversation
Fixes open-telemetry#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>
Assisted-by: Claude Opus 5 Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
|
We should probably update this to be more in line with what the spec expects: Namely that Span statuses form a total order |
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.
|
Thanks for laying the ordering out, I went through each point against the spec. Three of the four are in there: the total order I also wrote all twelve transitions up as tests and ran them against upstream What I've pushed now writes the order out explicitly and puts the decision in one method. It adds the twelve tests and keeps last call wins within the same rank, along with the exception for bare status. One thing worth mentioning: the enum is The main thing I wanted to check with you: should |
Shriprasad-P
left a comment
There was a problem hiding this comment.
Nice focused PR. Please make sure docs/changelog stay aligned if the repo requires it for this kind of change.
|
@herin049 any thoughts on the Error to Error question when you get a minute? Happy to switch it to ignoring a repeat of the same code if that is what you want, it is a small change. I only left it as last call wins because the spec says the value of the last call is the one recorded, and I did not want to diverge from that without you saying so. @Shriprasad-P thanks. The changelog fragment was rewritten alongside the code, .changelog/5662.fixed now describes the precedence change rather than the original description fix. |
The last sentence in the spec that states "Only the value of the last call will be recorded, and implementations are free to ignore previous calls." is a little strange and partially contradicts what is stated earlier in the paragraph. As a whole, I take this to mean that if a higher precedence status is set on a span, then previous calls can be ignored. I would expect that it is safe to ignore calls to set the status to |
Pull request dashboard statusWaiting on maintainers · refreshed 2026-09-21 17:31 UTC Merge when ready. Status above doesn't look right?
|
|
Thanks. The changelog fragment is already in the branch, .changelog/5662.fixed, which is the format this repo moved to instead of editing CHANGELOG.md directly. It covers all three behaviours the patch changes: the Ok > Error > Unset order being enforced explicitly, Unset always being ignored, and a bare Status(ERROR) no longer dropping a description that was already recorded. No user-facing docs reference the old behaviour, so nothing else needed there as far as I can see, but happy to add to it if you think somewhere else should call this out. |
…-keeps-description
…Tests 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>
|
Thanks, all three are in 69b6ab6 and I have replied on each thread. The one behaviour change is the first. A status that repeats the code already recorded is now ignored unless it supplies a description where none was recorded, so a message already on the span is never replaced and never dropped. That is your Error to Error point as well, and I am happy with it: the ordering is about codes, and once the code is unchanged the only call left carrying new information is one that adds a message. So the last call sentence still governs, it just governs the description rather than licensing a bare overwrite. Verification: 116 passed and 35 subtests passed in The changelog fragment now describes the precedence behaviour as it stands, and I have rewritten the PR description to match, since it still named the old tests. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Same-code handling incorrectly prevents replacing an existing error description with a newer one.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Updates SDK span status handling to enforce status-code precedence and preserve error descriptions.
Changes:
- Centralizes status precedence logic.
- Adds transition tests for both accepted input forms.
- Adds a changelog entry.
| File | Description |
|---|---|
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py |
Implements status precedence. |
opentelemetry-sdk/tests/trace/test_trace.py |
Tests status transitions. |
.changelog/5662.fixed |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>


Description
Fixes #5661.
Span.set_statusnow applies the precedence the specification gives,Ok > Error > Unset, in one place.Before this the rule was split across two ad-hoc conditions, one of which only reads correctly if you know Python's
and/orprecedence, and nothing tested it. The SDK was mostly doing the right thing already: of the twelve transitions in the lattice, eleven were correct on main. The one that was not is #5661, where a bareStatus(StatusCode.ERROR)replacedStatus(StatusCode.ERROR, "...")and the description was silently lost. The status code did not change, so nothing looked wrong; only the message that carried the diagnostic value disappeared.That is reachable without anyone doing anything odd. A library sets a specific error on a span, and later generic error handling, exception bookkeeping or an instrumentation helper sets a plain
ERRORon the same span.The decision now lives in
_accepts_status, and it is the whole rule:Unsetis never recorded. An attempt to set it is ignored, whatever is already there.Okis final, soErrornever displacesOk. OnceUnsetandOkare handled, any code that differs from the one recorded outranks it and is recorded.There is no separate ranking helper. Because
Unsetis ignored andOkis final, every remaining change of code is an upgrade, so_accepts_statusonly has to ask whether the code differs and, when it does not, whether a missing description is being filled in.Type of change
How Has This Been Tested?
opentelemetry-sdk/tests/trace/test_trace.pypasses in full: 116 passed, 35 subtests passed.The thirteen transitions are a
_PRECEDENCE_CASEStable inTestSpan, each entry being the calls to make and the status code and description expected. The table is driven throughself.subTestand run twice, once throughStatusinstances and once through theStatusCodeoverload, so the overload is exercised on every case rather than on one.The cases were checked to discriminate rather than merely pass:
new_status.description is not Noneturns two subtests red_accepts_statusaltogether turns twelve redruff checkandruff formatare clean and pylint scores 10.00/10 on both changed files.The behaviour was also reproduced against released
opentelemetry-sdk1.44.0 on CPython 3.11 before anything was changed, using the snippet in #5661.Does This PR Require a Contrib Repo Change?
Checklist:
On the changelog:
.changelog/names fragments after the PR number, which does not exist until this is opened, so<pr>.fixedfollows as a second commit straight after.AI assistance was used and is disclosed on the commits with an
Assisted-by:trailer, per AGENTS.md.