refactor histograms to support different buckets

Author: pietroalbini

src/metrics/histogram.rs

@@ -0,0 +1,96 @@
+use crate::metrics::macros::MetricFromOpts;
+use std::marker::PhantomData;
+use std::ops::{Deref, DerefMut};
+
+/// Prometheus's histograms work by dividing datapoints in buckets, with each bucket containing the
+/// count of datapoints equal or greater to the bucket value.
+///
+/// Histogram buckets are not an exact science, so feel free to tweak the buckets or create new
+/// ones if you see that the histograms are not really accurate. Just avoid adding too many buckets
+/// for a single type as that increases the number of exported metric series.
+pub trait HistogramBuckets {
+    const BUCKETS: &'static [f64];
+}
+
+/// Buckets geared towards measuring timings, such as the response time of our requests, going from
+/// 0.5ms to 100ms with a higher resolution and from 100ms to 5 seconds with a slightly lower
+/// resolution. This allows us to properly measure download requests (which take around 1ms) and
+/// other requests (our 95h is around 10-20ms).
+pub struct TimingBuckets;
+
+impl HistogramBuckets for TimingBuckets {
+    const BUCKETS: &'static [f64] = &[
+        0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.5, 1.0, 5.0,
+    ];
+}
+
+/// Wrapper type over [`prometheus::Histogram`] to support defining buckets.
+pub struct Histogram<B: HistogramBuckets> {
+    inner: prometheus::Histogram,
+    _phantom: PhantomData<B>,
+}
+
+impl<B: HistogramBuckets> MetricFromOpts for Histogram<B> {
+    fn from_opts(opts: prometheus::Opts) -> Result<Self, prometheus::Error> {
+        Ok(Histogram {
+            inner: prometheus::Histogram::with_opts(prometheus::HistogramOpts {
+                common_opts: opts,
+                buckets: B::BUCKETS.to_vec(),
+            })?,
+            _phantom: PhantomData,
+        })
+    }
+}
+
+impl<B: HistogramBuckets> Deref for Histogram<B> {
+    type Target = prometheus::Histogram;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<B: HistogramBuckets> DerefMut for Histogram<B> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.inner
+    }
+}
+
+/// Wrapper type over [`prometheus::HistogramVec`] to support defining buckets.
+pub struct HistogramVec<B: HistogramBuckets> {
+    inner: prometheus::HistogramVec,
+    _phantom: PhantomData<B>,
+}
+
+impl<B: HistogramBuckets> MetricFromOpts for HistogramVec<B> {
+    fn from_opts(opts: prometheus::Opts) -> Result<Self, prometheus::Error> {
+        Ok(HistogramVec {
+            inner: prometheus::HistogramVec::new(
+                prometheus::HistogramOpts {
+                    common_opts: opts.clone(),
+                    buckets: B::BUCKETS.to_vec(),
+                },
+                opts.variable_labels
+                    .iter()
+                    .map(|s| s.as_str())
+                    .collect::<Vec<_>>()
+                    .as_slice(),
+            )?,
+            _phantom: PhantomData,
+        })
+    }
+}
+
+impl<B: HistogramBuckets> Deref for HistogramVec<B> {
+    type Target = prometheus::HistogramVec;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<B: HistogramBuckets> DerefMut for HistogramVec<B> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.inner
+    }
+}

src/metrics/instance.rs

@@ -17,11 +17,10 @@
 //! As a rule of thumb, if the metric requires a database query to be updated it's probably a
 //! service-level metric, and you should add it to `src/metrics/service.rs` instead.
 
+use crate::metrics::histogram::{Histogram, HistogramVec, TimingBuckets};
 use crate::util::errors::AppResult;
 use crate::{app::App, db::DieselPool};
