-
Notifications
You must be signed in to change notification settings - Fork 542
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
fix(example): fix koa example #1088
Conversation
|
Codecov Report
@@ Coverage Diff @@
## main #1088 +/- ##
=======================================
Coverage 95.91% 95.91%
=======================================
Files 13 13
Lines 856 856
Branches 178 178
=======================================
Hits 821 821
Misses 35 35 |
@@ -13,10 +13,10 @@ function makeRequest() { | |||
api.context.with(api.trace.setSpan(api.ROOT_CONTEXT, span), async () => { | |||
try { | |||
const res = await axios.get('http://localhost:8081/run_test'); | |||
span.setStatus({ code: api.StatusCode.OK }); | |||
span.setStatus({ code: api.SpanStatusCode.OK }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, I think that we should actually delete this line.
span status code should be UNSET
which is the default.
Generally, Instrumentation Libraries SHOULD NOT set the status code to Ok, unless explicitly configured to do so. Instrumentation Libraries SHOULD leave the status code as Unset unless there is an error, as described above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this example an end-user example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blumamir would you mind shedding your light on this again? I believe this is an end-user example so setting the status code here should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legendecas is correct the end user should set the span status and this example is fine IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! LGTM % the style nits mentioned by @blumamir.
Which problem is this PR solving?
Short description of the changes