Skip to content

fix: _set_status should not override existing span status (#3713)#4410

Open
RiyaChaturvedi37 wants to merge 15 commits intoopen-telemetry:mainfrom
RiyaChaturvedi37:fix/set-status-override
Open

fix: _set_status should not override existing span status (#3713)#4410
RiyaChaturvedi37 wants to merge 15 commits intoopen-telemetry:mainfrom
RiyaChaturvedi37:fix/set-status-override

Conversation

@RiyaChaturvedi37
Copy link
Copy Markdown
Contributor

Description

Fixes #3713

Problem

_set_status was unconditionally calling span.set_status(Status(status))
without checking if a status was already set. This caused two bugs:

  • A previously set ERROR status could be downgraded to OK
  • A previously set status description was wiped out (overridden with None)

Fix

Added a priority check before calling span.set_status:

  • Never downgrade status (ERROR → OK or ERROR → UNSET are blocked)
  • Preserve existing description if same status code and no new description provided

Tests

Added 4 new tests in test_semconv_status.py:

  • test_does_not_downgrade_error_to_ok
  • test_does_not_wipe_description_with_none
  • test_upgrades_unset_to_error
  • test_unset_to_ok

@RiyaChaturvedi37 RiyaChaturvedi37 force-pushed the fix/set-status-override branch from 3ae80cc to d712f06 Compare April 9, 2026 14:55
Comment thread CHANGELOG.md Outdated
@RiyaChaturvedi37 RiyaChaturvedi37 force-pushed the fix/set-status-override branch from 239f47f to 7214d29 Compare April 10, 2026 09:07
Copy link
Copy Markdown
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks @RiyaChaturvedi37. I've left a suggestion for moving the dict out of the func and updating a test decription.

Comment thread opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py Outdated
Comment thread opentelemetry-instrumentation/tests/test_semconv_status.py
@github-project-automation github-project-automation Bot moved this to Reviewed PRs that need fixes in Python PR digest Apr 13, 2026
@RiyaChaturvedi37 RiyaChaturvedi37 force-pushed the fix/set-status-override branch from 76ae517 to c07192b Compare April 14, 2026 02:27
Copy link
Copy Markdown
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. There's still some more things to fix up before we can accept.

Comment thread opentelemetry-instrumentation/tests/test_semconv_status.py
Comment thread CHANGELOG.md
@RiyaChaturvedi37
Copy link
Copy Markdown
Contributor Author

Hi @MikeGoldsmith, I've addressed all the feedback — moved the dict to module level, simplified the _set_span_status function, fixed the test description, and resolved the CHANGELOG conflict. Please let me know if there's anything else needed. Thank you!

@RiyaChaturvedi37
Copy link
Copy Markdown
Contributor Author

Hi @MikeGoldsmith,
I've addressed all the feedback and wanted to give an update on the current state of the PR:
Regarding span.status suggestion:
I tried committing your suggestion to use span.status directly, but it caused 14 test failures in instrumentation-wsgi and instrumentation-asgi. The error was:
AttributeError: Mock object has no attribute 'status'
This happens because those tests use MagicMock(spec_set='Span'), which doesn't include status in its spec. The getattr(span, "status", None) form handles this safely by returning None instead of raising an AttributeError. I've reverted to getattr for this reason.
Regarding the check-links failure:
The only remaining CI failure is check-links, which contains pre-existing broken links not introduced by this PR. These include:
Malformed URLs with %5D encoding
Missing / in domain names
Deleted upstream specification files
This PR only modifies _semconv.py, test_semconv_status.py, and CHANGELOG.md — none of which contain these broken links.
All 750 other checks are now passing. Please let me know if any further changes are needed. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

_set_status semconv helper can override span status if already defined

3 participants