Skip to content

Fix StatusCode in stdout exporter (#1453)#1454

Merged
jtescher merged 2 commits into
open-telemetry:mainfrom
j-white:jw/fix-status-code
Jan 4, 2024
Merged

Fix StatusCode in stdout exporter (#1453)#1454
jtescher merged 2 commits into
open-telemetry:mainfrom
j-white:jw/fix-status-code

Conversation

@j-white

@j-white j-white commented Dec 24, 2023

Copy link
Copy Markdown
Contributor

Fixes #1453

Changes

Swap the ordinals to match the spec.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@j-white
j-white requested a review from a team December 24, 2023 18:22
@linux-foundation-easycla

linux-foundation-easycla Bot commented Dec 24, 2023

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: j-white / name: Jesse White (2ee6b6a)
  • ✅ login: lalitb / name: Lalit Kumar Bhasin (4eb10cd)

@codecov

codecov Bot commented Dec 24, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5aa0311) 60.8% compared to head (4eb10cd) 60.8%.

Files Patch % Lines
opentelemetry-stdout/src/trace/transform.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1454   +/-   ##
=====================================
  Coverage   60.8%   60.8%           
=====================================
  Files        146     146           
  Lines      19230   19230           
=====================================
+ Hits       11702   11711    +9     
+ Misses      7528    7519    -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

opentelemetry::trace::Status::Error { description } => Status {
message: Some(description),
code: 1,
code: 2,

@lalitb lalitb Dec 29, 2023

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.

Nit- should we use the constants for status codes for better readability?

const STATUS_CODE_UNSET: u32 = 0;
const STATUS_CODE_OK: u32 = 1;
const STATUS_CODE_ERROR: u32 = 2;

These constants can be used both in is_zero and From.

@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 suggestion.

@jtescher jtescher 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.

Thanks @j-white, think it's ok to merge as is and improve in follow up as well

@jtescher
jtescher merged commit 4c5c101 into open-telemetry:main Jan 4, 2024
@j-white
j-white deleted the jw/fix-status-code branch January 4, 2024 21:05
takumi-earth pushed a commit to earthlings-dev/opentelemetry-rust that referenced this pull request Jan 27, 2026
…ry#1454)

Fixes open-telemetry#1453

## Changes

Swap the ordinals to match the spec.

Co-authored-by: Lalit Kumar Bhasin <[email protected]>
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]: Wrong StatusCode ordinal in stdout exporter

3 participants