-
Notifications
You must be signed in to change notification settings - Fork 452
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
[Metrics API] Mark NoopMeterProvider as pub(crate) #2191
[Metrics API] Mark NoopMeterProvider as pub(crate) #2191
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2191 +/- ##
=====================================
Coverage 79.2% 79.2%
=====================================
Files 121 121
Lines 20870 20870
=====================================
Hits 16534 16534
Misses 4336 4336 ☔ View full report in Codecov by Sentry. |
changelog as this is removing existing public api though this should affect noone? |
Just confirm if this is specs compliant - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/noop.md, as per the specs - "All language implementations of OpenTelemetry MUST provide a No-Op." |
This is spec compliant. We have no-op behavior, just not exposing them to public. |
What if the user wants to explicitly set the no-op provider, to (say) disable the telemetry? As per the specs, the noop is for end-user. |
If there is a need for that, we can offer mechanisms to achieve that. |
Can we make this public again? I was depending on this to disable telemetry selectively. cc @cijothomas |
@phillipleblanc Not directly answering comment, even though I voted above to keep the API. But just to say "Hi" - Not sure you remember, but we worked together in App Center (VSAC) in Microsoft :) |
Hey, yep I remember! Small world :) |
back to the query, if you would like to use NoopMeterProvider to disable telemetry, you can save the handle initially at startup as |
I ended up copying the implementation of |
Could you open a new issue for this, so we can track accordingly? |
Filed! #2444 |
Changes
NoopMeterProvider
is only used in the API projectPlease provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes