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

Verify actual status instead of just errored #608

Closed
anuraaga opened this issue Jun 29, 2020 · 2 comments · Fixed by #2710
Closed

Verify actual status instead of just errored #608

anuraaga opened this issue Jun 29, 2020 · 2 comments · Fixed by #2710

Comments

@anuraaga
Copy link
Contributor

OTel spans have a status with different codes but many of our tests only test success vs error. We should migrate to asserting on the status instead.

@trask trask added contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome area:tests labels Jun 29, 2020
@trask
Copy link
Member

trask commented Jul 16, 2020

Before anyone picks up this issue, it sounds span status may be going away open-telemetry/oteps#123 (comment)

@trask trask removed the contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome label Jul 16, 2020
@breedx-splk
Copy link
Contributor

There was a LOT of discussion around the removal of Span.Status but according to this comment it looks like there was a decision to remove it, but then according to the actual span spec today (which is frozen), spans definitely still have a status.

Since this issue has been around for a while, maybe we can tweak it to help foster contribution. How about splitting this into smaller, more targeted issues? There are a LOT of tests, and a LOT of usages of SpanAssert.errored().

Suggest picking our top/favorite instrumentations that could benefit from status checking and put those in a new issue or calling them out by name here with more detail (eg. what status is expected and if semantic conventions are also to be verified).

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

Successfully merging a pull request may close this issue.

3 participants