From 5a61549dd52e874a12bb0ed74c4f716301699171 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 09:09:47 -0500 Subject: [PATCH 1/6] owned strings as key is working, needs a lot more tests --- metrics-macros/src/lib.rs | 229 +++++++++++------- metrics-macros/src/tests.rs | 94 +++---- metrics/src/key.rs | 11 + metrics/tests/macros.rs | 4 +- metrics/tests/macros/01_basic.rs | 12 + metrics/tests/macros/02_metric_name.rs | 11 - metrics/tests/macros/02_metric_name.stderr | 5 - ...trailing_comma.rs => 02_trailing_comma.rs} | 0 8 files changed, 202 insertions(+), 164 deletions(-) create mode 100644 metrics/tests/macros/01_basic.rs delete mode 100644 metrics/tests/macros/02_metric_name.rs delete mode 100644 metrics/tests/macros/02_metric_name.stderr rename metrics/tests/macros/{01_trailing_comma.rs => 02_trailing_comma.rs} (100%) diff --git a/metrics-macros/src/lib.rs b/metrics-macros/src/lib.rs index 91bfc1dd..d3637e07 100644 --- a/metrics-macros/src/lib.rs +++ b/metrics-macros/src/lib.rs @@ -2,10 +2,9 @@ extern crate proc_macro; use self::proc_macro::TokenStream; -use lazy_static::lazy_static; +use proc_macro2::TokenStream as TokenStream2; use quote::{format_ident, quote, ToTokens}; -use regex::Regex; -use syn::parse::discouraged::Speculative; +use syn::{Lit, parse::discouraged::Speculative}; use syn::parse::{Error, Parse, ParseStream, Result}; use syn::{parse_macro_input, Expr, LitStr, Token}; @@ -18,18 +17,18 @@ enum Labels { } struct WithoutExpression { - key: LitStr, + key: Expr, labels: Option, } struct WithExpression { - key: LitStr, + key: Expr, op_value: Expr, labels: Option, } struct Registration { - key: LitStr, + key: Expr, unit: Option, description: Option, labels: Option, @@ -37,7 +36,7 @@ struct Registration { impl Parse for WithoutExpression { fn parse(mut input: ParseStream) -> Result { - let key = read_key(&mut input)?; + let key = input.parse::()?; let labels = parse_labels(&mut input)?; Ok(WithoutExpression { key, labels }) @@ -46,7 +45,7 @@ impl Parse for WithoutExpression { impl Parse for WithExpression { fn parse(mut input: ParseStream) -> Result { - let key = read_key(&mut input)?; + let key = input.parse::()?; input.parse::()?; let op_value: Expr = input.parse()?; @@ -63,7 +62,7 @@ impl Parse for WithExpression { impl Parse for Registration { fn parse(mut input: ParseStream) -> Result { - let key = read_key(&mut input)?; + let key = input.parse::()?; // We accept three possible parameters: unit, description, and labels. // @@ -251,13 +250,12 @@ pub fn histogram(input: TokenStream) -> TokenStream { fn get_expanded_registration( metric_type: &str, - name: LitStr, + name: Expr, unit: Option, description: Option, labels: Option, -) -> proc_macro2::TokenStream { +) -> TokenStream2 { let register_ident = format_ident!("register_{}", metric_type); - let key = key_to_quoted(labels); let unit = match unit { Some(e) => quote! { Some(#e) }, @@ -269,14 +267,14 @@ fn get_expanded_registration( None => quote! { None }, }; + let statics = generate_statics(&name, &labels); + let metric_key = generate_metric_key(&name, &labels); quote! { { - static METRIC_NAME: [metrics::SharedString; 1] = [metrics::SharedString::const_str(#name)]; + #statics // Only do this work if there's a recorder installed. if let Some(recorder) = metrics::try_recorder() { - // Registrations are fairly rare, don't attempt to cache here - // and just use an owned ref. - recorder.#register_ident(metrics::Key::Owned(#key), #unit, #description); + recorder.#register_ident(#metric_key, #unit, #description); } } } @@ -285,10 +283,10 @@ fn get_expanded_registration( fn get_expanded_callsite( metric_type: &str, op_type: &str, - name: LitStr, + name: Expr, labels: Option, op_values: V, -) -> proc_macro2::TokenStream +) -> TokenStream2 where V: ToTokens, { @@ -301,104 +299,153 @@ where }; let op_ident = format_ident!("{}_{}", op_type, metric_type); + let statics = generate_statics(&name, &labels); + let metric_key = generate_metric_key(&name, &labels); + quote! { + { + #statics + // Only do this work if there's a recorder installed. + if let Some(recorder) = metrics::try_recorder() { + recorder.#op_ident(#metric_key, #op_values); + } + } + } +} - let use_fast_path = can_use_fast_path(&labels); - if use_fast_path { - // We're on the fast path here, so we'll build our key, statically cache it, - // and use a borrowed reference to it for this and future operations. - let statics = match labels { - Some(Labels::Inline(pairs)) => { +fn name_is_fast_path(name: &Expr) -> bool { + match name { + Expr::Lit(lit) => match lit.lit { + Lit::Str(_) => true, + _ => false, + }, + _ => false, + } +} + +fn labels_are_fast_path(labels: &Labels) -> bool { + match labels { + Labels::Existing(_) => false, + Labels::Inline(pairs) => pairs.iter().all(|(_, v)| matches!(v, Expr::Lit(_))), + } +} + +fn generate_statics(name: &Expr, labels: &Option) -> TokenStream2 { + // Create the static for the name, if possible. + let use_name_static = name_is_fast_path(name); + let name_static = if use_name_static { + quote! { + static METRIC_NAME: [metrics::SharedString; 1] = [metrics::SharedString::const_str(#name)]; + } + } else { + quote! {} + }; + + // Create the static for the labels, if possible. + let has_labels = labels.is_some(); + let use_labels_static = match labels.as_ref() { + Some(labels) => labels_are_fast_path(labels), + None => true, + }; + + let labels_static = match labels.as_ref() { + Some(labels) => if labels_are_fast_path(labels) { + if let Labels::Inline(pairs) = labels { let labels = pairs - .into_iter() + .iter() .map(|(key, val)| quote! { metrics::Label::from_static_parts(#key, #val) }) .collect::>(); let labels_len = labels.len(); let labels_len = quote! { #labels_len }; quote! { - static METRIC_NAME: [metrics::SharedString; 1] = [metrics::SharedString::const_str(#name)]; static METRIC_LABELS: [metrics::Label; #labels_len] = [#(#labels),*]; - static METRIC_KEY: metrics::KeyData = - metrics::KeyData::from_static_parts(&METRIC_NAME, &METRIC_LABELS); - } - } - None => { - quote! { - static METRIC_NAME: [metrics::SharedString; 1] = [metrics::SharedString::const_str(#name)]; - static METRIC_KEY: metrics::KeyData = - metrics::KeyData::from_static_name(&METRIC_NAME); } + } else { + quote! {} } - _ => unreachable!("use_fast_path == true, but found expression-based labels"), - }; - - quote! { - { - #statics + } else { + quote! {} + }, + None => quote! {}, + }; - // Only do this work if there's a recorder installed. - if let Some(recorder) = metrics::try_recorder() { - recorder.#op_ident(metrics::Key::Borrowed(&METRIC_KEY), #op_values); - } + let key_static = if use_name_static && use_labels_static { + if has_labels { + quote! { + static METRIC_KEY: metrics::KeyData = metrics::KeyData::from_static_parts(&METRIC_NAME, &METRIC_LABELS); + } + } else { + quote!{ + static METRIC_KEY: metrics::KeyData = metrics::KeyData::from_static_name(&METRIC_NAME); } } } else { - // We're on the slow path, so we allocate, womp. - let key = key_to_quoted(labels); - quote! { - { - static METRIC_NAME: [metrics::SharedString; 1] = [metrics::SharedString::const_str(#name)]; + quote! {} + }; - // Only do this work if there's a recorder installed. - if let Some(recorder) = metrics::try_recorder() { - recorder.#op_ident(metrics::Key::Owned(#key), #op_values); - } - } - } + quote! { + #name_static + #labels_static + #key_static } } -fn can_use_fast_path(labels: &Option) -> bool { - match labels { - None => true, - Some(labels) => match labels { - Labels::Existing(_) => false, - Labels::Inline(pairs) => pairs.iter().all(|(_, v)| matches!(v, Expr::Lit(_))), - }, - } -} +fn generate_metric_key(name: &Expr, labels: &Option) -> TokenStream2 { + let use_name_static = name_is_fast_path(name); -fn read_key(input: &mut ParseStream) -> Result { - let key = input.parse::()?; - let inner = key.value(); + let has_labels = labels.is_some(); + let use_labels_static = match labels.as_ref() { + Some(labels) => labels_are_fast_path(labels), + None => true, + }; - lazy_static! { - static ref RE: Regex = Regex::new("^[a-zA-Z][a-zA-Z0-9_:\\.]*$").unwrap(); - } - if !RE.is_match(&inner) { - return Err(Error::new( - key.span(), - "metric name must match ^[a-zA-Z][a-zA-Z0-9_:.]*$", - )); + if use_name_static && use_labels_static { + // Key is entirely static, so we can simply reference our generated statics. They will be + // inclusive of whether or not labels were specified. + quote! { metrics::Key::Borrowed(&METRIC_KEY) } + } else if use_name_static && !use_labels_static { + // The name is static, but we have labels which are not static. Since `use_labels_static` + // cannot be false unless labels _are_ specified, we know this unwrap is safe. + let labels = labels.as_ref().unwrap(); + let quoted_labels = labels_to_quoted(labels); + quote! { + metrics::Key::Owned(metrics::KeyData::from_hybrid(&METRIC_NAME, #quoted_labels)) + } + } else if !use_name_static && !use_labels_static { + // The name is not static, and neither are the labels. Since `use_labels_static` + // cannot be false unless labels _are_ specified, we know this unwrap is safe. + let labels = labels.as_ref().unwrap(); + let quoted_labels = labels_to_quoted(labels); + quote! { + metrics::Key::Owned(metrics::KeyData::from_parts(#name, #quoted_labels)) + } + } else { + // The name is not static, but the labels are. This could technically mean that there + // simply are no labels, so we have to discriminate in a slightly different way + // to figure out the correct key. + if has_labels { + let labels = labels.as_ref().unwrap(); + let quoted_labels = labels_to_quoted(labels); + quote! { + metrics::Key::Owned(metrics::KeyData::from_parts(#name, #quoted_labels)) + } + } else { + quote! { + metrics::Key::Owned(metrics::KeyData::from_name(#name)) + } + } } - - Ok(key) } -fn key_to_quoted(labels: Option) -> proc_macro2::TokenStream { +fn labels_to_quoted(labels: &Labels) -> proc_macro2::TokenStream { match labels { - None => quote! { metrics::KeyData::from_static_name(&METRIC_NAME) }, - Some(labels) => match labels { - Labels::Inline(pairs) => { - let labels = pairs - .into_iter() - .map(|(key, val)| quote! { metrics::Label::new(#key, #val) }); - quote! { - metrics::KeyData::from_parts(&METRIC_NAME[..], vec![#(#labels),*]) - } - } - Labels::Existing(e) => quote! { metrics::KeyData::from_parts(&METRIC_NAME[..], #e) }, - }, + Labels::Inline(pairs) => { + let labels = pairs + .into_iter() + .map(|(key, val)| quote! { metrics::Label::new(#key, #val) }); + quote! { vec![#(#labels),*] } + } + Labels::Existing(e) => quote! { #e }, } } diff --git a/metrics-macros/src/tests.rs b/metrics-macros/src/tests.rs index d769eb8c..f7225597 100644 --- a/metrics-macros/src/tests.rs +++ b/metrics-macros/src/tests.rs @@ -12,13 +12,11 @@ fn test_get_expanded_registration() { let expected = concat!( "{ ", "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", + "static METRIC_KEY : metrics :: KeyData = metrics :: KeyData :: from_static_name (& METRIC_NAME) ; ", "if let Some (recorder) = metrics :: try_recorder () { ", - "recorder . register_mytype (", - "metrics :: Key :: Owned (metrics :: KeyData :: from_static_name (& METRIC_NAME)) , ", - "None , ", - "None", - ") ; ", - "} }", + "recorder . register_mytype (metrics :: Key :: Borrowed (& METRIC_KEY) , None , None) ; ", + "} ", + "}", ); assert_eq!(stream.to_string(), expected); @@ -39,13 +37,11 @@ fn test_get_expanded_registration_with_unit() { let expected = concat!( "{ ", "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", + "static METRIC_KEY : metrics :: KeyData = metrics :: KeyData :: from_static_name (& METRIC_NAME) ; ", "if let Some (recorder) = metrics :: try_recorder () { ", - "recorder . register_mytype (", - "metrics :: Key :: Owned (metrics :: KeyData :: from_static_name (& METRIC_NAME)) , ", - "Some (metrics :: Unit :: Nanoseconds) , ", - "None", - ") ; ", - "} }", + "recorder . register_mytype (metrics :: Key :: Borrowed (& METRIC_KEY) , Some (metrics :: Unit :: Nanoseconds) , None) ; ", + "} ", + "}", ); assert_eq!(stream.to_string(), expected); @@ -65,13 +61,11 @@ fn test_get_expanded_registration_with_description() { let expected = concat!( "{ ", "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", + "static METRIC_KEY : metrics :: KeyData = metrics :: KeyData :: from_static_name (& METRIC_NAME) ; ", "if let Some (recorder) = metrics :: try_recorder () { ", - "recorder . register_mytype (", - "metrics :: Key :: Owned (metrics :: KeyData :: from_static_name (& METRIC_NAME)) , ", - "None , ", - "Some (\"flerkin\")", - ") ; ", - "} }", + "recorder . register_mytype (metrics :: Key :: Borrowed (& METRIC_KEY) , None , Some (\"flerkin\")) ; ", + "} ", + "}", ); assert_eq!(stream.to_string(), expected); @@ -92,20 +86,18 @@ fn test_get_expanded_registration_with_unit_and_description() { let expected = concat!( "{ ", "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", + "static METRIC_KEY : metrics :: KeyData = metrics :: KeyData :: from_static_name (& METRIC_NAME) ; ", "if let Some (recorder) = metrics :: try_recorder () { ", - "recorder . register_mytype (", - "metrics :: Key :: Owned (metrics :: KeyData :: from_static_name (& METRIC_NAME)) , ", - "Some (metrics :: Unit :: Nanoseconds) , ", - "Some (\"flerkin\")", - ") ; ", - "} }", + "recorder . register_mytype (metrics :: Key :: Borrowed (& METRIC_KEY) , Some (metrics :: Unit :: Nanoseconds) , Some (\"flerkin\")) ; ", + "} ", + "}", ); assert_eq!(stream.to_string(), expected); } #[test] -fn test_get_expanded_callsite_fast_path_no_labels() { +fn test_get_expanded_callsite_static_name_no_labels() { let stream = get_expanded_callsite( "mytype", "myop", @@ -127,7 +119,7 @@ fn test_get_expanded_callsite_fast_path_no_labels() { } #[test] -fn test_get_expanded_callsite_fast_path_static_labels() { +fn test_get_expanded_callsite_static_name_static_inline_labels() { let labels = Labels::Inline(vec![(parse_quote! { "key1" }, parse_quote! { "value1" })]); let stream = get_expanded_callsite( "mytype", @@ -152,7 +144,7 @@ fn test_get_expanded_callsite_fast_path_static_labels() { } #[test] -fn test_get_expanded_callsite_fast_path_dynamic_labels() { +fn test_get_expanded_callsite_static_name_dynamic_inline_labels() { let labels = Labels::Inline(vec![(parse_quote! { "key1" }, parse_quote! { &value1 })]); let stream = get_expanded_callsite( "mytype", @@ -167,7 +159,7 @@ fn test_get_expanded_callsite_fast_path_dynamic_labels() { "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", "if let Some (recorder) = metrics :: try_recorder () { ", "recorder . myop_mytype (metrics :: Key :: Owned (", - "metrics :: KeyData :: from_parts (& METRIC_NAME [..] , vec ! [metrics :: Label :: new (\"key1\" , & value1)])", + "metrics :: KeyData :: from_hybrid (& METRIC_NAME , vec ! [metrics :: Label :: new (\"key1\" , & value1)])", ") , 1) ; ", "} ", "}", @@ -178,7 +170,7 @@ fn test_get_expanded_callsite_fast_path_dynamic_labels() { /// If there are dynamic labels - generate a direct invocation. #[test] -fn test_get_expanded_callsite_regular_path() { +fn test_get_expanded_callsite_static_name_existing_labels() { let stream = get_expanded_callsite( "mytype", "myop", @@ -191,52 +183,44 @@ fn test_get_expanded_callsite_regular_path() { "{ ", "static METRIC_NAME : [metrics :: SharedString ; 1] = [metrics :: SharedString :: const_str (\"mykeyname\")] ; ", "if let Some (recorder) = metrics :: try_recorder () { ", - "recorder . myop_mytype (", - "metrics :: Key :: Owned (metrics :: KeyData :: from_parts (& METRIC_NAME [..] , mylabels)) , ", - "1", - ") ; ", - "} }", + "recorder . myop_mytype (metrics :: Key :: Owned (metrics :: KeyData :: from_hybrid (& METRIC_NAME , mylabels)) , 1) ; ", + "} ", + "}", ); assert_eq!(stream.to_string(), expected); } #[test] -fn test_key_to_quoted_no_labels() { - let stream = key_to_quoted(None); - let expected = "metrics :: KeyData :: from_static_name (& METRIC_NAME)"; - assert_eq!(stream.to_string(), expected); -} - -#[test] -fn test_key_to_quoted_existing_labels() { - let stream = key_to_quoted(Some(Labels::Existing(Expr::Path( +fn test_labels_to_quoted_existing_labels() { + let labels = Labels::Existing(Expr::Path( parse_quote! { mylabels }, - )))); - let expected = "metrics :: KeyData :: from_parts (& METRIC_NAME [..] , mylabels)"; + )); + let stream = labels_to_quoted(&labels); + let expected = "mylabels"; assert_eq!(stream.to_string(), expected); } -/// Registration can only operate on static labels (i.e. labels baked into the -/// Key). #[test] -fn test_key_to_quoted_inline_labels() { - let stream = key_to_quoted(Some(Labels::Inline(vec![ +fn test_labels_to_quoted_inline_labels() { + let labels = Labels::Inline(vec![ (parse_quote! {"mylabel1"}, parse_quote! { mylabel1 }), (parse_quote! {"mylabel2"}, parse_quote! { "mylabel2" }), - ]))); + ]); + let stream = labels_to_quoted(&labels); let expected = concat!( - "metrics :: KeyData :: from_parts (& METRIC_NAME [..] , vec ! [", + "vec ! [", "metrics :: Label :: new (\"mylabel1\" , mylabel1) , ", "metrics :: Label :: new (\"mylabel2\" , \"mylabel2\")", - "])" + "]" ); assert_eq!(stream.to_string(), expected); } #[test] -fn test_key_to_quoted_inline_labels_empty() { - let stream = key_to_quoted(Some(Labels::Inline(vec![]))); - let expected = concat!("metrics :: KeyData :: from_parts (& METRIC_NAME [..] , vec ! [])"); +fn test_labels_to_quoted_inline_labels_empty() { + let labels = Labels::Inline(vec![]); + let stream = labels_to_quoted(&labels); + let expected = "vec ! []"; assert_eq!(stream.to_string(), expected); } diff --git a/metrics/src/key.rs b/metrics/src/key.rs index ac21d24e..2dd60c32 100644 --- a/metrics/src/key.rs +++ b/metrics/src/key.rs @@ -113,6 +113,17 @@ impl KeyData { } } + /// Creates a [`KeyData`] from a static name and non-static set of labels. + pub fn from_hybrid(name_parts: &'static [SharedString], labels: L) -> Self + where + L: IntoLabels, + { + Self { + name_parts: NameParts::from_static_names(name_parts), + labels: Cow::owned(labels.into_labels()), + } + } + /// Creates a [`KeyData`] from a static name. /// /// This function is `const`, so it can be used in a static context. diff --git a/metrics/tests/macros.rs b/metrics/tests/macros.rs index 222fdc57..b835b216 100644 --- a/metrics/tests/macros.rs +++ b/metrics/tests/macros.rs @@ -1,6 +1,6 @@ #[test] pub fn macros() { let t = trybuild::TestCases::new(); - t.pass("tests/macros/01_trailing_comma.rs"); - t.compile_fail("tests/macros/02_metric_name.rs"); + t.pass("tests/macros/01_basic.rs"); + t.pass("tests/macros/02_trailing_comma.rs"); } diff --git a/metrics/tests/macros/01_basic.rs b/metrics/tests/macros/01_basic.rs new file mode 100644 index 00000000..a215ad61 --- /dev/null +++ b/metrics/tests/macros/01_basic.rs @@ -0,0 +1,12 @@ +use metrics::counter; + +fn static_key() { + counter!("abcdef", 1); +} + +fn dynamic_key() { + let some_u16 = 0u16; + counter!(format!("response_status_{}", some_u16), 1); +} + +fn main() {} diff --git a/metrics/tests/macros/02_metric_name.rs b/metrics/tests/macros/02_metric_name.rs deleted file mode 100644 index b248f79a..00000000 --- a/metrics/tests/macros/02_metric_name.rs +++ /dev/null @@ -1,11 +0,0 @@ -use metrics::counter; - -fn valid_name() { - counter!("abc_def", 1); -} - -fn invalid_name() { - counter!("abc$def"); -} - -fn main() {} diff --git a/metrics/tests/macros/02_metric_name.stderr b/metrics/tests/macros/02_metric_name.stderr deleted file mode 100644 index b3e6ff53..00000000 --- a/metrics/tests/macros/02_metric_name.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: metric name must match ^[a-zA-Z][a-zA-Z0-9_:.]*$ - --> $DIR/02_metric_name.rs:8:14 - | -8 | counter!("abc$def"); - | ^^^^^^^^^ diff --git a/metrics/tests/macros/01_trailing_comma.rs b/metrics/tests/macros/02_trailing_comma.rs similarity index 100% rename from metrics/tests/macros/01_trailing_comma.rs rename to metrics/tests/macros/02_trailing_comma.rs From e6951a49b7fa1483bd6362acf1fb08a9b8ca58f2 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 10:05:38 -0500 Subject: [PATCH 2/6] fmt --- metrics-macros/src/lib.rs | 34 ++++++++++++++++++---------------- metrics-macros/src/tests.rs | 4 +--- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/metrics-macros/src/lib.rs b/metrics-macros/src/lib.rs index d3637e07..d5dda367 100644 --- a/metrics-macros/src/lib.rs +++ b/metrics-macros/src/lib.rs @@ -4,8 +4,8 @@ use self::proc_macro::TokenStream; use proc_macro2::TokenStream as TokenStream2; use quote::{format_ident, quote, ToTokens}; -use syn::{Lit, parse::discouraged::Speculative}; use syn::parse::{Error, Parse, ParseStream, Result}; +use syn::{parse::discouraged::Speculative, Lit}; use syn::{parse_macro_input, Expr, LitStr, Token}; #[cfg(test)] @@ -348,24 +348,26 @@ fn generate_statics(name: &Expr, labels: &Option) -> TokenStream2 { }; let labels_static = match labels.as_ref() { - Some(labels) => if labels_are_fast_path(labels) { - if let Labels::Inline(pairs) = labels { - let labels = pairs - .iter() - .map(|(key, val)| quote! { metrics::Label::from_static_parts(#key, #val) }) - .collect::>(); - let labels_len = labels.len(); - let labels_len = quote! { #labels_len }; - - quote! { - static METRIC_LABELS: [metrics::Label; #labels_len] = [#(#labels),*]; + Some(labels) => { + if labels_are_fast_path(labels) { + if let Labels::Inline(pairs) = labels { + let labels = pairs + .iter() + .map(|(key, val)| quote! { metrics::Label::from_static_parts(#key, #val) }) + .collect::>(); + let labels_len = labels.len(); + let labels_len = quote! { #labels_len }; + + quote! { + static METRIC_LABELS: [metrics::Label; #labels_len] = [#(#labels),*]; + } + } else { + quote! {} } } else { quote! {} } - } else { - quote! {} - }, + } None => quote! {}, }; @@ -375,7 +377,7 @@ fn generate_statics(name: &Expr, labels: &Option) -> TokenStream2 { static METRIC_KEY: metrics::KeyData = metrics::KeyData::from_static_parts(&METRIC_NAME, &METRIC_LABELS); } } else { - quote!{ + quote! { static METRIC_KEY: metrics::KeyData = metrics::KeyData::from_static_name(&METRIC_NAME); } } diff --git a/metrics-macros/src/tests.rs b/metrics-macros/src/tests.rs index f7225597..f4838f51 100644 --- a/metrics-macros/src/tests.rs +++ b/metrics-macros/src/tests.rs @@ -193,9 +193,7 @@ fn test_get_expanded_callsite_static_name_existing_labels() { #[test] fn test_labels_to_quoted_existing_labels() { - let labels = Labels::Existing(Expr::Path( - parse_quote! { mylabels }, - )); + let labels = Labels::Existing(Expr::Path(parse_quote! { mylabels })); let stream = labels_to_quoted(&labels); let expected = "mylabels"; assert_eq!(stream.to_string(), expected); From 545c45475cc2a2eac57d780c37218aef595a37ba Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 18:52:18 -0500 Subject: [PATCH 3/6] use static labels when available --- metrics-macros/src/lib.rs | 4 +- metrics-macros/src/tests.rs | 90 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/metrics-macros/src/lib.rs b/metrics-macros/src/lib.rs index d5dda367..c37f80fc 100644 --- a/metrics-macros/src/lib.rs +++ b/metrics-macros/src/lib.rs @@ -426,10 +426,8 @@ fn generate_metric_key(name: &Expr, labels: &Option) -> TokenStream2 { // simply are no labels, so we have to discriminate in a slightly different way // to figure out the correct key. if has_labels { - let labels = labels.as_ref().unwrap(); - let quoted_labels = labels_to_quoted(labels); quote! { - metrics::Key::Owned(metrics::KeyData::from_parts(#name, #quoted_labels)) + metrics::Key::Owned(metrics::KeyData::from_parts(#name, & METRICS_LABELS)) } } else { quote! { diff --git a/metrics-macros/src/tests.rs b/metrics-macros/src/tests.rs index f4838f51..86a6f793 100644 --- a/metrics-macros/src/tests.rs +++ b/metrics-macros/src/tests.rs @@ -191,6 +191,96 @@ fn test_get_expanded_callsite_static_name_existing_labels() { assert_eq!(stream.to_string(), expected); } +#[test] +fn test_get_expanded_callsite_owned_name_no_labels() { + let stream = get_expanded_callsite( + "mytype", + "myop", + parse_quote! { String::from("owned") }, + None, + quote! { 1 }, + ); + + let expected = concat!( + "{ ", + "if let Some (recorder) = metrics :: try_recorder () { ", + "recorder . myop_mytype (metrics :: Key :: Owned (metrics :: KeyData :: from_name (String :: from (\"owned\"))) , 1) ; ", + "} ", + "}", + ); + + assert_eq!(stream.to_string(), expected); +} + +#[test] +fn test_get_expanded_callsite_owned_name_static_inline_labels() { + let labels = Labels::Inline(vec![(parse_quote! { "key1" }, parse_quote! { "value1" })]); + let stream = get_expanded_callsite( + "mytype", + "myop", + parse_quote! { String::from("owned") }, + Some(labels), + quote! { 1 }, + ); + + let expected = concat!( + "{ ", + "static METRIC_LABELS : [metrics :: Label ; 1usize] = [metrics :: Label :: from_static_parts (\"key1\" , \"value1\")] ; ", + "if let Some (recorder) = metrics :: try_recorder () { ", + "recorder . myop_mytype (metrics :: Key :: Owned (metrics :: KeyData :: from_parts (String :: from (\"owned\") , & METRICS_LABELS)) , 1) ; ", + "} ", + "}", + ); + + assert_eq!(stream.to_string(), expected); +} + +#[test] +fn test_get_expanded_callsite_owned_name_dynamic_inline_labels() { + let labels = Labels::Inline(vec![(parse_quote! { "key1" }, parse_quote! { &value1 })]); + let stream = get_expanded_callsite( + "mytype", + "myop", + parse_quote! { String::from("owned") }, + Some(labels), + quote! { 1 }, + ); + + let expected = concat!( + "{ ", + "if let Some (recorder) = metrics :: try_recorder () { ", + "recorder . myop_mytype (metrics :: Key :: Owned (", + "metrics :: KeyData :: from_parts (String :: from (\"owned\") , vec ! [metrics :: Label :: new (\"key1\" , & value1)])", + ") , 1) ; ", + "} ", + "}", + ); + + assert_eq!(stream.to_string(), expected); +} + +/// If there are dynamic labels - generate a direct invocation. +#[test] +fn test_get_expanded_callsite_owned_name_existing_labels() { + let stream = get_expanded_callsite( + "mytype", + "myop", + parse_quote! { String::from("owned") }, + Some(Labels::Existing(parse_quote! { mylabels })), + quote! { 1 }, + ); + + let expected = concat!( + "{ ", + "if let Some (recorder) = metrics :: try_recorder () { ", + "recorder . myop_mytype (metrics :: Key :: Owned (metrics :: KeyData :: from_parts (String :: from (\"owned\") , mylabels)) , 1) ; ", + "} ", + "}", + ); + + assert_eq!(stream.to_string(), expected); +} + #[test] fn test_labels_to_quoted_existing_labels() { let labels = Labels::Existing(Expr::Path(parse_quote! { mylabels })); From 146ad6fed0abd8b37c0ca32a3b7dbee4d042f247 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 18:58:48 -0500 Subject: [PATCH 4/6] disable default features for hdrhistogram to fix breakage --- metrics-benchmark/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics-benchmark/Cargo.toml b/metrics-benchmark/Cargo.toml index bc3ddb52..620b9a06 100644 --- a/metrics-benchmark/Cargo.toml +++ b/metrics-benchmark/Cargo.toml @@ -9,7 +9,7 @@ publish = false log = "0.4" env_logger = "0.8" getopts = "0.2" -hdrhistogram = "7.2" +hdrhistogram = { version = "7.2", default-features = false } quanta = "0.7" atomic-shim = "0.1" metrics = { version = "0.13", path = "../metrics" } From b3f68c98cafdbd9c1f56ed86dabb4e7634a0e4bb Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 19:16:46 -0500 Subject: [PATCH 5/6] and here --- metrics-util/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics-util/Cargo.toml b/metrics-util/Cargo.toml index 427404bd..9e3f9d39 100644 --- a/metrics-util/Cargo.toml +++ b/metrics-util/Cargo.toml @@ -54,7 +54,7 @@ lazy_static = "1.3" rand = { version = "0.8", features = ["small_rng"] } rand_distr = "0.4" getopts = "0.2" -hdrhistogram = "7.2" +hdrhistogram = { version = "7.2", default-features = false } sketches-ddsketch = "0.1" ndarray = "0.14" ndarray-stats = "0.4" From d5e1c43e51c85206fbe47ff578a60850deb83121 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 2 Feb 2021 21:42:43 -0500 Subject: [PATCH 6/6] clippy fixes + doc updates --- metrics-macros/src/lib.rs | 12 +++++------ metrics/src/lib.rs | 43 +++++++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/metrics-macros/src/lib.rs b/metrics-macros/src/lib.rs index c37f80fc..02ece1ad 100644 --- a/metrics-macros/src/lib.rs +++ b/metrics-macros/src/lib.rs @@ -313,13 +313,11 @@ where } fn name_is_fast_path(name: &Expr) -> bool { - match name { - Expr::Lit(lit) => match lit.lit { - Lit::Str(_) => true, - _ => false, - }, - _ => false, + if let Expr::Lit(lit) = name { + return matches!(lit.lit, Lit::Str(_)); } + + false } fn labels_are_fast_path(labels: &Labels) -> bool { @@ -441,7 +439,7 @@ fn labels_to_quoted(labels: &Labels) -> proc_macro2::TokenStream { match labels { Labels::Inline(pairs) => { let labels = pairs - .into_iter() + .iter() .map(|(key, val)| quote! { metrics::Label::new(#key, #val) }); quote! { vec![#(#labels),*] } } diff --git a/metrics/src/lib.rs b/metrics/src/lib.rs index d7ea2534..794cca95 100644 --- a/metrics/src/lib.rs +++ b/metrics/src/lib.rs @@ -196,14 +196,13 @@ //! identifier, which we handle by using [`Key`]. //! //! [`Key`] itself is a wrapper for [`KeyData`], which holds not only the name of a metric, but -//! potentially holds labels for it as well. The name of a metric must always be a literal string. -//! The labels are a key/value pair, where both components are strings as well. +//! potentially holds labels for it as well. Metric name and labels consist entirely of string +//! values. //! //! Internally, `metrics` uses a clone-on-write "smart pointer" for these values to optimize cases //! where the values are static strings, which can provide significant performance benefits. These //! smart pointers can also hold owned `String` values, though, so users can mix and match static -//! strings and owned strings for labels without issue. Metric names, as mentioned above, are always -//! static strings. +//! strings and owned strings without issue. //! //! Two [`Key`] objects can be checked for equality and considered to point to the same metric if //! they are equal. Equality checks both the name of the key and the labels of a key. Labels are @@ -307,6 +306,10 @@ pub use self::recorder::*; /// recorder does anything with the description is implementation defined. Labels can also be /// specified when registering a metric. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::register_counter; @@ -350,6 +353,10 @@ pub use metrics_macros::register_counter; /// recorder does anything with the description is implementation defined. Labels can also be /// specified when registering a metric. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::register_gauge; @@ -393,6 +400,10 @@ pub use metrics_macros::register_gauge; /// recorder does anything with the description is implementation defined. Labels can also be /// specified when registering a metric. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::register_histogram; @@ -432,6 +443,10 @@ pub use metrics_macros::register_histogram; /// Counters represent a single monotonic value, which means the value can only be incremented, not /// decremented, and always starts out with an initial value of zero. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::increment_counter; @@ -455,6 +470,10 @@ pub use metrics_macros::increment_counter; /// Counters represent a single monotonic value, which means the value can only be incremented, not /// decremented, and always starts out with an initial value of zero. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::counter; @@ -478,6 +497,10 @@ pub use metrics_macros::counter; /// Gauges represent a single value that can go up or down over time, and always starts out with an /// initial value of zero. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::gauge; @@ -501,6 +524,10 @@ pub use metrics_macros::gauge; /// Gauges represent a single value that can go up or down over time, and always starts out with an /// initial value of zero. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::increment_gauge; @@ -524,6 +551,10 @@ pub use metrics_macros::increment_gauge; /// Gauges represent a single value that can go up or down over time, and always starts out with an /// initial value of zero. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::decrement_gauge; @@ -556,6 +587,10 @@ pub use metrics_macros::decrement_gauge; /// External libraries and applications can create their own conversions by implementing the /// [`IntoF64`] trait for their types, which is required for the value being passed to `histogram!`. /// +/// Metric names are shown below using string literals, but they can also be owned `String` values, +/// which includes using macros such as `format!` directly at the callsite. String literals are +/// preferred for performance where possible. +/// /// # Example /// ``` /// # use metrics::histogram;