-
Notifications
You must be signed in to change notification settings - Fork 422
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
sdk: expose hidden Tracer::should_sample() method #1937
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1937 +/- ##
=======================================
- Coverage 74.9% 74.9% -0.1%
=======================================
Files 122 122
Lines 20311 20314 +3
=======================================
Hits 15217 15217
- Misses 5094 5097 +3 ☔ View full report in Codecov by Sentry. |
Downstream PR in tokio-rs/tracing-opentelemetry#155. It doesn't compile yet, but it's late here. If anyone wants to help figure this out, would appreciate it. (At this time, not sure if this PR is necessary or sufficient, probably better not to merge it yet.) |
Will hold off until we get confirmation that this will unblock tracing-opentelemetry. |
I've spent some time staring at the downstream conversion and have been having a hard time making sense of how all the bits are supposed to fit together. Apparently stuff like I'm honestly wondering whether it wouldn't make more sense to have a separate |
@djc We are constantly seeing issues with For the specific changes with OTLPExporter returning
Looks like you are leaning towards Option 4 from #1571, where otel and tracing just operates as 2 independent libraries, no special casing to support each other. (TBH, I haven't seen anyone supporting that approach so far. Apologies if I misinterpreted your statement.) If #1689 is adopted, then there won't be need for a separate bridge, as OTel will natively understand Spans from |
@djc - I updated the example code in tracing-opentelemetry fork to demonstrate the fix to be done refer to the changes in the Also +1 to have someone from |
Honestly, I'm a "reluctant" maintainer of tracing-opentelemetry -- I'm mainly helping out because I have a use case at work for funnelling tracing-rs spans into (a) Datadog (via OTLP), (b) Google Cloud Trace (née Stackdriver) and (c) Slack (via a custom I previously found being involved in the opentelemetry-rust project pretty frustrating, so I don't think I want to ramp that back up. I do think the opentelemetry-rust maintainers should consider tracing-opentelemetry an integral part of the project since, as far as I can tell, a majority of the opentelemetry-rust tracing consumers probably use it. So I would suggest we invert the burden of maintenance and say that openteleemtry-rust maintenance responsibility should extend out towards tracing-opentelemetry at least for trace-related public API changes.
#1689 seems like the right direction to me -- I think that a way to provide |
@lalitb thanks for the help. Based on your changes, I was able to update tokio-rs/tracing-opentelemetry#155 so that it passes CI. I think it would be great if this can be merged and published. |
Thanks for the PR @djc. Patch release is done for opentelemetry_sdk. |
Thanks! |
I'd be really interested to know more on this. The current maintainers (myself included!) would definitely benefit learning about this from folks who previously maintained the library. |
|
Changes
Unfortunately while working on #1934 I missed that tracing-opentelemetry needs one additional API method. Add that here. I will put up a draft tracing-opentelemetry PR after this to make sure this is complete.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes