From da591802af2b09528424126db1ecc14a5f571f53 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Fri, 10 Aug 2018 10:20:47 -0700 Subject: [PATCH] Extract metrics::Scopes into its own module (#55) The metrics::Scopes type exposes its internal implementation to many of its uses. By extracting the type into its own module, we are forced to provide an explicit public interface, hiding its IndexMap implementation details. --- src/telemetry/metrics/http.rs | 4 +- src/telemetry/metrics/mod.rs | 98 ++++++++++-------------------- src/telemetry/metrics/record.rs | 29 ++++----- src/telemetry/metrics/scopes.rs | 50 +++++++++++++++ src/telemetry/metrics/transport.rs | 4 +- 5 files changed, 101 insertions(+), 84 deletions(-) create mode 100644 src/telemetry/metrics/scopes.rs diff --git a/src/telemetry/metrics/http.rs b/src/telemetry/metrics/http.rs index 201174559..477a05f7a 100644 --- a/src/telemetry/metrics/http.rs +++ b/src/telemetry/metrics/http.rs @@ -36,7 +36,7 @@ impl RequestScopes { impl fmt::Display for RequestScopes { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.scopes.is_empty() { + if self.is_empty() { return Ok(()); } @@ -74,7 +74,7 @@ impl ResponseScopes { impl fmt::Display for ResponseScopes { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.scopes.is_empty() { + if self.is_empty() { return Ok(()); } diff --git a/src/telemetry/metrics/mod.rs b/src/telemetry/metrics/mod.rs index f56138f47..5522866da 100644 --- a/src/telemetry/metrics/mod.rs +++ b/src/telemetry/metrics/mod.rs @@ -33,8 +33,6 @@ use std::marker::PhantomData; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; -use indexmap::IndexMap; - use ctx; mod counter; mod gauge; @@ -43,6 +41,7 @@ mod http; mod labels; pub mod latency; mod record; +mod scopes; mod serve; mod transport; @@ -57,6 +56,7 @@ use self::labels::{ }; pub use self::labels::DstLabels; pub use self::record::Record; +pub use self::scopes::Scopes; pub use self::serve::Serve; use super::process; use super::tls_config_reload; @@ -100,15 +100,6 @@ struct Root { process: process::Report, } - -/// Holds an `S`-typed scope for each `L`-typed label set. -/// -/// An `S` type typically holds one or more metrics. -#[derive(Debug)] -pub struct Scopes { - scopes: IndexMap, -} - #[derive(Debug)] struct Stamped { stamp: Instant, @@ -150,7 +141,7 @@ impl<'a, M: FmtMetric> Metric<'a, M> { scopes: &Scopes, to_metric: F )-> fmt::Result { - for (labels, scope) in &scopes.scopes { + for (labels, scope) in scopes { to_metric(scope).fmt_metric_labeled(f, self.name, labels)?; } @@ -170,30 +161,25 @@ impl Root { } fn request(&mut self, labels: RequestLabels) -> &mut http::RequestMetrics { - self.requests.scopes.entry(labels) - .or_insert_with(|| http::RequestMetrics::default().into()) - .stamped() + self.requests.get_or_default(labels).stamped() + } fn response(&mut self, labels: ResponseLabels) -> &mut http::ResponseMetrics { - self.responses.scopes.entry(labels) - .or_insert_with(|| http::ResponseMetrics::default().into()) - .stamped() + self.responses.get_or_default(labels).stamped() } fn transport(&mut self, labels: TransportLabels) -> &mut transport::OpenMetrics { - self.transports.scopes.entry(labels) - .or_insert_with(|| transport::OpenMetrics::default()) + self.transports.get_or_default(labels) } fn transport_close(&mut self, labels: TransportCloseLabels) -> &mut transport::CloseMetrics { - self.transport_closes.scopes.entry(labels) - .or_insert_with(|| transport::CloseMetrics::default()) + self.transport_closes.get_or_default(labels) } fn retain_since(&mut self, epoch: Instant) { - self.requests.retain_since(epoch); - self.responses.retain_since(epoch); + self.requests.retain(|_, v| v.stamp >= epoch); + self.responses.retain(|_, v| v.stamp >= epoch); } } @@ -219,6 +205,12 @@ impl Stamped { } } +impl Default for Stamped { + fn default() -> Self { + T::default().into() + } +} + impl From for Stamped { fn from(inner: T) -> Self { Self { @@ -235,32 +227,6 @@ impl ::std::ops::Deref for Stamped { } } -// ===== impl Scopes ===== - -impl Default for Scopes { - fn default() -> Self { - Scopes { scopes: IndexMap::default(), } - } -} - -impl Scopes { - pub fn is_empty(&self) -> bool { - self.scopes.is_empty() - } -} - -impl Scopes { - pub fn get_or_default(&mut self, key: L) -> &mut S { - self.scopes.entry(key).or_insert_with(|| S::default()) - } -} - -impl Scopes> { - fn retain_since(&mut self, epoch: Instant) { - self.scopes.retain(|_, v| v.stamp >= epoch); - } -} - #[cfg(test)] mod tests { use ctx::test_util::*; @@ -318,27 +284,27 @@ mod tests { mock_route(&mut root, &proxy, &server, "sixers"); let t2 = Instant::now(); - assert_eq!(root.requests.scopes.len(), 2); - assert_eq!(root.responses.scopes.len(), 2); - assert_eq!(root.transports.scopes.len(), 2); - assert_eq!(root.transport_closes.scopes.len(), 1); + assert_eq!(root.requests.len(), 2); + assert_eq!(root.responses.len(), 2); + assert_eq!(root.transports.len(), 2); + assert_eq!(root.transport_closes.len(), 1); root.retain_since(t0); - assert_eq!(root.requests.scopes.len(), 2); - assert_eq!(root.responses.scopes.len(), 2); - assert_eq!(root.transports.scopes.len(), 2); - assert_eq!(root.transport_closes.scopes.len(), 1); + assert_eq!(root.requests.len(), 2); + assert_eq!(root.responses.len(), 2); + assert_eq!(root.transports.len(), 2); + assert_eq!(root.transport_closes.len(), 1); root.retain_since(t1); - assert_eq!(root.requests.scopes.len(), 1); - assert_eq!(root.responses.scopes.len(), 1); - assert_eq!(root.transports.scopes.len(), 2); - assert_eq!(root.transport_closes.scopes.len(), 1); + assert_eq!(root.requests.len(), 1); + assert_eq!(root.responses.len(), 1); + assert_eq!(root.transports.len(), 2); + assert_eq!(root.transport_closes.len(), 1); root.retain_since(t2); - assert_eq!(root.requests.scopes.len(), 0); - assert_eq!(root.responses.scopes.len(), 0); - assert_eq!(root.transports.scopes.len(), 2); - assert_eq!(root.transport_closes.scopes.len(), 1); + assert_eq!(root.requests.len(), 0); + assert_eq!(root.responses.len(), 0); + assert_eq!(root.transports.len(), 2); + assert_eq!(root.transport_closes.len(), 1); } } diff --git a/src/telemetry/metrics/record.rs b/src/telemetry/metrics/record.rs index 3d324bc7f..9fdcad3c0 100644 --- a/src/telemetry/metrics/record.rs +++ b/src/telemetry/metrics/record.rs @@ -137,7 +137,7 @@ mod test { assert!(r.metrics.lock() .expect("lock") - .responses.scopes + .responses .get(&labels) .is_none() ); @@ -146,7 +146,8 @@ mod test { { let lock = r.metrics.lock() .expect("lock"); - let scope = lock.responses.scopes + let scope = lock + .responses .get(&labels) .expect("scope should be some after event"); @@ -249,12 +250,12 @@ mod test { { let lock = r.metrics.lock() .expect("lock"); - assert!(lock.requests.scopes.get(&req_labels).is_none()); - assert!(lock.responses.scopes.get(&rsp_labels).is_none()); - assert!(lock.transports.scopes.get(&srv_open_labels).is_none()); - assert!(lock.transports.scopes.get(&client_open_labels).is_none()); - assert!(lock.transport_closes.scopes.get(&srv_close_labels).is_none()); - assert!(lock.transport_closes.scopes.get(&client_close_labels).is_none()); + assert!(lock.requests.get(&req_labels).is_none()); + assert!(lock.responses.get(&rsp_labels).is_none()); + assert!(lock.transports.get(&srv_open_labels).is_none()); + assert!(lock.transports.get(&client_open_labels).is_none()); + assert!(lock.transport_closes.get(&srv_close_labels).is_none()); + assert!(lock.transport_closes.get(&client_close_labels).is_none()); } for e in &events { @@ -267,7 +268,7 @@ mod test { // === request scope ==================================== assert_eq!( - lock.requests.scopes + lock.requests .get(&req_labels) .map(|scope| scope.total()), Some(1) @@ -275,7 +276,7 @@ mod test { // === response scope =================================== let response_scope = lock - .responses.scopes + .responses .get(&rsp_labels) .expect("response scope missing"); assert_eq!(response_scope.total(), 1); @@ -287,7 +288,7 @@ mod test { // === server transport open scope ====================== let srv_transport_scope = lock - .transports.scopes + .transports .get(&srv_open_labels) .expect("server transport scope missing"); assert_eq!(srv_transport_scope.open_total(), 1); @@ -296,7 +297,7 @@ mod test { // === client transport open scope ====================== let client_transport_scope = lock - .transports.scopes + .transports .get(&client_open_labels) .expect("client transport scope missing"); assert_eq!(client_transport_scope.open_total(), 1); @@ -307,7 +308,7 @@ mod test { // === server transport close scope ===================== let srv_transport_close_scope = lock - .transport_closes.scopes + .transport_closes .get(&srv_close_labels) .expect("server transport close scope missing"); assert_eq!(srv_transport_close_scope.close_total(), 1); @@ -318,7 +319,7 @@ mod test { // === client transport close scope ===================== let client_transport_close_scope = lock - .transport_closes.scopes + .transport_closes .get(&client_close_labels) .expect("client transport close scope missing"); assert_eq!(client_transport_close_scope.close_total(), 1); diff --git a/src/telemetry/metrics/scopes.rs b/src/telemetry/metrics/scopes.rs new file mode 100644 index 000000000..b451ab7ae --- /dev/null +++ b/src/telemetry/metrics/scopes.rs @@ -0,0 +1,50 @@ +use indexmap::IndexMap; +use std::{fmt::Display, hash::Hash}; + +/// Holds an `S`-typed scope for each `L`-typed label set. +/// +/// An `S` type typically holds one or more metrics. +#[derive(Debug)] +pub struct Scopes(IndexMap); + +impl Default for Scopes { + fn default() -> Self { + Scopes(IndexMap::default()) + } +} + +impl Scopes { + pub fn get(&self, key: &L) -> Option<&S> { + self.0.get(key) + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + pub fn len(&self) -> usize { + self.0.len() + } + + pub fn retain(&mut self, f: F) + where + F: FnMut(&L, &mut S) -> bool, + { + self.0.retain(f) + } +} + +impl Scopes { + pub fn get_or_default(&mut self, key: L) -> &mut S { + self.0.entry(key).or_insert_with(|| S::default()) + } +} + +impl<'a, L: Display + Hash + Eq, S> IntoIterator for &'a Scopes { + type Item = <&'a IndexMap as IntoIterator>::Item; + type IntoIter = <&'a IndexMap as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.iter() + } +} diff --git a/src/telemetry/metrics/transport.rs b/src/telemetry/metrics/transport.rs index 7fc821d9a..b7895516b 100644 --- a/src/telemetry/metrics/transport.rs +++ b/src/telemetry/metrics/transport.rs @@ -42,7 +42,7 @@ impl OpenScopes { impl fmt::Display for OpenScopes { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.scopes.is_empty() { + if self.is_empty() { return Ok(()); } @@ -103,7 +103,7 @@ impl CloseScopes { impl fmt::Display for CloseScopes { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.scopes.is_empty() { + if self.is_empty() { return Ok(()); }