-use prometheus::{
-    proto::MetricFamily, Histogram, HistogramVec, IntCounter, IntCounterVec, IntGauge, IntGaugeVec,
-};
+use prometheus::{proto::MetricFamily, IntCounter, IntCounterVec, IntGauge, IntGaugeVec};
 
 metrics! {
     pub struct InstanceMetrics {
@@ -30,15 +29,15 @@ metrics! {
         /// Number of used database connections in the pool
         database_used_conns: IntGaugeVec["pool"],
         /// Amount of time required to obtain a database connection
-        pub database_time_to_obtain_connection: HistogramVec["pool"],
+        pub database_time_to_obtain_connection: HistogramVec<TimingBuckets>["pool"],
 
         /// Number of requests processed by this instance
         pub requests_total: IntCounter,
         /// Number of requests currently being processed
         pub requests_in_flight: IntGauge,
 
         /// Response times of our endpoints
-        pub response_times: HistogramVec["endpoint"],
+        pub response_times: HistogramVec<TimingBuckets>["endpoint"],
         /// Nmber of responses per status code
         pub responses_by_status_code_total: IntCounterVec["status"],
 
@@ -47,7 +46,7 @@ metrics! {
         /// Number of download requests with a non-canonical crate name.
         pub downloads_non_canonical_crate_name_total: IntCounter,
         /// How long it takes to execute the SELECT query in the download endpoint.
-        pub downloads_select_query_execution_time: Histogram,
+        pub downloads_select_query_execution_time: Histogram<TimingBuckets>,
         /// Number of download requests that are not counted yet.
         downloads_not_counted_total: IntGauge,
     }

src/metrics/macros.rs

@@ -1,19 +1,4 @@
-use prometheus::{Histogram, HistogramOpts, HistogramVec, Opts};
-
-/// Prometheus's histograms work by dividing datapoints in buckets, with each bucket containing
-/// the count of datapoints equal or greater to the bucket value.
-///
-/// The buckets used by crates.io are geared towards measuring the response time of our requests,
-/// going from 0.5ms to 100ms with a higher resolution and from 100ms to 5 seconds with a slightly
-/// lower resolution. This allows us to properly measure download requests (which take around 1ms)
-/// and other requests (our 95h is around 10-20ms).
-///
-/// Histogram buckets are not an exact science, so feel free to tweak the buckets if you see that
-/// the histograms are not really accurate. Just avoid adding too many buckets as that increases
-/// the number of exported metric series.
-const HISTOGRAM_BUCKETS: &[f64] = &[
-    0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.5, 1.0, 5.0,
-];
+use prometheus::Opts;
 
 pub(super) trait MetricFromOpts: Sized {
     fn from_opts(opts: Opts) -> Result<Self, prometheus::Error>;
@@ -105,29 +90,4 @@ load_metric_type!(GaugeVec as vec);
 load_metric_type!(IntGauge as single);
 load_metric_type!(IntGaugeVec as vec);
 
-// Use a custom implementation for histograms to customize the buckets.
-
-impl MetricFromOpts for Histogram {
-    fn from_opts(opts: Opts) -> Result<Self, prometheus::Error> {
-        Histogram::with_opts(HistogramOpts {
-            common_opts: opts,
-            buckets: HISTOGRAM_BUCKETS.to_vec(),
-        })
-    }
-}
-
-impl MetricFromOpts for HistogramVec {
-    fn from_opts(opts: Opts) -> Result<Self, prometheus::Error> {
-        HistogramVec::new(
-            HistogramOpts {
-                common_opts: opts.clone(),
-                buckets: HISTOGRAM_BUCKETS.to_vec(),
-            },
-            opts.variable_labels
-                .iter()
-                .map(|s| s.as_str())
-                .collect::<Vec<_>>()
-                .as_slice(),
-        )
-    }
-}
+// Histograms are defined in histogram.rs

src/metrics/mod.rs

@@ -5,6 +5,7 @@ pub use self::service::ServiceMetrics;
 #[macro_use]
 mod macros;
 
+mod histogram;
 mod instance;
 mod log_encoder;
 mod service;