Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#6207 Ensure Span Status Cannot Be Updated After StatusCode.OK Is Set #6209

Merged
merged 1 commit into from
Feb 8, 2024
Merged

#6207 Ensure Span Status Cannot Be Updated After StatusCode.OK Is Set #6209

merged 1 commit into from
Feb 8, 2024

Conversation

apederson94
Copy link
Contributor

@apederson94 apederson94 commented Feb 8, 2024

Add a check to ensure that calling Span.setStatus() will do nothing if StatusCode.OK has previously been set on the Span.
Add a unit test to ensure this functionality is/remains correct.

Should fix #6207

@apederson94 apederson94 requested a review from a team as a code owner February 8, 2024 01:06
Copy link

linux-foundation-easycla bot commented Feb 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: apederson94 / name: Austin Pederson (93a6577)

@apederson94
Copy link
Contributor Author

@jack-berg I have recreated the code and pull request on my personal machine and signed the CLA. Once this is merged, how long will it be until I can expect this code to be in production code?

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c12bb73) 91.03% compared to head (93a6577) 91.00%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6209      +/-   ##
============================================
- Coverage     91.03%   91.00%   -0.03%     
+ Complexity     5678     5677       -1     
============================================
  Files           621      621              
  Lines         16587    16590       +3     
  Branches       1694     1695       +1     
============================================
- Hits          15100    15098       -2     
- Misses          996     1003       +7     
+ Partials        491      489       -2     

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

@jack-berg
Copy link
Member

@jack-berg I have recreated the code and pull request on my personal machine and signed the CLA.

Thanks @apederson94! Something still isn't quite right with the CLA. Can you check out this link and try to resolve?
image

As for the timeline, we release the firday after the first monday of each month. Our next release is tomorrow, 2/9/24, and the following would be 3/9/24.

@apederson94
Copy link
Contributor Author

apederson94 commented Feb 8, 2024

@jack-berg I'm not sure why, but it is still pointing the CLA to my work email when I go to sign it. I checked the email that was attached to the commit in my IntelliJ IDE and it says one of the emails I signed the CLA with. (I signed it twice with two different emails to be sure it would work). I just reviewed the git log and this is what I see:

Author: Austin Pederson <austinpederson@pm.me>
Date:   Wed Feb 7 17:04:20 2024 -0800

    add check against Span current status before allowing update and add unit test

@jack-berg
Copy link
Member

/easycla

@jack-berg
Copy link
Member

Is the austinpederson@pm.me email address associated with your github account?

@apederson94
Copy link
Contributor Author

That was the issue. I just added that email to my account.

@trask
Copy link
Member

trask commented Feb 8, 2024

/easycla

@jack-berg jack-berg merged commit c0b73f5 into open-telemetry:main Feb 8, 2024
18 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.

StatusCode.OK is not respected as per the OpenTelemetry spec
3 participants