Skip to content

[SDK] Enforce Span Status transition rules - #4547

Merged
marcalff merged 5 commits into
open-telemetry:mainfrom
denizariyan:fix/status-transition
Sep 13, 2026
Merged

marcalff merged 5 commits into
open-telemetry:mainfrom
denizariyan:fix/status-transition

Conversation

@denizariyan

@denizariyan denizariyan commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #4546

Changes

  • Enforces the span status transition rules as per the spec.
  • I choose to enforce it on the span level as that is the common access path with the sdk and as far as my reading, what the spec actually wants us to do. The recordable level is mostly left as-is except where changes are required.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@denizariyan
denizariyan force-pushed the fix/status-transition branch from 5b480bb to 4c630e7 Compare September 9, 2026 16:01
Comment on lines +651 to +653
// The description is not null terminated at its end, so a copy through data() alone overruns it.
std::string description = "boomX";
span->SetStatus(opentelemetry::trace::StatusCode::kError,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added this because the prev impl actually had this issue with nullptr or non-null terminated strs, not exactly related to this PR but fixed along the way since it's a single line I am changing around anyways

@denizariyan
denizariyan marked this pull request as ready for review September 9, 2026 16:06
@denizariyan
denizariyan requested a review from a team as a code owner September 9, 2026 16:06
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.45%. Comparing base (e81d957) to head (c1e8793).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4547      +/-   ##
==========================================
+ Coverage   86.41%   86.45%   +0.05%     
==========================================
  Files         524      525       +1     
  Lines       20435    20450      +15     
==========================================
+ Hits        17657    17679      +22     
+ Misses       2778     2771       -7     
Files with missing lines Coverage Δ
exporters/otlp/src/otlp_recordable.cc 91.34% <100.00%> (+0.07%) ⬆️
exporters/zipkin/src/recordable.cc 88.29% <100.00%> (+0.10%) ⬆️
sdk/include/opentelemetry/sdk/trace/span_status.h 100.00% <100.00%> (ø)
sdk/src/trace/span.cc 94.92% <100.00%> (+1.06%) ⬆️
sdk/src/trace/span.h 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@denizariyan
denizariyan force-pushed the fix/status-transition branch from 4c630e7 to ae40757 Compare September 9, 2026 18:21
Comment thread sdk/include/opentelemetry/sdk/trace/span_status.h
Comment thread CHANGELOG.md

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. With nit comment for header reorg, and changelog. thanks.

@marcalff
marcalff merged commit e0e0458 into open-telemetry:main Sep 13, 2026
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Span::SetStatus does not adhere to the status transition rules per the specification

3 participants