You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "koushiro (via GitHub)" <gi...@apache.org> on 2024/08/30 05:13:20 UTC
[PR] refactor(layers/prometheus): provide consistent APIs [opendal]
koushiro opened a new pull request, #5072:
URL: https://github.com/apache/opendal/pull/5072
# Which issue does this PR close?
Related issue: https://github.com/apache/opendal/pull/5069#discussion_r1736066873
# Rationale for this change
Provide similar APIs for `PrometheusLayer` and `PrometheusClientLayer`
# What changes are included in this PR?
- add `PrometheusLayer::register`
- impl `Default` for `PrometheusInterceptor`
- move `path_label_value` into `observe` mod
- add more examples in doc
# Are there any user-facing changes?
API breaking change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742065465
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
+ PrometheusLayerBuilder::default().register(registry)
+ }
+
+ /// Create a [`PrometheusLayerBuilder`] to modify the default metric configuration.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let duration_seconds_buckets = prometheus::exponential_buckets(0.01, 2.0, 16).unwrap();
+ /// let bytes_buckets = prometheus::exponential_buckets(1.0, 2.0, 16).unwrap();
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(
+ /// PrometheusLayer::builder()
+ /// .operation_duration_seconds_buckets(duration_seconds_buckets)
+ /// .operation_bytes_buckets(bytes_buckets)
+ /// .enable_path_label(1)
+ /// .register(registry)
+ /// )
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn builder() -> PrometheusLayerBuilder {
+ PrometheusLayerBuilder::default()
+ }
+}
+
+impl<A: Access> Layer<A> for PrometheusLayer {
+ type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
+
+ fn layer(&self, inner: A) -> Self::LayeredAccess {
+ observe::MetricsLayer::new(self.interceptor.clone()).layer(inner)
+ }
+}
+
+/// [`PrometheusLayerBuilder`] is a config builder to build a [`PrometheusLayer`].
+pub struct PrometheusLayerBuilder {
operation_duration_seconds_buckets: Vec<f64>,
operation_bytes_buckets: Vec<f64>,
path_label_level: usize,
}
-impl PrometheusLayer {
- /// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
+impl Default for PrometheusLayerBuilder {
Review Comment:
`PrometheusLayerBuilder` should be exposed; otherwise, users can't access its API documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738095912
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
Okay, how about this: Let's use your previous proposed builder's API.
```rust
let layer: impl Layer = PrometheusClientLayer::builder()
.operation_bytes_buckets(vec![1.0, 2.0])
.register(registry);
```
`PrometheusClientLayer::builder()` returns a `PrometheusClientLayerBuilder`. Users can only use this layer after calling `register`. We can provide `default()` if the upstream lib has `default` registry concept.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5072:
URL: https://github.com/apache/opendal/pull/5072
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738062106
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
Review Comment:
Actually, I don't think we should hold the registry in `PrometheusLayer` struct (although the `Registry` type uses Arc internally)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1741936138
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
Review Comment:
How about removing the `new` API? Offering a single method to build layers simplifies usage for users.
We can offer an API `register_default()` instead, which is better composed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742249533
##########
core/src/types/error.rs:
##########
@@ -414,6 +414,13 @@ impl From<Error> for io::Error {
}
}
+#[cfg(feature = "layers-prometheus")]
Review Comment:
Please don't add `From` for `Error`. Implement a `parse_prometheus_error` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742025742
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
Review Comment:
You mean like this?
```rust
pub fn register_default() -> Self {
let registry = prometheus::default_registry();
PrometheusLayerBuilder::default().register(registry)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742363233
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,148 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the default registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ ///
+ /// let op = Operator::new(builder)?
+ /// .layer(PrometheusLayer::register_default().expect("register metrics successfully"))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register_default() -> Result<Self> {
Review Comment:
Sorry, I missed this API. I believe it belongs to `PrometheusLayerBuilder`.
---
So, why are we moving all APIs out of `PrometheusLayer`?
For composition. Providing any API at `PrometheusLayer` makes it difficult to integrate with other public APIs that `PrometheusLayerBuilder` offers. For example, if users want to use `register_default` and custom-defined buckets, they will need to repeat the same logic here as well.
##########
core/src/layers/prometheus.rs:
##########
Review Comment:
The comments on `Prometheus Metrics` are no longer accurate. How about adding those metrics comments in the `observe` module and just linking to it?
##########
core/src/layers/prometheus.rs:
##########
@@ -143,81 +281,106 @@ impl PrometheusLayer {
/// - level = 0: we will ignore the path label.
/// - level > 0: the path label will be the path split by "/" and get the last n level,
/// if n=1 and input path is "abc/def/ghi", and then we will get "abc/" as the path label.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)?
+ /// .layer(
+ /// PrometheusLayer::builder()
+ /// .enable_path_label(1)
+ /// .register(registry)
+ /// .expect("register metrics successfully")
+ /// )
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
Review Comment:
Let's call this API `path_label` for alignment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738105478
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
So do we need to use similar APIs in `PrometheusLayer`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738061866
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
Personally, I don't believe we should perform `register` during `new` unless the upstream API forbid it. Creating the layer itself should have no side effects. Everything should occur during the layer.
So `PrometheusLayer::new()` should just take a `registry` and return `Self`. We will perform the registry during `Layer::layer`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738119906
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
Yes, if you agree with the design, I'll apply it to `PrometheusLayer` and `PrometheusClientLayer`.
But I'm not sure if this design will fit well with other metric layers, I'm not very optimistic after using the `metrics` library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738110212
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
> So do we need to use similar APIs in `PrometheusLayer`?
I believe this design can adapt to all metrics libraries. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738125698
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
I feel like we can call `with_recorder` direcly: https://docs.rs/metrics/latest/src/metrics/recorder/mod.rs.html#126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1737997405
##########
core/src/layers/mod.rs:
##########
@@ -108,4 +108,5 @@ mod dtrace;
#[cfg(all(target_os = "linux", feature = "layers-dtrace"))]
pub use self::dtrace::DtraceLayer;
+#[cfg(any(feature = "layers-prometheus", feature = "layers-prometheus-client"))]
Review Comment:
`observe` doesn't introduce any additional dependencies. It's fine to leave them without feature flags.
##########
core/src/layers/observe/mod.rs:
##########
@@ -33,3 +34,37 @@ pub use metrics::LABEL_SCHEME;
pub use metrics::METRIC_OPERATION_BYTES;
pub use metrics::METRIC_OPERATION_DURATION_SECONDS;
pub use metrics::METRIC_OPERATION_ERRORS_TOTAL;
+
+pub(crate) fn path_label_value(path: &str, path_level: usize) -> Option<&str> {
+ if path.is_empty() {
Review Comment:
We can check `path_level` here directly to make code easier to read.
##########
core/src/layers/prometheus.rs:
##########
@@ -218,6 +311,53 @@ impl PrometheusInterceptor {
}
}
+impl PrometheusInterceptor {
+ fn with_operation_duration_seconds(mut self, buckets: Vec<f64>) -> Self {
+ let labels = OperationLabels::names(false, self.path_label_level);
+ self.operation_duration_seconds = HistogramVec::new(
+ histogram_opts!(
+ observe::METRIC_OPERATION_DURATION_SECONDS.name(),
+ observe::METRIC_OPERATION_DURATION_SECONDS.help(),
+ buckets
+ ),
+ &labels,
+ )
+ .unwrap();
+ self
+ }
+
+ fn with_operation_bytes(mut self, buckets: Vec<f64>) -> Self {
+ let labels = OperationLabels::names(false, self.path_label_level);
+ self.operation_bytes = HistogramVec::new(
+ histogram_opts!(
+ observe::METRIC_OPERATION_BYTES.name(),
+ observe::METRIC_OPERATION_BYTES.help(),
+ buckets
+ ),
+ &labels,
+ )
+ .unwrap();
+ self
+ }
+
+ fn with_path_label_level(mut self, level: usize) -> Self {
+ self.path_label_level = level;
+ self
+ }
+
+ fn register(&self, registry: &Registry) {
Review Comment:
We can call `register` inside the `layer` call.
##########
core/src/layers/observe/mod.rs:
##########
@@ -33,3 +34,37 @@ pub use metrics::LABEL_SCHEME;
pub use metrics::METRIC_OPERATION_BYTES;
pub use metrics::METRIC_OPERATION_DURATION_SECONDS;
pub use metrics::METRIC_OPERATION_ERRORS_TOTAL;
+
+pub(crate) fn path_label_value(path: &str, path_level: usize) -> Option<&str> {
Review Comment:
Please add comments to this function regarding its behavior.
##########
core/src/layers/prometheus.rs:
##########
@@ -218,6 +311,53 @@ impl PrometheusInterceptor {
}
}
+impl PrometheusInterceptor {
+ fn with_operation_duration_seconds(mut self, buckets: Vec<f64>) -> Self {
Review Comment:
We don't need to provide APIs at `PrometheusInterceptor` level.
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
We don't need this API. `PrometheusLayer::new()` should be enough.
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
Review Comment:
Let's build `interceptor` while calling `layer`.
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
Review Comment:
Hi, I don't think this change is necessary. We can implement `Default` for `PrometheusLayer` to use `default_registry()` by default, and offer a `new(registry: Registry)` method for users who want to use their own registry.
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
Review Comment:
We can take a `Registry` directly since it uses `Arc` internally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742064752
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
Review Comment:
Yes!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742066004
##########
core/src/layers/prometheus.rs:
##########
@@ -147,77 +224,68 @@ impl PrometheusLayer {
self.path_label_level = level;
self
}
-}
-impl<A: Access> Layer<A> for PrometheusLayer {
- type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
-
- fn layer(&self, inner: A) -> Self::LayeredAccess {
- let interceptor = PrometheusInterceptor::register(
- self.registry.clone(),
- self.operation_duration_seconds_buckets.clone(),
- self.operation_bytes_buckets.clone(),
- self.path_label_level,
- );
- observe::MetricsLayer::new(interceptor).layer(inner)
- }
-}
-
-#[derive(Clone, Debug)]
-pub struct PrometheusInterceptor {
- operation_duration_seconds: HistogramVec,
- operation_bytes: HistogramVec,
- operation_errors_total: GenericCounterVec<AtomicU64>,
- path_label_level: usize,
-}
-
-impl PrometheusInterceptor {
- fn register(
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
- ) -> Self {
- let labels = OperationLabels::names(false, path_label_level);
- let operation_duration_seconds = register_histogram_vec_with_registry!(
+ /// Register the metrics into the given registry and return a [`PrometheusLayer`].
+ pub fn register(self, registry: &Registry) -> PrometheusLayer {
Review Comment:
opendal only returns `opendal::Error`, we can wrap it inside.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1741935525
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
Review Comment:
How about removing `Default` for `PrometheusLayer`? We can expect all users build layers by our `builder` directly?
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
+ PrometheusLayerBuilder::default().register(registry)
+ }
+
+ /// Create a [`PrometheusLayerBuilder`] to modify the default metric configuration.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let duration_seconds_buckets = prometheus::exponential_buckets(0.01, 2.0, 16).unwrap();
+ /// let bytes_buckets = prometheus::exponential_buckets(1.0, 2.0, 16).unwrap();
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(
+ /// PrometheusLayer::builder()
+ /// .operation_duration_seconds_buckets(duration_seconds_buckets)
+ /// .operation_bytes_buckets(bytes_buckets)
+ /// .enable_path_label(1)
+ /// .register(registry)
+ /// )
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn builder() -> PrometheusLayerBuilder {
+ PrometheusLayerBuilder::default()
+ }
+}
+
+impl<A: Access> Layer<A> for PrometheusLayer {
+ type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
+
+ fn layer(&self, inner: A) -> Self::LayeredAccess {
+ observe::MetricsLayer::new(self.interceptor.clone()).layer(inner)
+ }
+}
+
+/// [`PrometheusLayerBuilder`] is a config builder to build a [`PrometheusLayer`].
+pub struct PrometheusLayerBuilder {
operation_duration_seconds_buckets: Vec<f64>,
operation_bytes_buckets: Vec<f64>,
path_label_level: usize,
}
-impl PrometheusLayer {
- /// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
+impl Default for PrometheusLayerBuilder {
Review Comment:
How about removing `Default` from `PrometheusLayerBuilder`? We can provide a private `new` function for `PrometheusLayerBuilder` to prevent users from creating a builder outside the layer.
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
Review Comment:
How about removing the `new` API? Offering a single method to build layers simplifies usage for users.
##########
core/src/layers/prometheus.rs:
##########
@@ -147,77 +224,68 @@ impl PrometheusLayer {
self.path_label_level = level;
self
}
-}
-impl<A: Access> Layer<A> for PrometheusLayer {
- type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
-
- fn layer(&self, inner: A) -> Self::LayeredAccess {
- let interceptor = PrometheusInterceptor::register(
- self.registry.clone(),
- self.operation_duration_seconds_buckets.clone(),
- self.operation_bytes_buckets.clone(),
- self.path_label_level,
- );
- observe::MetricsLayer::new(interceptor).layer(inner)
- }
-}
-
-#[derive(Clone, Debug)]
-pub struct PrometheusInterceptor {
- operation_duration_seconds: HistogramVec,
- operation_bytes: HistogramVec,
- operation_errors_total: GenericCounterVec<AtomicU64>,
- path_label_level: usize,
-}
-
-impl PrometheusInterceptor {
- fn register(
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
- ) -> Self {
- let labels = OperationLabels::names(false, path_label_level);
- let operation_duration_seconds = register_histogram_vec_with_registry!(
+ /// Register the metrics into the given registry and return a [`PrometheusLayer`].
+ pub fn register(self, registry: &Registry) -> PrometheusLayer {
Review Comment:
How about returning an error instead of just unwrapping?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742007852
##########
core/src/layers/prometheus.rs:
##########
@@ -93,32 +89,117 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
#[derive(Clone, Debug)]
pub struct PrometheusLayer {
- registry: Registry,
+ interceptor: PrometheusInterceptor,
+}
+
+impl Default for PrometheusLayer {
+ fn default() -> Self {
+ let register = prometheus::default_registry();
+ Self::new(register)
+ }
+}
+
+impl PrometheusLayer {
+ /// Create a [`PrometheusLayer`] and register its metrics to the given registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::new(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn new(registry: &Registry) -> Self {
+ PrometheusLayerBuilder::default().register(registry)
+ }
+
+ /// Create a [`PrometheusLayerBuilder`] to modify the default metric configuration.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let duration_seconds_buckets = prometheus::exponential_buckets(0.01, 2.0, 16).unwrap();
+ /// let bytes_buckets = prometheus::exponential_buckets(1.0, 2.0, 16).unwrap();
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(
+ /// PrometheusLayer::builder()
+ /// .operation_duration_seconds_buckets(duration_seconds_buckets)
+ /// .operation_bytes_buckets(bytes_buckets)
+ /// .enable_path_label(1)
+ /// .register(registry)
+ /// )
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn builder() -> PrometheusLayerBuilder {
+ PrometheusLayerBuilder::default()
+ }
+}
+
+impl<A: Access> Layer<A> for PrometheusLayer {
+ type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
+
+ fn layer(&self, inner: A) -> Self::LayeredAccess {
+ observe::MetricsLayer::new(self.interceptor.clone()).layer(inner)
+ }
+}
+
+/// [`PrometheusLayerBuilder`] is a config builder to build a [`PrometheusLayer`].
+pub struct PrometheusLayerBuilder {
operation_duration_seconds_buckets: Vec<f64>,
operation_bytes_buckets: Vec<f64>,
path_label_level: usize,
}
-impl PrometheusLayer {
- /// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
+impl Default for PrometheusLayerBuilder {
Review Comment:
We have not exported the `PrometheusLayerBuilder` type, so I think implementing `Default` might be more consistent with the current semantics (default config of metrics)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742016043
##########
core/src/layers/prometheus.rs:
##########
@@ -147,77 +224,68 @@ impl PrometheusLayer {
self.path_label_level = level;
self
}
-}
-impl<A: Access> Layer<A> for PrometheusLayer {
- type LayeredAccess = observe::MetricsAccessor<A, PrometheusInterceptor>;
-
- fn layer(&self, inner: A) -> Self::LayeredAccess {
- let interceptor = PrometheusInterceptor::register(
- self.registry.clone(),
- self.operation_duration_seconds_buckets.clone(),
- self.operation_bytes_buckets.clone(),
- self.path_label_level,
- );
- observe::MetricsLayer::new(interceptor).layer(inner)
- }
-}
-
-#[derive(Clone, Debug)]
-pub struct PrometheusInterceptor {
- operation_duration_seconds: HistogramVec,
- operation_bytes: HistogramVec,
- operation_errors_total: GenericCounterVec<AtomicU64>,
- path_label_level: usize,
-}
-
-impl PrometheusInterceptor {
- fn register(
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
- ) -> Self {
- let labels = OperationLabels::names(false, path_label_level);
- let operation_duration_seconds = register_histogram_vec_with_registry!(
+ /// Register the metrics into the given registry and return a [`PrometheusLayer`].
+ pub fn register(self, registry: &Registry) -> PrometheusLayer {
Review Comment:
Should we return `prometheus::Error` or `opendal::Error`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on PR #5072:
URL: https://github.com/apache/opendal/pull/5072#issuecomment-2326644773
There are some weird errors in CI
```
error[E0282]: type annotations needed
--> src/services/icloud/core.rs:530:25
|
530 | Some(it) => Ok(Some(it.drivewsid.clone())),
| ^^ cannot infer type of the type parameter `E` declared on the enum `Result`
|
help: consider specifying the generic arguments
|
530 | Some(it) => Ok::<std::option::Option<std::string::String>, E>(Some(it.drivewsid.clone())),
| +++++++++++++++++++++++++++++++++++++++++++++++
error[E0283]: type annotations needed
--> src/services/icloud/core.rs:530:25
|
530 | Some(it) => Ok(Some(it.drivewsid.clone())),
| ^^ cannot infer type of the type parameter `E` declared on the enum `Result`
531 | None => Ok(None),
532 | }?;
| - type must be known at this point
|
note: multiple `impl`s satisfying `types::error::Error: std::convert::From<_>` found
--> src/types/error.rs:418:1
|
418 | impl From<prometheus::Error> for Error {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: and another `impl` found in the `core` crate: `impl<T> std::convert::From<T> for T;`
= note: required for `std::result::Result<std::option::Option<std::string::String>, types::error::Error>` to implement `std::ops::FromResidual<std::result::Result<std::convert::Infallible, _>>`
help: consider specifying the generic arguments
|
530 | Some(it) => Ok::<std::option::Option<std::string::String>, E>(Some(it.drivewsid.clone())),
| +++++++++++++++++++++++++++++++++++++++++++++++
```
But if I change the icloud code, these errors are gone
```diff
- let id = match node.items.iter().find(|it| it.name == name) {
- Some(it) => Ok(Some(it.drivewsid.clone())),
- None => Ok(None),
- }?;
- Ok(id)
+ Ok(node
+ .items
+ .iter()
+ .find(|it| it.name == name)
+ .map(|it| it.drivewsid.clone()))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738054032
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
IMO, `PrometheusLayer::new` should just be a helper function that uses the default config and registers the metrics with the default config to the registry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide consistent APIs [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1738065511
##########
core/src/layers/prometheus.rs:
##########
@@ -93,47 +89,123 @@ use crate::*;
/// println!("## Prometheus Metrics");
/// println!("{}", String::from_utf8(buffer.clone()).unwrap());
/// Ok(())
-/// }
+/// # }
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
pub struct PrometheusLayer {
- registry: Registry,
- operation_duration_seconds_buckets: Vec<f64>,
- operation_bytes_buckets: Vec<f64>,
- path_label_level: usize,
+ interceptor: PrometheusInterceptor,
}
impl PrometheusLayer {
/// Create a [`PrometheusLayer`] while registering itself to this registry.
- pub fn new(registry: Registry) -> Self {
- Self {
- registry,
- operation_duration_seconds_buckets: exponential_buckets(0.01, 2.0, 16).unwrap(),
- operation_bytes_buckets: exponential_buckets(1.0, 2.0, 16).unwrap(),
- path_label_level: 0,
- }
+ pub fn new(registry: &Registry) -> Self {
+ let interceptor = PrometheusInterceptor::default();
+ Self { interceptor }.register(registry)
+ }
+
+ /// Register the metrics into the registry.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// # use log::debug;
+ /// # use opendal::layers::PrometheusLayer;
+ /// # use opendal::services;
+ /// # use opendal::Operator;
+ /// # use opendal::Result;
+ /// #
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// // Pick a builder and configure it.
+ /// let builder = services::Memory::default();
+ /// let registry = prometheus::default_registry();
+ ///
+ /// let op = Operator::new(builder)
+ /// .expect("must init")
+ /// .layer(PrometheusLayer::default().register(registry))
+ /// .finish();
+ /// debug!("operator: {op:?}");
+ ///
+ /// Ok(())
+ /// # }
+ /// ```
+ pub fn register(self, registry: &Registry) -> Self {
Review Comment:
So how about `PrometheusClientLayer` in #5073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5072:
URL: https://github.com/apache/opendal/pull/5072#issuecomment-2326835151
[`9b5c096` (#5072)](https://github.com/apache/opendal/pull/5072/commits/9b5c0968229d7d35970bf1ff05a303a75b61b49e)
I apologize for not providing a clear review. I suggest adding a `parse_prometheus_error(err: prometheus::Error) -> Error` as a standalone function in the Prometheus layer module itself, rather than within `Error`.
In this way, we keep our public types clean and easy to maintain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5072:
URL: https://github.com/apache/opendal/pull/5072#issuecomment-2326656956
> But if I change the icloud code, these errors are gone
I think it's a new Clippy lint introduced in the latest Rust version. Would you like to submit a new PR to address it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(layers/prometheus): provide builder APIs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5072:
URL: https://github.com/apache/opendal/pull/5072#discussion_r1742249533
##########
core/src/types/error.rs:
##########
@@ -414,6 +414,13 @@ impl From<Error> for io::Error {
}
}
+#[cfg(feature = "layers-prometheus")]
Review Comment:
Hi, please don't add `From` for `Error`. Implement a `parse_prometheus_error` instead.
##########
core/src/types/error.rs:
##########
@@ -414,6 +414,13 @@ impl From<Error> for io::Error {
}
}
+#[cfg(feature = "layers-prometheus")]
+impl From<prometheus::Error> for Error {
+ fn from(err: prometheus::Error) -> Self {
+ Self::new(ErrorKind::Unexpected, err.to_string())
Review Comment:
Please add a short desc for this error, also add the source error in `source`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org