Fixing Span status when exporting span #1751
Conversation
obecny
requested review from
dyladan,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1751 +/- ##
==========================================
- Coverage 92.12% 92.09% -0.03%
==========================================
Files 166 166
Lines 5573 5593 +20
Branches 1194 1199 +5
==========================================
+ Hits 5134 5151 +17
- Misses 439 442 +3
|
| const spanStatus: opentelemetryProto.trace.v1.Status = { | ||
| code: toCollectorCode(status.code), | ||
| }; | ||
| if (typeof status.message !== 'undefined') { |
There was a problem hiding this comment.
I think it would be better to directly test for undefined value instead of using typeof
if(status.message !== undefined)There was a problem hiding this comment.
not really, if message is set to anything I just pass it through, this is not validation but just checking if the message was set or not and then pass it through exactly as it was
There was a problem hiding this comment.
Yes but to check it you are using typeof and then compare the result to a string 'undefined'.
What is the benefit of it over testing directly for equality to undefined?
|
It's also worth noting that plugins dependent on @opentelemetry/api version <0.13.0 will still use the old CanonicalCode. If exported via the new exported, the wrong enum will be populated for those spans into the status code field which will be misunderstood by new collectors. Also, using the new exporter with an old collector in grpc will cause collector to not parse the status code from the new field, thus marking all spans as OK. |
| * An enumeration of status codes. | ||
| * https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/trace/v1/trace.proto#L304 | ||
| */ | ||
| export enum StatusCode { |
There was a problem hiding this comment.
Can we make this (and all enums) const?
Again for better minification as during "compiliation" to JS, typescript converts all usages to the numeric value only.
The downside is that if you have code to convert "string" names back to an the enum value its not possible without adding your own extra "lookups"
obecny
added
bug
Recently statuscode of span has been updated but not everything has been fixed. It was missing conversion from api status to collector status and proto needs to be updated to ver. 0.6
Which problem is this PR solving?
Short description of the changes