Skip to content
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

refactor(collector): have Registry:encode do the encoding #149

Merged
merged 9 commits into from
Jul 12, 2023

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jun 4, 2023

Let's face it, the Collector trait I designed in #82 is too complex.

Instead of having Registry provide access to its inner Metrics, have
Registry encode the Metrics directly via Registry::encode. In the same
vain, have Collector::encode encode the collected Metrics directly.

This allows for a much simpler Collector trait, removing all complexity due to
providing access to a Metric externally.

/// The [`Collector`] abstraction allows users to provide additional metrics and
/// their description on each scrape.
///
/// An example use-case is an exporter that retrieves a set of operating system metrics
/// ad-hoc on each scrape.
///
/// Register a [`Collector`] with a [`Registry`](crate::registry::Registry) via
/// [`Registry::register_collector`](crate::registry::Registry::register_collector).
///
/// ```
/// # use prometheus_client::metrics::counter::ConstCounter;
/// # use prometheus_client::collector::Collector;
/// # use prometheus_client::encoding::{DescriptorEncoder, EncodeMetric};
/// #
/// #[derive(Debug)]
/// struct MyCollector {}
///
/// impl Collector for MyCollector {
///     fn encode(&self, mut encoder: DescriptorEncoder) -> Result<(), std::fmt::Error> {
///         let counter = ConstCounter::new(42);
///         let metric_encoder = encoder.encode_descriptor(
///             "my_counter",
///             "some help",
///             None,
///             counter.metric_type(),
///         )?;
///         counter.encode(metric_encoder)?;
///         Ok(())
///     }
/// }
/// ```
pub trait Collector: std::fmt::Debug + Send + Sync + 'static {
    /// Once the [`Collector`] is registered, this method is called on each scrape.
    fn encode<'a>(&'a self, encoder: DescriptorEncoder) -> Result<(), std::fmt::Error>;
}

mxinden added 4 commits June 3, 2023 13:12
Instead of having `Registry` provide access to its inner `Metric`s, have
`Registry` encode the `Metric`s directly via `Registry::encode`. In the same
vain, have `Collector::encode` encode the collected `Metric`s directly.

This allows for a much simpler `Collector` trait, removing all complexity due to
providing access to a `Metric` externally.

``` rust
/// The [`Collector`] abstraction allows users to provide additional metrics and
/// their description on each scrape.
///
/// An example use-case is an exporter that retrieves a set of operating system metrics
/// ad-hoc on each scrape.
///
/// Register a [`Collector`] with a [`Registry`](crate::registry::Registry) via
/// [`Registry::register_collector`](crate::registry::Registry::register_collector).
///
/// ```
/// # use prometheus_client::metrics::counter::ConstCounter;
/// # use prometheus_client::collector::Collector;
/// # use prometheus_client::encoding::{DescriptorEncoder, EncodeMetric};
/// #
/// #[derive(Debug)]
/// struct MyCollector {}
///
/// impl Collector for MyCollector {
///     fn encode<'a>(&'a self, mut encoder: DescriptorEncoder) -> Result<(), std::fmt::Error> {
///         let counter = ConstCounter::new(42);
///         let metric_encoder = encoder.encode_descriptor(
///             "my_counter",
///             "some help",
///             None,
///             counter.metric_type(),
///         )?;
///         counter.encode(metric_encoder)?;
///         Ok(())
///     }
/// }
/// ```
pub trait Collector: std::fmt::Debug + Send + Sync + 'static {
    /// Once the [`Collector`] is registered, this method is called on each scrape.
    ///
    /// Note that the return type allows you to either return owned (convenient)
    /// or borrowed (performant) descriptions and metrics.
    fn encode<'a>(&'a self, encoder: DescriptorEncoder) -> Result<(), std::fmt::Error>;
}
```
@mxinden
Copy link
Member Author

mxinden commented Jul 5, 2023

@thomaseizinger based on your comments in #82 (comment) I think you will like this change.

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jul 8, 2023
Updates to `prometheus-client` `Collector` trait refactor introduced with
prometheus/client_rust#149.
@thomaseizinger
Copy link
Contributor

I think I was able to simplify this a bit in mxinden#2.

@amunra amunra mentioned this pull request Jul 11, 2023
@mxinden mxinden merged commit 5dd49ce into prometheus:master Jul 12, 2023
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Oct 25, 2023
Updates to `prometheus-client` `Collector` trait refactor introduced with prometheus/client_rust#149.

Pull-Request: #4160.
@gurinderu
Copy link

@mxinden looks like there is no way to add custom labels with the new API. Probably we should add an additional encode_descriptor function with optional labels. WDYT?

@mxinden
Copy link
Member Author

mxinden commented Nov 14, 2023

Does the encode_family method solve your use-case? Example:

https://github.com/libp2p/rust-libp2p/blob/cddc306978a7cca705de5ce10ec5e16e8b732084/misc/metrics/src/identify.rs#L211-L223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants