Tracing: Add code and message to Span -> Status #328
Conversation
mayurkale22
changed the title
mayurkale22
changed the title
mayurkale22
force-pushed
the
set_status
branch
from
3bc29f9 to
abd29c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #328 +/- ##
========================================
+ Coverage 94.95% 95% +0.04%
========================================
Files 119 120 +1
Lines 8172 8202 +30
Branches 732 730 -2
========================================
+ Hits 7760 7792 +32
+ Misses 412 410 -2
Continue to review full report at Codecov.
|
| */ | ||
| setStatus(code: number, message?: string) { | ||
| this.status.code = code; | ||
| if (message) { |
There was a problem hiding this comment.
If someone sets a new code but no message, would this check leave the old message assigned to status?
Could this function just be this.status = {code, message}, since we the message field is optional?
| * with an optional descriptive message. | ||
| */ | ||
| export interface Status { | ||
| /** The status code. */ |
There was a problem hiding this comment.
Could you add in the comment that this status code should correspond with a specific set of OpenCensus trace status codes and that HTTP status codes should be appropriately converted per the specs at https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#mapping-from-http-status-codes-to-trace-status-codes?
There was a problem hiding this comment.
Done as per #328 (comment).
| /** | ||
| * Should set a status | ||
| */ | ||
| describe('addStatus()', () => { |
| rootSpan.start(); | ||
| const span = new Span(rootSpan); | ||
| span.start(); | ||
| span.setStatus(400, 'This is an error'); |
There was a problem hiding this comment.
Per comment about about the canonical values, could this be a number like span.SetSTatus('3', 'Invalid argument') ?
| /** A developer-facing error message. */ | ||
| message?: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
What would you think about adding an enum with the list of OpenCensus status codes from the table referenced above?
There was a problem hiding this comment.
To be clear I'm not suggesting the type of the code field be the enum (that might limit clients of the library if a new status is added but they can't upgrade yet, and for opencensus-web I want to keep @opencensus/core just as a dev dependency). But just having the enum provides documentation for what the possible canonical status codes are.
There was a problem hiding this comment.
I checked in Java implementation, we are using enum for code -> https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/trace/Status.java#L384. This is also matching with status spec.
There was a problem hiding this comment.
OK, I'd be fine with using an enum for it.
mayurkale22
force-pushed
the
set_status
branch
from
1123b8f to
4e11ae6
Compare
| @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. | |||
| - Add ignoreIncomingPaths and ignoreOutgoingUrls support to the http and https tracing instrumentations. | |||
| - Add ```opencensus-resource-util``` to auto detect AWS, GCE and Kubernetes(K8S) monitored resource, based on the environment where the application is running. | |||
| - Add optional `uncompressedSize` and `compressedSize` fields to `MessageEvent` interface. | |||
| - Add a ```setStatus``` method in the Span. | |||
|
|
|||
| **This release has multiple breaking changes. Please test your code accordingly after upgrading.** | |||
There was a problem hiding this comment.
Should you mention that the status field on Span is no longer a number, since that's a breaking change?
| @@ -258,4 +258,35 @@ describe('Span', () => { | |||
| assert.ok(instanceOfLink(span.messageEvents[0])); | |||
| }); | |||
| }); | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Optional: what would you think about removing this comment? I personally find it a bit redundant with the describe('setStatus
| @@ -219,15 +219,15 @@ export class GrpcPlugin extends BasePlugin { | |||
| // tslint:disable-next-line:no-any | |||
| value: any, trailer: grpcTypes.Metadata, flags: grpcTypes.writeFlags) { | |||
| if (err) { | |||
| rootSpan.status = GrpcPlugin.convertGrpcStatusToSpanStatus(err.code); | |||
There was a problem hiding this comment.
Do you need to make similar changes to HttpPlugin.convertTraceStatus to make use of this new enum?
See:
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks! I think I got lost in the changes there and did not look closely enough.
|
Thanks @draffensperger for the reviews. |
Fixes #327