Update for the more restricted span status codes. #1701
Conversation
jkwatson
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1701 +/- ##
============================================
+ Coverage 85.36% 85.44% +0.08%
- Complexity 1382 1386 +4
============================================
Files 164 164
Lines 5418 5395 -23
Branches 556 556
============================================
- Hits 4625 4610 -15
+ Misses 594 587 -7
+ Partials 199 198 -1
Continue to review full report at Codecov.
|
| * @since 0.1.0 | ||
| */ | ||
| UNAVAILABLE(14), | ||
| OK(0), |
There was a problem hiding this comment.
Do we need the numbers anymore?
There was a problem hiding this comment.
I'm honestly not sure. I noticed that in the Jaeger mapping, we directly just copy that code over as a status code. Is that correct? No idea, either. I'd prefer to get rid of the numeric codes, and the accessor for them, but we need to decide what to do with the existing mappings.
There was a problem hiding this comment.
Got it - we can change this latter anyways.
| @@ -121,7 +121,7 @@ public V call() throws Exception { | |||
|
|
|||
| private static void setErrorStatus(Span span, Throwable t) { | |||
| span.setStatus( | |||
| Status.UNKNOWN.withDescription( | |||
| Status.ERROR.withDescription( | |||
There was a problem hiding this comment.
I realised that I still an open question about this use case: open-telemetry/opentelemetry-specification#1008
There was a problem hiding this comment.
I'd be happy to revisit this when the issue is clarified. Meanwhile, it does seem like approximately preserving the existing behavior is a good thing for now. And, as I commented on that issue, I think the intent with recordException is that it's an error, most commonly.
There was a problem hiding this comment.
When that gets merged, I'll create an issue to address it here. Meanwhile, keeping the existing behavior seems like the right decision for this PR.
resolves #1700
Note: I made the numeric codes match the proposed protobuf status codes.