-
Notifications
You must be signed in to change notification settings - Fork 513
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: Hide MetricReader and friends #2928
feat: Hide MetricReader and friends #2928
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2928 +/- ##
=====================================
Coverage 81.3% 81.3%
=====================================
Files 126 126
Lines 24254 24254
=====================================
Hits 19736 19736
Misses 4518 4518 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I have looked a bit deeper into this and here's my understanding (maybe not correct :)) about the situation. So if we could achieve something similar in #2917 , then probably we could stabilize this in Metrics SDK :) P.S. maybe we should think about naming for |
Yes, Users should be in control of export interval - #2928 (comment) |
Agree with removing "Push" from MetricExporter. |
- *Breaking* `MetricError`, `MetricResult` no longer public (except when | ||
`spec_unstable_metrics_views` feature flag is enabled). `OTelSdkResult` should | ||
be used instead, wherever applicable. | ||
- *Breaking* change, affecting custom MetricReader authors: The |
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.
- *Breaking* change, affecting custom MetricReader authors: The | |
- *Breaking* change, affecting custom `MetricReader` authors: The |
- *Breaking* change, affecting custom MetricReader authors: The | ||
`shutdown_with_timeout` method is added to `MetricReader` trait. `collect` | ||
method on `MetricReader` modified to return `OTelSdkResult`. | ||
[#2905](https://github.com/open-telemetry/opentelemetry-rust/pull/2905) |
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.
This PR link is added twice.
pub(crate) mod aggregation; | ||
pub mod data; | ||
mod error; | ||
pub mod exporter; | ||
pub(crate) mod instrument; | ||
pub(crate) mod internal; | ||
#[cfg(feature = "experimental_metrics_custom_reader")] | ||
pub(crate) mod manual_reader; |
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.
We should probably just delete manual_reader.rs
file instead of providing it under a feature. There doesn't seem to be any use case for it.
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.
Left some suggestions.
Option2 from #2917
Trimming public API, by hiding MetricReader+friends under feature flag. This won't affect normal usage of Metrics, but only custom MetricReader authors. It is unclear what scenario requires custom MetricReaders.....
Ability to write custom Exporter is still maintained.
Also removed
Aggregation
enum to existing View feature, as that was the only use for Aggregation.Hoping to trim the questionable public API before declaring Metrics SDK as stable.