Completely switch to KeyRef at the API and re-add callsite caching

metrics-exporter-prometheus/src/lib.rs

@@ -6,7 +6,7 @@ use hyper::{
     service::{make_service_fn, service_fn},
     {Body, Error as HyperError, Response, Server},
 };
-use metrics::{Identifier, Key, Label, Recorder, SetRecorderError};
+use metrics::{KeyRef, Recorder, SetRecorderError};
 use metrics_util::{
     parse_quantiles, CompositeKey, Handle, Histogram, MetricKind, Quantile, Registry,
 };
@@ -335,7 +335,7 @@ pub struct PrometheusRecorder {
 }
 
 impl PrometheusRecorder {
-    fn add_description_if_missing(&self, key: &Key, description: Option<&'static str>) {
+    fn add_description_if_missing(&self, key: &KeyRef, description: Option<&'static str>) {
         if let Some(description) = description {
             let mut descriptions = self.inner.descriptions.write();
             if !descriptions.contains_key(key.name().as_ref()) {
@@ -494,65 +494,82 @@ impl PrometheusBuilder {
 }
 
 impl Recorder for PrometheusRecorder {
-    fn register_counter(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_counter(&self, key: KeyRef, description: Option<&'static str>) {
         self.add_description_if_missing(&key, description);
         self.inner
             .registry()
             .get_or_create_identifier(CompositeKey::new(MetricKind::Counter, key), |_| {
                 Handle::counter()
-            })
+            });
     }
 
-    fn register_gauge(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_gauge(&self, key: KeyRef, description: Option<&'static str>) {
         self.add_description_if_missing(&key, description);
         self.inner
             .registry()
             .get_or_create_identifier(CompositeKey::new(MetricKind::Gauge, key), |_| {
                 Handle::gauge()
-            })
+            });
     }
 
-    fn register_histogram(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_histogram(&self, key: KeyRef, description: Option<&'static str>) {
         self.add_description_if_missing(&key, description);
         self.inner
             .registry()
             .get_or_create_identifier(CompositeKey::new(MetricKind::Histogram, key), |_| {
                 Handle::histogram()
-            })
+            });
     }
 
-    fn increment_counter(&self, key: Key, value: u64) {
-        let id = self.register_counter(key, None);
+    fn increment_counter(&self, key: KeyRef, value: u64) {
+        let id = self
+            .inner
+            .registry()
+            .get_or_create_identifier(CompositeKey::new(MetricKind::Counter, key), |_| {
+                Handle::counter()
+            });
         self.inner
             .registry()
             .with_handle(id, |h| h.increment_counter(value));
     }
 
-    fn update_gauge(&self, key: Key, value: f64) {
-        let id = self.register_gauge(key, None);
+    fn update_gauge(&self, key: KeyRef, value: f64) {
+        let id = self
+            .inner
+            .registry()
+            .get_or_create_identifier(CompositeKey::new(MetricKind::Gauge, key), |_| {
+                Handle::gauge()
+            });
         self.inner
             .registry()
             .with_handle(id, |h| h.update_gauge(value));
     }
 
-    fn record_histogram(&self, key: Key, value: u64) {
-        let id = self.register_histogram(key, None);
+    fn record_histogram(&self, key: KeyRef, value: u64) {
+        let id = self
+            .inner
+            .registry()
+            .get_or_create_identifier(CompositeKey::new(MetricKind::Histogram, key), |_| {
+                Handle::histogram()
+            });
         self.inner
             .registry()
             .with_handle(id, |h| h.record_histogram(value));
     }
 }
 
-fn key_to_parts(key: Key) -> (String, Vec<String>) {
-    let (name, labels) = key.into_parts();
+fn key_to_parts(key: KeyRef) -> (String, Vec<String>) {
+    let name = key.name();
+    let labels = key.labels();
     let sanitize = |c| c == '.' || c == '=' || c == '{' || c == '}' || c == '+' || c == '-';
     let name = name.replace(sanitize, "_");
     let labels = labels
         .map(|labels| {
             labels
                 .into_iter()
-                .map(Label::into_parts)
-                .map(|(k, v)| {
+                .map(|label| {
+                    let k = label.key();
+                    let v = label.value();
                     format!(
                         "{}=\"{}\"",
                         k,

metrics-exporter-tcp/src/lib.rs

@@ -53,7 +53,7 @@ use std::time::SystemTime;
 
 use bytes::Bytes;
 use crossbeam_channel::{bounded, unbounded, Receiver, Sender};
-use metrics::{Identifier, Key, Recorder, SetRecorderError};
+use metrics::{Identifier, KeyRef, Recorder, SetRecorderError};
 use metrics_util::Registry;
 use mio::{
     net::{TcpListener, TcpStream},
@@ -87,10 +87,10 @@ enum MetricValue {
 }
 
 #[derive(Eq, PartialEq, Hash, Clone)]
-struct CompositeKey(MetricKind, Key);
+struct CompositeKey(MetricKind, KeyRef);
 
 impl CompositeKey {
-    pub fn key(&self) -> &Key {
+    pub fn key(&self) -> &KeyRef {
         &self.1
     }
 }
@@ -214,7 +214,7 @@ impl TcpBuilder {
 }
 
 impl TcpRecorder {
-    fn register_metric(&self, kind: MetricKind, key: Key) -> Identifier {
+    fn register_metric(&self, kind: MetricKind, key: KeyRef) -> Identifier {
         let ckey = CompositeKey(kind, key);
         self.registry.get_or_create_identifier(ckey, |k| k.clone())
     }
@@ -230,30 +230,30 @@ impl TcpRecorder {
 }
 
 impl Recorder for TcpRecorder {
-    fn register_counter(&self, key: Key, _description: Option<&'static str>) -> Identifier {
-        self.register_metric(MetricKind::Counter, key)
+    fn register_counter(&self, key: KeyRef, _description: Option<&'static str>) {
+        self.register_metric(MetricKind::Counter, key);
     }
 
-    fn register_gauge(&self, key: Key, _description: Option<&'static str>) -> Identifier {
-        self.register_metric(MetricKind::Gauge, key)
+    fn register_gauge(&self, key: KeyRef, _description: Option<&'static str>) {
+        self.register_metric(MetricKind::Gauge, key);
     }
 
-    fn register_histogram(&self, key: Key, _description: Option<&'static str>) -> Identifier {
-        self.register_metric(MetricKind::Histogram, key)
+    fn register_histogram(&self, key: KeyRef, _description: Option<&'static str>) {
+        self.register_metric(MetricKind::Histogram, key);
     }
 
-    fn increment_counter(&self, key: Key, value: u64) {
-        let id = self.register_counter(key, None);
+    fn increment_counter(&self, key: KeyRef, value: u64) {
+        let id = self.register_metric(MetricKind::Counter, key);
         self.push_metric(id, MetricValue::Counter(value));
     }
 
-    fn update_gauge(&self, key: Key, value: f64) {
-        let id = self.register_gauge(key, None);
+    fn update_gauge(&self, key: KeyRef, value: f64) {
+        let id = self.register_metric(MetricKind::Gauge, key);
         self.push_metric(id, MetricValue::Gauge(value));
     }
 
-    fn record_histogram(&self, key: Key, value: u64) {
-        let id = self.register_histogram(key, None);
+    fn record_histogram(&self, key: KeyRef, value: u64) {
+        let id = self.register_metric(MetricKind::Histogram, key);
         self.push_metric(id, MetricValue::Histogram(value));
     }
 }

metrics-macros/src/lib.rs

@@ -190,7 +190,9 @@ fn get_expanded_registration(
         {
             // Only do this work if there's a recorder installed.
             if let Some(recorder) = metrics::try_recorder() {
-                recorder.#register_ident(#key, #description);
+                // Registrations are fairly rare, don't attempt to cache here
+                // and just use and owned ref.
+                recorder.#register_ident(metrics::KeyRef::Owned(#key), #description);
             }
         }
     }
@@ -206,6 +208,7 @@ fn get_expanded_callsite<V>(
 where
     V: ToTokens,
 {
+    let use_fast_path = can_use_fast_path(&labels);
     let key = key_to_quoted(key, labels);
 
     let op_values = if metric_type == "histogram" {
@@ -217,16 +220,59 @@ where
     };
 
     let op_ident = format_ident!("{}_{}", op_type, metric_type);
-    quote! {
-        {
-            // Only do this work if there's a recorder installed.
-            if let Some(recorder) = metrics::try_recorder() {
-                recorder.#op_ident(#key, #op_values);
+
+    if use_fast_path {
+        // We're on the fast path here, so we'll end up registering with the recorder
+        // and statically caching the identifier for our metric to speed up any future
+        // increment operations.
+        quote! {
+            {
+                static CACHED_KEY: metrics::OnceKey = metrics::OnceKey::new();
+
+                // Only do this work if there's a recorder installed.
+                if let Some(recorder) = metrics::try_recorder() {
+                    // Initialize our fast path.
+                    let key = CACHED_KEY.get_or_init(|| { #key });
+                    recorder.#op_ident(metrics::KeyRef::Borrowed(&key), #op_values);
+                }
+            }
+        }
+    } else {
+        // We're on the slow path, so basically we register every single time.
+        //
+        // Recorders are expected to deduplicate any duplicate registrations.
+        quote! {
+            {
+                // Only do this work if there's a recorder installed.
+                if let Some(recorder) = metrics::try_recorder() {
+                    recorder.#op_ident(metrics::KeyRef::Owned(#key), #op_values);
+                }
             }
         }
     }
 }
 
+fn can_use_fast_path(labels: &Option<Labels>) -> bool {
+    match labels {
+        None => true,
+        Some(labels) => match labels {
+            Labels::Existing(_) => false,
+            Labels::Inline(pairs) => {
+                let mut use_fast_path = true;
+                for (_, lvalue) in pairs {
+                    match lvalue {
+                        Expr::Lit(_) => {}
+                        _ => {
+                            use_fast_path = false;
+                        }
+                    }
+                }
+                use_fast_path
+            }
+        },
+    }
+}
+
 fn read_key(input: &mut ParseStream) -> Result<Key> {
     if let Ok(_) = input.parse::<Token![<]>() {
         let s = input.parse::<LitStr>()?;

metrics-macros/src/tests.rs

@@ -29,7 +29,7 @@ fn test_get_expanded_registration() {
     let expected = concat!(
         "{ if let Some ( recorder ) = metrics :: try_recorder ( ) { ",
         "recorder . register_mytype ( ",
-        "metrics :: Key :: from_name ( \"mykeyname\" ) , ",
+        "metrics :: KeyRef :: Owned ( metrics :: Key :: from_name ( \"mykeyname\" ) ) , ",
         "None ",
         ") ; ",
         "} }",
@@ -38,9 +38,9 @@ fn test_get_expanded_registration() {
     assert_eq!(stream.to_string(), expected);
 }
 
-/// If there are no dynamic labels - we generate the static invocation.
+/// If there are no dynamic labels - generate an invocation with caching.
 #[test]
-fn test_get_expanded_callsite() {
+fn test_get_expanded_callsite_fast_path() {
     let stream = get_expanded_callsite(
         "mytype",
         "myop",
@@ -51,8 +51,36 @@ fn test_get_expanded_callsite() {
 
     let expected = concat!(
         "{ ",
+        "static CACHED_KEY : metrics :: OnceKey = metrics :: OnceKey :: new ( ) ; ",
         "if let Some ( recorder ) = metrics :: try_recorder ( ) { ",
-        "recorder . myop_mytype ( metrics :: Key :: from_name ( \"mykeyname\" ) , 1 ) ; ",
+        "let key = CACHED_KEY . get_or_init ( || { ",
+        "metrics :: Key :: from_name ( \"mykeyname\" ) ",
+        "} ) ; ",
+        "recorder . myop_mytype ( metrics :: KeyRef :: Borrowed ( & key ) , 1 ) ; ",
+        "} }",
+    );
+
+    assert_eq!(stream.to_string(), expected);
+}
+
+/// If there are dynamic labels - generate a direct invocation.
+#[test]
+fn test_get_expanded_callsite_regular_path() {
+    let stream = get_expanded_callsite(
+        "mytype",
+        "myop",
+        Key::NotScoped(parse_quote! {"mykeyname"}),
+        Some(Labels::Existing(parse_quote! { mylabels })),
+        quote! { 1 },
+    );
+
+    let expected = concat!(
+        "{ ",
+        "if let Some ( recorder ) = metrics :: try_recorder ( ) { ",
+        "recorder . myop_mytype ( ",
+        "metrics :: KeyRef :: Owned ( metrics :: Key :: from_name_and_labels ( \"mykeyname\" , mylabels ) ) , ",
+        "1 ",
+        ") ; ",
         "} }",
     );
 

metrics-tracing-context/src/lib.rs

@@ -1,4 +1,4 @@
-use metrics::{Identifier, Key, Label, Recorder};
+use metrics::{Key, KeyRef, Label, Recorder};
 use metrics_util::layers::Layer;
 use tracing::Span;
 
@@ -28,42 +28,43 @@ impl<R> TracingContext<R> {
         });
     }
 
-    fn enhance_key(&self, key: Key) -> Key {
-        let (name, labels) = key.into_parts();
+    fn enhance_key(&self, key: KeyRef) -> KeyRef {
+        let (name, labels) = key.into_owned().into_parts();
         let mut labels = labels.unwrap_or_default();
         self.enhance_labels(&mut labels);
         if labels.is_empty() {
             Key::from_name(name)
         } else {
             Key::from_name_and_labels(name, labels)
         }
+        .into()
     }
 }
 
 impl<R: Recorder> Recorder for TracingContext<R> {
-    fn register_counter(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_counter(&self, key: KeyRef, description: Option<&'static str>) {
         self.inner.register_counter(key, description)
     }
 
-    fn register_gauge(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_gauge(&self, key: KeyRef, description: Option<&'static str>) {
         self.inner.register_gauge(key, description)
     }
 
-    fn register_histogram(&self, key: Key, description: Option<&'static str>) -> Identifier {
+    fn register_histogram(&self, key: KeyRef, description: Option<&'static str>) {
         self.inner.register_histogram(key, description)
     }
 
-    fn increment_counter(&self, key: Key, value: u64) {
+    fn increment_counter(&self, key: KeyRef, value: u64) {
         let key = self.enhance_key(key);
         self.inner.increment_counter(key, value);
     }
 
-    fn update_gauge(&self, key: Key, value: f64) {
+    fn update_gauge(&self, key: KeyRef, value: f64) {
         let key = self.enhance_key(key);
         self.inner.update_gauge(key, value);
     }
 
-    fn record_histogram(&self, key: Key, value: u64) {
+    fn record_histogram(&self, key: KeyRef, value: u64) {
         let key = self.enhance_key(key);
         self.inner.record_histogram(key, value);
     }