-
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
Implement IsSampled for OpenTelemetry bridgeSpanContext #3570
Conversation
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3570 +/- ##
=====================================
Coverage 81.6% 81.7%
=====================================
Files 166 166
Lines 12442 12444 +2
=====================================
+ Hits 10162 10168 +6
+ Misses 2065 2061 -4
Partials 215 215
|
ed76677
to
254f7f4
Compare
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.
Please update the README with a section on how the sampled state can be accessed. Additionally, it might be worth adding an example test that accesses this method.
c19e2f7
to
1c42732
Compare
I'm not sure example test is worthwhile here. It would contain a lot of boilerplate just to set it up and then very trivial example. Also, there is no documentation for it to be attached to. I would skip it for this component. I think example in the README should be enough (given that this is the documentation for this package). If you do feel strongly about it, I can add it still. Let me know. |
1c42732
to
9f04f21
Compare
@MrAlias could you see the latest changes? Would be much appreciated. |
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
68efca3
to
1455d05
Compare
Merged your suggestions, moved note to the unreleased, added PR ID to the note and merged the |
I merged |
This still needs another review from someone else before it is ready to merge. |
Thank you for reviewing the PR. I updated the branch to the latest |
This is a PR in response to issue #3532 .
IsSampled
is not exposed by OpenTracing but its implementations did (at least some; sometimes called differently; Jaeger does). Cosidering that in OpenTracing this information was accessed by casting types, this implementation is no different - instances can be casted to an interface with expected methods in order to access it. Once moved to the bridge, eventually bridge can be dropped and this information would continue being exposed through the OpenTelemetry's span context.Resolves #3532