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

Improve trace status handling #3214

Merged
merged 5 commits into from Sep 23, 2022
Merged

Conversation

wdullaer
Copy link
Contributor

According to the specification SetStatus should not allow overwriting the status if it has explicitly been set to Ok.

From https://opentelemetry.io/docs/reference/specification/trace/api/#set-status

These values form a total order: Ok > Error > Unset. This means that setting Status with StatusCode=Ok will override any prior or future attempts to set span Status with StatusCode=Error or StatusCode=Unset. See below for more specific rules.

This is important for us, because the open telemetry middleware will explicit set an Error status for each non Ok grpc status code, while not all of these status codes indicate an error in our application semantics. Because the middleware runs after our user code in the request flow, we have no way of explicitly marking such requests as Ok, even though the tracing specification clearly foresees it.

sdk/trace/span.go Outdated Show resolved Hide resolved
@dmathieu
Copy link
Member

This will require a changelog entry.

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #3214 (50a1df9) into main (d7bfe66) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3214   +/-   ##
=====================================
  Coverage   77.3%   77.3%           
=====================================
  Files        159     159           
  Lines      11139   11142    +3     
=====================================
+ Hits        8613    8620    +7     
+ Misses      2329    2325    -4     
  Partials     197     197           
Impacted Files Coverage Δ
trace/trace.go 98.2% <ø> (ø)
sdk/trace/span.go 88.3% <100.0%> (+<0.1%) ⬆️
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+0.8%) ⬆️

@wdullaer wdullaer force-pushed the fix-set-status branch 3 times, most recently from 38abca3 to 9d88b74 Compare September 21, 2022 16:53
trace/trace.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Aneurysm9 Aneurysm9 changed the title Implement specification compliant trace status handling Improve trace status handling Sep 22, 2022
trace/trace.go Outdated Show resolved Hide resolved
wdullaer and others added 2 commits September 23, 2022 10:50
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@MrAlias
Copy link
Contributor

MrAlias commented Sep 23, 2022

This looks ready to merge once the lint issue is addressed.

@MrAlias MrAlias merged commit b369e59 into open-telemetry:main Sep 23, 2022
@MrAlias MrAlias mentioned this pull request Oct 12, 2022
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.

None yet

4 participants