-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added methods for SpanID and TraceID on bridgeSpanContext #3966
Conversation
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.
- It is not documented how to use it. See
bridge/opentracing/README.md
and the "extending" section. - It would be good to add some tests or at least comments in code that would make sure that someone would not remove these methods in future.
- Missing changelog entry.
EDIT: See #3570 for reference.
Updated the PR based on your suggestions. However, I am not exactly sure about the usage of the methods. So I have added a similar description as the reference you provided. So suggestions/feedback would be really helpful there. Also, updated the test case based on that, added comments on the methods and added entry in CHANGELOG.md. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3966 +/- ##
=======================================
- Coverage 82.2% 82.2% -0.1%
=======================================
Files 175 175
Lines 13067 13065 -2
=======================================
- Hits 10744 10742 -2
Misses 2102 2102
Partials 221 221
|
I'm wondering, instead of chasing down all the methods that SpanContext exposes, should we just have the bridgeSpanContext be able to produce the otel SpanContext? We can't drop IsSampled, but this might head off the need for IsValid, IsRemote, TraceFlags, TraceState, and anything else that might be added in the future. |
@MadVikingGod, @pellared, I made changes as you suggested. I added |
|
Co-authored-by: Robert Pająk <[email protected]>
Hi @MrAlias , I have made the required changes, could you please take a look and provide any feedback? |
@Kaushal28 please update this branch with |
@MrAlias I updated this branch but a CI step failed while uploading codecov report with error |
Resolves: #3954
Added methods for SpanID and TraceID on bridgeSpanContext and corresponding unit test case.