From c1bb0e2cc1fdf31632129503c41b18fe9608ac82 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 29 Sep 2022 15:20:55 +0100 Subject: [PATCH] src/registry: Use dynamic dispatch and remove type parameter M Instead of being generic over the metric type on `Registry`, use dynamic dispatch. On the upside this significantly simplifies type signatures exposed to the user. On the downside this requires users to always use dynamic dispatch and requires all metrics to implement auxiliary trait `Debug` and auto traits `Send` and `Sync`. --- CHANGELOG.md | 2 + examples/actix-web.rs | 12 ++-- examples/custom-metric.rs | 1 + examples/tide.rs | 6 +- src/encoding/proto.rs | 12 ++-- src/encoding/text.rs | 22 +------ src/lib.rs | 6 +- src/metrics/family.rs | 13 +++- src/registry.rs | 122 +++++++++++++++++++++++++------------- 9 files changed, 113 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 901b2c91..0283893e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Always use dynamic dispatch on Registry, i.e. remove generic type parameter `M` from `Registry`. + - Move`Encode` trait from `prometheus_client::encoding::text` to `prometheus_client::encoding`. See [PR 83]. [PR 83]: https://github.com/prometheus/client_rust/pull/83 diff --git a/examples/actix-web.rs b/examples/actix-web.rs index 80144aa2..5b59cd2c 100644 --- a/examples/actix-web.rs +++ b/examples/actix-web.rs @@ -7,13 +7,13 @@ use prometheus_client::metrics::counter::Counter; use prometheus_client::metrics::family::Family; use prometheus_client::registry::Registry; -#[derive(Clone, Hash, PartialEq, Eq, Encode)] +#[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] pub enum Method { Get, Post, } -#[derive(Clone, Hash, PartialEq, Eq, Encode)] +#[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] pub struct MethodLabels { pub method: Method, } @@ -55,11 +55,9 @@ async fn main() -> std::io::Result<()> { let mut state = AppState { registry: Registry::default(), }; - state.registry.register( - "requests", - "Count of requests", - Box::new(metrics.requests.clone()), - ); + state + .registry + .register("requests", "Count of requests", metrics.requests.clone()); let state = web::Data::new(Mutex::new(state)); HttpServer::new(move || { diff --git a/examples/custom-metric.rs b/examples/custom-metric.rs index 7fcda080..fe8f3cef 100644 --- a/examples/custom-metric.rs +++ b/examples/custom-metric.rs @@ -7,6 +7,7 @@ use prometheus_client::registry::Registry; /// Related to the concept of "Custom Collectors" in other implementations. /// /// [`MyCustomMetric`] generates and encodes a random number on each scrape. +#[derive(Debug)] struct MyCustomMetric {} impl EncodeMetric for MyCustomMetric { diff --git a/examples/tide.rs b/examples/tide.rs index 244db06d..677105d3 100644 --- a/examples/tide.rs +++ b/examples/tide.rs @@ -44,13 +44,13 @@ async fn main() -> std::result::Result<(), std::io::Error> { Ok(()) } -#[derive(Clone, Hash, PartialEq, Eq, Encode)] +#[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] struct Labels { method: Method, path: String, } -#[derive(Clone, Hash, PartialEq, Eq, Encode)] +#[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] enum Method { Get, Put, @@ -58,7 +58,7 @@ enum Method { #[derive(Clone)] struct State { - registry: Arc>>, + registry: Arc, } #[derive(Default)] diff --git a/src/encoding/proto.rs b/src/encoding/proto.rs index 1230ee4b..f183551c 100644 --- a/src/encoding/proto.rs +++ b/src/encoding/proto.rs @@ -47,7 +47,7 @@ pub use prometheus_client_derive_encode::*; /// Encode the metrics registered with the provided [`Registry`] into MetricSet /// using the OpenMetrics protobuf format. -pub fn encode(registry: &Registry) -> openmetrics_data_model::MetricSet +pub fn encode(registry: &Registry) -> openmetrics_data_model::MetricSet where M: EncodeMetric, { @@ -57,7 +57,8 @@ where let mut family = openmetrics_data_model::MetricFamily { name: desc.name().to_string(), r#type: { - let metric_type: openmetrics_data_model::MetricType = metric.metric_type().into(); + let metric_type: openmetrics_data_model::MetricType = + EncodeMetric::metric_type(metric.as_ref()).into(); metric_type as i32 }, unit: if let Some(unit) = desc.unit() { @@ -71,7 +72,7 @@ where let mut labels = vec![]; desc.labels().encode(&mut labels); - metric.encode(labels, &mut family.metrics); + EncodeMetric::encode(metric.as_ref(), labels, &mut family.metrics); metric_set.metric_families.push(family); } @@ -118,11 +119,6 @@ impl EncodeMetric for Box { } } -/// Trait combining [`EncodeMetric`] and [`Send`]. -pub trait SendEncodeMetric: EncodeMetric + Send {} - -impl SendEncodeMetric for T {} - /// Trait to implement its label encoding in the OpenMetrics protobuf format. pub trait EncodeLabels { /// Encode the given instance into Labels in the OpenMetrics protobuf diff --git a/src/encoding/text.rs b/src/encoding/text.rs index 754b08a7..7504b97b 100644 --- a/src/encoding/text.rs +++ b/src/encoding/text.rs @@ -40,10 +40,9 @@ use std::ops::Deref; /// Encode the metrics registered with the provided [`Registry`] into the /// provided [`Write`]r using the OpenMetrics text format. -pub fn encode(writer: &mut W, registry: &Registry) -> Result<(), std::io::Error> +pub fn encode(writer: &mut W, registry: &Registry) -> Result<(), std::io::Error> where W: Write, - M: EncodeMetric, { for (desc, metric) in registry.iter() { writer.write_all(b"# HELP ")?; @@ -63,7 +62,7 @@ where unit.encode(writer)?; } writer.write_all(b" ")?; - metric.metric_type().encode(writer)?; + EncodeMetric::metric_type(metric.as_ref()).encode(writer)?; writer.write_all(b"\n")?; if let Some(unit) = desc.unit() { @@ -84,7 +83,7 @@ where labels: None, }; - metric.encode(encoder)?; + EncodeMetric::encode(metric.as_ref(), encoder)?; } writer.write_all(b"# EOF\n")?; @@ -402,21 +401,6 @@ impl EncodeMetric for Box { } } -/// Trait combining [`EncodeMetric`], [`Send`] and [`Sync`]. -pub trait SendSyncEncodeMetric: EncodeMetric + Send + Sync {} - -impl SendSyncEncodeMetric for T {} - -impl EncodeMetric for Box { - fn encode(&self, encoder: Encoder) -> Result<(), std::io::Error> { - self.deref().encode(encoder) - } - - fn metric_type(&self) -> MetricType { - self.deref().metric_type() - } -} - ///////////////////////////////////////////////////////////////////////////////// // Counter diff --git a/src/lib.rs b/src/lib.rs index dbc70f31..061b5cf7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,7 +30,7 @@ //! // //! // You could as well use `(String, String)` to represent a label set, //! // instead of the custom type below. -//! #[derive(Clone, Hash, PartialEq, Eq, Encode)] +//! #[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] //! struct Labels { //! // Use your own enum types to represent label values. //! method: Method, @@ -38,7 +38,7 @@ //! path: String, //! }; //! -//! #[derive(Clone, Hash, PartialEq, Eq, Encode)] +//! #[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] //! enum Method { //! GET, //! PUT, @@ -54,7 +54,7 @@ //! "http_requests", //! // And the metric help text. //! "Number of HTTP requests received", -//! Box::new(http_requests.clone()), +//! http_requests.clone(), //! ); //! //! // Somewhere in your business logic record a single HTTP GET request. diff --git a/src/metrics/family.rs b/src/metrics/family.rs index 304b0e43..9b170d59 100644 --- a/src/metrics/family.rs +++ b/src/metrics/family.rs @@ -65,12 +65,12 @@ use std::sync::Arc; /// # use std::io::Write; /// # /// # let mut registry = Registry::default(); -/// #[derive(Clone, Hash, PartialEq, Eq, Encode)] +/// #[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] /// struct Labels { /// method: Method, /// }; /// -/// #[derive(Clone, Hash, PartialEq, Eq, Encode)] +/// #[derive(Clone, Debug, Hash, PartialEq, Eq, Encode)] /// enum Method { /// GET, /// PUT, @@ -97,7 +97,6 @@ use std::sync::Arc; /// # assert_eq!(expected, String::from_utf8(buffer).unwrap()); /// ``` // TODO: Consider exposing hash algorithm. -#[derive(Debug)] pub struct Family M> { metrics: Arc>>, /// Function that when called constructs a new metric. @@ -111,6 +110,14 @@ pub struct Family M> { constructor: C, } +impl std::fmt::Debug for Family { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Family") + .field("metrics", &self.metrics) + .finish() + } +} + /// A constructor for creating new metrics in a [`Family`] when calling /// [`Family::get_or_create`]. Such constructor is provided via /// [`Family::new_with_constructor`]. diff --git a/src/registry.rs b/src/registry.rs index b88dc46b..e69ad3b9 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -25,10 +25,7 @@ use std::borrow::Cow; /// # use prometheus_client::registry::Registry; /// # /// // Create a metric registry. -/// // -/// // Note the angle brackets to make sure to use the default (dynamic -/// // dispatched boxed metric) for the generic type parameter. -/// let mut registry = ::default(); +/// let mut registry = Registry::default(); /// /// let counter: Counter = Counter::default(); /// let gauge: Gauge = Gauge::default(); @@ -36,12 +33,12 @@ use std::borrow::Cow; /// registry.register( /// "my_counter", /// "This is my counter", -/// Box::new(counter.clone()), +/// counter.clone(), /// ); /// registry.register( /// "my_gauge", /// "This is my gauge", -/// Box::new(gauge.clone()), +/// gauge.clone(), /// ); /// /// # // Encode all metrics in the registry in the text format. @@ -58,14 +55,14 @@ use std::borrow::Cow; /// # assert_eq!(expected, String::from_utf8(buffer).unwrap()); /// ``` #[derive(Debug)] -pub struct Registry> { +pub struct Registry { prefix: Option, labels: Vec<(Cow<'static, str>, Cow<'static, str>)>, - metrics: Vec<(Descriptor, M)>, - sub_registries: Vec>, + metrics: Vec<(Descriptor, Box)>, + sub_registries: Vec, } -impl Default for Registry { +impl Default for Registry { fn default() -> Self { Self { prefix: None, @@ -76,7 +73,7 @@ impl Default for Registry { } } -impl Registry { +impl Registry { /// Creates a new default [`Registry`] with the given prefix. pub fn with_prefix(prefix: impl Into) -> Self { Self { @@ -103,12 +100,17 @@ impl Registry { /// # use prometheus_client::metrics::counter::{Atomic as _, Counter}; /// # use prometheus_client::registry::{Registry, Unit}; /// # - /// let mut registry: Registry = Registry::default(); - /// let counter = Counter::default(); + /// let mut registry = Registry::default(); + /// let counter: Counter = Counter::default(); /// /// registry.register("my_counter", "This is my counter", counter.clone()); /// ``` - pub fn register, H: Into>(&mut self, name: N, help: H, metric: M) { + pub fn register, H: Into>( + &mut self, + name: N, + help: H, + metric: impl Metric, + ) { self.priv_register(name, help, metric, None) } @@ -124,8 +126,8 @@ impl Registry { /// # use prometheus_client::metrics::counter::{Atomic as _, Counter}; /// # use prometheus_client::registry::{Registry, Unit}; /// # - /// let mut registry: Registry = Registry::default(); - /// let counter = Counter::default(); + /// let mut registry = Registry::default(); + /// let counter: Counter = Counter::default(); /// /// registry.register_with_unit( /// "my_counter", @@ -139,7 +141,7 @@ impl Registry { name: N, help: H, unit: Unit, - metric: M, + metric: impl Metric, ) { self.priv_register(name, help, metric, Some(unit)) } @@ -148,7 +150,7 @@ impl Registry { &mut self, name: N, help: H, - metric: M, + metric: impl Metric, unit: Option, ) { let name = name.into(); @@ -164,7 +166,7 @@ impl Registry { labels: self.labels.clone(), }; - self.metrics.push((descriptor, metric)); + self.metrics.push((descriptor, Box::new(metric))); } // TODO: Update doc. @@ -183,17 +185,17 @@ impl Registry { /// # use prometheus_client::metrics::counter::{Atomic as _, Counter}; /// # use prometheus_client::registry::{Registry, Unit}; /// # - /// let mut registry: Registry = Registry::default(); + /// let mut registry = Registry::default(); /// - /// let subsystem_a_counter_1 = Counter::default(); - /// let subsystem_a_counter_2 = Counter::default(); + /// let subsystem_a_counter_1: Counter = Counter::default(); + /// let subsystem_a_counter_2: Counter = Counter::default(); /// /// let subsystem_a_registry = registry.sub_registry_with_prefix("subsystem_a"); /// registry.register("counter_1", "", subsystem_a_counter_1.clone()); /// registry.register("counter_2", "", subsystem_a_counter_2.clone()); /// - /// let subsystem_b_counter_1 = Counter::default(); - /// let subsystem_b_counter_2 = Counter::default(); + /// let subsystem_b_counter_1: Counter = Counter::default(); + /// let subsystem_b_counter_2: Counter = Counter::default(); /// /// let subsystem_a_registry = registry.sub_registry_with_prefix("subsystem_b"); /// registry.register("counter_1", "", subsystem_b_counter_1.clone()); @@ -239,7 +241,7 @@ impl Registry { } /// [`Iterator`] over all metrics registered with the [`Registry`]. - pub fn iter(&self) -> RegistryIterator { + pub fn iter(&self) -> RegistryIterator { let metrics = self.metrics.iter(); let sub_registries = self.sub_registries.iter(); RegistryIterator { @@ -253,14 +255,14 @@ impl Registry { /// Iterator iterating both the metrics registered directly with the registry as /// well as all metrics registered with sub-registries. #[derive(Debug)] -pub struct RegistryIterator<'a, M> { - metrics: std::slice::Iter<'a, (Descriptor, M)>, - sub_registries: std::slice::Iter<'a, Registry>, - sub_registry: Option>>, +pub struct RegistryIterator<'a> { + metrics: std::slice::Iter<'a, (Descriptor, Box)>, + sub_registries: std::slice::Iter<'a, Registry>, + sub_registry: Option>>, } -impl<'a, M> Iterator for RegistryIterator<'a, M> { - type Item = &'a (Descriptor, M); +impl<'a> Iterator for RegistryIterator<'a> { + type Item = &'a (Descriptor, Box); fn next(&mut self) -> Option { if let Some(metric) = self.metrics.next() { @@ -365,6 +367,45 @@ impl Unit { } } +// TODO: Instead of depending on dynamic dispatch for all metrics, we could use +// static dispatch for the most common ones and only fall back to dynamic +// dispatch. That said, this needs to be proven performant in a benchmark first. +/// Super trait representing an abstract Prometheus metric. +#[cfg(not(feature = "protobuf"))] +pub trait Metric: + crate::encoding::text::EncodeMetric + Send + Sync + std::fmt::Debug + 'static +{ +} + +/// Super trait representing an abstract Prometheus metric. +#[cfg(feature = "protobuf")] +pub trait Metric: + crate::encoding::text::EncodeMetric + + crate::encoding::proto::EncodeMetric + + Send + + Sync + + std::fmt::Debug + + 'static +{ +} + +#[cfg(not(feature = "protobuf"))] +impl Metric for T where + T: crate::encoding::text::EncodeMetric + Send + Sync + std::fmt::Debug + 'static +{ +} + +#[cfg(feature = "protobuf")] +impl Metric for T where + T: crate::encoding::text::EncodeMetric + + crate::encoding::proto::EncodeMetric + + Send + + Sync + + std::fmt::Debug + + 'static +{ +} + #[cfg(test)] mod tests { use super::*; @@ -372,8 +413,8 @@ mod tests { #[test] fn register_and_iterate() { - let mut registry: Registry = Registry::default(); - let counter = Counter::default(); + let mut registry = Registry::default(); + let counter: Counter = Counter::default(); registry.register("my_counter", "My counter", counter); assert_eq!(1, registry.iter().count()) @@ -382,28 +423,29 @@ mod tests { #[test] fn sub_registry_with_prefix_and_label() { let top_level_metric_name = "my_top_level_metric"; - let mut registry = Registry::::default(); - registry.register(top_level_metric_name, "some help", Default::default()); + let mut registry = Registry::default(); + let counter: Counter = Counter::default(); + registry.register(top_level_metric_name, "some help", counter.clone()); let prefix_1 = "prefix_1"; let prefix_1_metric_name = "my_prefix_1_metric"; let sub_registry = registry.sub_registry_with_prefix(prefix_1); - sub_registry.register(prefix_1_metric_name, "some help", Default::default()); + sub_registry.register(prefix_1_metric_name, "some help", counter.clone()); let prefix_1_1 = "prefix_1_1"; let prefix_1_1_metric_name = "my_prefix_1_1_metric"; let sub_sub_registry = sub_registry.sub_registry_with_prefix(prefix_1_1); - sub_sub_registry.register(prefix_1_1_metric_name, "some help", Default::default()); + sub_sub_registry.register(prefix_1_1_metric_name, "some help", counter.clone()); let label_1_2 = (Cow::Borrowed("registry"), Cow::Borrowed("1_2")); let prefix_1_2_metric_name = "my_prefix_1_2_metric"; let sub_sub_registry = sub_registry.sub_registry_with_label(label_1_2.clone()); - sub_sub_registry.register(prefix_1_2_metric_name, "some help", Default::default()); + sub_sub_registry.register(prefix_1_2_metric_name, "some help", counter.clone()); let prefix_1_2_1 = "prefix_1_2_1"; let prefix_1_2_1_metric_name = "my_prefix_1_2_1_metric"; let sub_sub_sub_registry = sub_sub_registry.sub_registry_with_prefix(prefix_1_2_1); - sub_sub_sub_registry.register(prefix_1_2_1_metric_name, "some help", Default::default()); + sub_sub_sub_registry.register(prefix_1_2_1_metric_name, "some help", counter.clone()); let prefix_2 = "prefix_2"; let _ = registry.sub_registry_with_prefix(prefix_2); @@ -411,7 +453,7 @@ mod tests { let prefix_3 = "prefix_3"; let prefix_3_metric_name = "my_prefix_3_metric"; let sub_registry = registry.sub_registry_with_prefix(prefix_3); - sub_registry.register(prefix_3_metric_name, "some help", Default::default()); + sub_registry.register(prefix_3_metric_name, "some help", counter.clone()); let mut metric_iter = registry .iter()