-
Notifications
You must be signed in to change notification settings - Fork 508
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: Leverage Suppression Context in Sdk #2868
feat: Leverage Suppression Context in Sdk #2868
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2868 +/- ##
=======================================
+ Coverage 80.8% 81.1% +0.2%
=======================================
Files 124 124
Lines 23836 23927 +91
=======================================
+ Hits 19283 19410 +127
+ Misses 4553 4517 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The regular instruments don't have to follow what bounded instrument does. They can easily accommodate the cost of doing the check, but we probably should have a consistent story for all metric instruments. |
Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
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.
Thanks for working on this., LGTM, with nit comments.
Fixes: #761
Continuing on top of #2821 , this PR modifies SDK's own components to suppress telemetry from its own operation using
Context::enter_telemetry_suppressed_scope
. This is done in the dedicated thread created by BatchProcessors, PeriodicReader.Also modified Log SDK to respect this and ignore logs if under suppression.
I did not modify Trace SDK and Metrics SDK to do the same because:
Trace SDK (and API) need some refactoring to find the right spot to do this. This is better handled in its own PR.Edited: This is done in Sampler now. This could be moved even earlier after some refactoring outside of this PR.Note: This does not prevent logs from external crates like tonic, hyper being fed back to Otel, as they don't propagate Otel context. That also can be solved, but this PR is not attempting to solve that.
Basic tests added, design doc updated.