-
Notifications
You must be signed in to change notification settings - Fork 753
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
feat(shim-opentracing): update setTag based on new spec #2194
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2194 +/- ##
==========================================
+ Coverage 92.28% 92.79% +0.51%
==========================================
Files 127 144 +17
Lines 4186 5178 +992
Branches 852 1062 +210
==========================================
+ Hits 3863 4805 +942
- Misses 323 373 +50
|
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.
Should addTags map error tags to status code as well? (Not defined in the OTEL spec, since it's not in the OpenTracing spec)
Not sure what you meant here ?
There is an addTags method in the opentracing-js API, which sets multiple tags at once. I was unsure if the shim should look at each tag in the args, and map any error tags to status, like |
As my other comment, i would suggest to do what you think is best given the time you have ! |
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.
lgtm, thanks for taking the time to fix this !
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.
lgtm
@vreynolds Could you rebase the PR ? There is one conflict :/ |
* map error true/false to status code error/ok
* map UNSET status code explicitly
399baba
to
1e64f3b
Compare
@vreynolds Sorry but it seems there is still conflict |
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.
Looks great to me but there is still a conflict
Thanks for merging upstream @dyladan ! Looks like conflicts have been resolved, but LMK if there's anything more I can do. |
Which problem is this PR solving?
error
tag to status code (true --> Error, false --> Ok)Short description of the changes
error
tag that's not true/false will default to Unset status codeQuestions
Should addTags map error tags to status code as well? (Not defined in the OTEL spec, since it's not in the OpenTracing spec)I updatedaddTags
as well, since that seems like the spirit of the spec.