Review

autometrics-macros/Cargo.toml

@@ -19,4 +19,3 @@ percent-encoding = "2.2"
 proc-macro2 = "1"
 quote = "1"
 syn =  { version = "1", features = ["full"] }
-spez = { version = "0.1.2" }

autometrics-macros/src/lib.rs

@@ -139,7 +139,7 @@ fn instrument_function(args: &AutometricsArgs, item: ItemFn) -> Result<TokenStre
         quote! {
             {
                 use autometrics::__private::{CALLER, CounterLabels, GetLabels};
-                let result_labels = autometrics::result_labels!(&result);
+                let result_labels = autometrics::get_result_labels_for_value!(&result);
                 CounterLabels::new(
                     #function_name,
                     module_path!(),

autometrics-macros/src/result_labels.rs

@@ -1,28 +1,5 @@
-//! The definition of the ResultLabels derive macro, that allows to specify
-//! inside an enumeration whether variants should be considered as errors or
-//! successes as far as the automatic metrics are concerned.
-//!
-//! For example, this would allow you to put all the client-side errors in a
-//! HTTP webserver (4**) as successes, since it means the handler function
-//! _successfully_ rejected a bad request, and that should not affect the SLO or
-//! the success rate of the function in the metrics.
-//!
-//! ```rust,ignore
-//! #[derive(ResultLabels)]
-//! enum ServiceError {
-//! // By default, the variant will be labeled as an error,
-//! // so you do not need to decorate every variant
-//! Database,
-//! // It is possible to mention it as well of course.
-//! // Only "error" and "ok" are accepted values
-//! #[label(result = "error")]
-//! Network,
-//! #[label(result = "ok")]
-//! Authentication,
-//! #[label(result = "ok")]
-//! Authorization,
-//! }
-//! ```
+//! The definition of the ResultLabels derive macro, see
+//! autometrics::ResultLabels for more information.
 
 use proc_macro2::TokenStream;
 use quote::quote;
@@ -41,15 +18,15 @@ const ACCEPTED_LABELS: [&str; 2] = [ERROR_KEY, OK_KEY];
 
 /// Entry point of the ResultLabels macro
 pub(crate) fn expand(input: DeriveInput) -> Result<TokenStream> {
-    let Data::Enum(DataEnum {
-        variants,
-        ..}) = &input.data else
-        {
-                return Err(Error::new_spanned(
-                    input,
-                    "ResultLabels only works with 'Enum's.",
-                ))
-        };
+    let variants = match &input.data {
+        Data::Enum(DataEnum { variants, .. }) => variants,
+        _ => {
+            return Err(Error::new_spanned(
+                input,
+                "ResultLabels only works with 'Enum's.",
+            ))
+        }
+    };
     let enum_name = &input.ident;
     let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
     let conditional_clauses_for_labels = conditional_label_clauses(variants, enum_name)?;
@@ -58,7 +35,7 @@ pub(crate) fn expand(input: DeriveInput) -> Result<TokenStream> {
         #[automatically_derived]
         impl #impl_generics ::autometrics::__private::GetLabels for #enum_name #ty_generics #where_clause {
             fn __autometrics_get_labels(&self) -> Option<&'static str> {
-                #(#conditional_clauses_for_labels)*
+                #conditional_clauses_for_labels
             }
         }
     })
@@ -68,36 +45,38 @@ pub(crate) fn expand(input: DeriveInput) -> Result<TokenStream> {
 fn conditional_label_clauses(
     variants: &Punctuated<Variant, Comma>,
     enum_name: &Ident,
-) -> Result<Vec<TokenStream>> {
-    // Dummy first clause to write all the useful payload with 'else if's
-    std::iter::once(Ok(quote![if false {
-        None
-    }]))
-    .chain(variants.iter().map(|variant| {
-        let variant_name = &variant.ident;
-        let variant_matcher: TokenStream = match variant.fields {
-            syn::Fields::Named(_) => quote! { #variant_name {..} },
-            syn::Fields::Unnamed(_) => quote! { #variant_name (_) },
-            syn::Fields::Unit => quote! { #variant_name },
-        };
-        if let Some(key) = extract_label_attribute(&variant.attrs)? {
-            Ok(quote! [
-                else if ::std::matches!(self, & #enum_name :: #variant_matcher) {
-                   Some(#key)
-                }
-            ])
-        } else {
-            // Let the code flow through the last value
-            Ok(quote! {})
+) -> Result<TokenStream> {
+    let clauses: Vec<TokenStream> = variants
+        .iter()
+        .map(|variant| {
+            let variant_name = &variant.ident;
+            let variant_matcher: TokenStream = match variant.fields {
+                syn::Fields::Named(_) => quote! { #variant_name {..} },
+                syn::Fields::Unnamed(_) => quote! { #variant_name (_) },
+                syn::Fields::Unit => quote! { #variant_name },
+            };
+            if let Some(key) = extract_label_attribute(&variant.attrs)? {
+                Ok(quote! [
+                    else if ::std::matches!(self, & #enum_name :: #variant_matcher) {
+                       Some(#key)
+                    }
+                ])
+            } else {
+                // Let the code flow through the last value
+                Ok(quote! {})
+            }
+        })
+        .collect::<Result<Vec<_>>>()?;
+
+    Ok(quote! [
+        if false {
+            None
         }
-    }))
-    // Fallback case: we return None
-    .chain(std::iter::once(Ok(quote! [
+        #(#clauses)*
         else {
             None
         }
-    ])))
-    .collect()
+    ])
 }
 
 /// Extract the wanted label from the annotation in the variant, if present.
@@ -124,11 +103,12 @@ fn extract_label_attribute(attrs: &[Attribute]) -> Result<Option<LitStr>> {
                         }
 
                         // Only lists are allowed
-                        let Some(syn::NestedMeta::Meta(syn::Meta::NameValue(pair))) = list.nested.first() else {
-                            return Some(Err(Error::new_spanned(
+                        let pair = match list.nested.first() {
+                            Some(syn::NestedMeta::Meta(syn::Meta::NameValue(pair))) => pair,
+                            _ => return Some(Err(Error::new_spanned(
                             meta,
                             format!("Only `{ATTR_LABEL}({RESULT_KEY} = \"RES\")` (RES can be {OK_KEY:?} or {ERROR_KEY:?}) is supported"),
-                            )))
+                            ))),
                         };
 
                         // Inside list, only 'result = ...' are allowed
@@ -140,11 +120,14 @@ fn extract_label_attribute(attrs: &[Attribute]) -> Result<Option<LitStr>> {
                         }
 
                         // Inside 'result = val', 'val' must be a string literal
-                        let Lit::Str(ref lit_str) = pair.lit else {
+                        let lit_str = match pair.lit {
+                            Lit::Str(ref lit_str) => lit_str,
+                            _ => {
                             return Some(Err(Error::new_spanned(
                                 &pair.lit,
                             format!("Only {OK_KEY:?} or {ERROR_KEY:?}, as string literals, are accepted as result values"),
                             )));
+                        }
                         };
 
                         // Inside 'result = val', 'val' must be one of the allowed string literals

autometrics/src/labels.rs

@@ -229,8 +229,19 @@ pub trait GetStaticStr {
 }
 impl_trait_for_types!(GetStaticStr);
 
+/// Return the value of labels to use for the "result" counter according to
+/// the value's exact type and attributes.
+///
+/// The macro uses the autoref specialization trick through spez to get the labels for the type in a variety of circumstances.
+/// Specifically, if the value is a Result, it will add the ok or error label accordingly unless one or both of the types that
+/// the Result<T, E> is generic over implements the GetLabels trait. The label allows to override the inferred label, and the
+/// [`ResultLabels`](crate::result_labels) macro implements the GetLabels trait for the user using annotations.
+///
+/// The macro is meant to be called with a reference as argument: `get_result_labels_for_value(&return_value)`
+///
+/// Ref: https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md
 #[macro_export]
-macro_rules! result_labels {
+macro_rules! get_result_labels_for_value {
     ($e:expr) => {{
         use $crate::__private::{
             GetLabels, GetStaticStr, ResultAndReturnTypeLabels, ERROR_KEY, OK_KEY,

autometrics/src/lib.rs

@@ -136,14 +136,13 @@ pub use autometrics_macros::autometrics;
 
 /// # Autometrics custom error labelling
 ///
-/// The ResultLabels derive macro allows to specify
-/// inside an enumeration whether variants should be considered as errors or
-/// successes as far as the [automatic metrics](autometrics) are concerned.
+/// The `ResultLabels` derive macro allows to specify
+/// inside an enumeration whether its variants should be considered as errors or
+/// successes from the perspective of [automatic metrics](autometrics) generated by
+/// autometrics.
 ///
-/// For example, this would allow you to put all the client-side errors in a
-/// HTTP webserver (4**) as successes, since it means the handler function
-/// _successfully_ rejected a bad request, and that should not affect the SLO or
-/// the success rate of the function in the metrics.
+/// For example, this would allow you to ignore the client-side HTTP errors (400-499)
+/// from the function's success rate and any Service-Level Objectives (SLOs) it is part of.
 ///
 /// Putting such a policy in place would look like this in code:
 ///
@@ -152,13 +151,21 @@ pub use autometrics_macros::autometrics;
 ///
 /// #[derive(ResultLabels)]
 /// pub enum ServiceError {
-///     // By default, the variant will be labeled as an error,
-///     // so you do not need to decorate every variant
+///     // By default, the variant will be inferred by the context,
+///     // so you do not need to decorate every variant.
+///     // - if ServiceError::Database is in an `Err(_)` variant, it will be an "error",
+///     // - if ServiceError::Database is in an `Ok(_)` variant, it will be an "ok",
+///     // - otherwise, no label will be added
 ///     Database,
 ///     // It is possible to mention it as well of course.
 ///     // Only "error" and "ok" are accepted values
+///     //
+///     // Forcing "error" here means that even returning `Ok(ServiceError::Network)`
+///     // from a function will count as an error for autometrics.
 ///     #[label(result = "error")]
 ///     Network,
+///     // Forcing "ok" here means that even returning `Err(ServiceError::Authentication)`
+///     // from a function will count as a success for autometrics.
 ///     #[label(result = "ok")]
 ///     Authentication,
 ///     #[label(result = "ok")]
@@ -172,6 +179,17 @@ pub use autometrics_macros::autometrics;
 /// `ServiceError::Authentication` or `Authorization` would _not_ count as a
 /// failure from your handler that should trigger alerts and consume the "error
 /// budget" of the service.
+///
+/// ## Per-function labelling
+///
+/// The `ResultLabels` macro does _not_ have the granularity to behave
+/// differently on different functions: if returning
+/// `ServiceError::Authentication` from `function_a` is "ok", then returning
+/// `ServiceError::Authentication` from `function_b` will be "ok" too.
+///
+/// To work around this, you must use the `ok_if` or `error_if` arguments to the
+/// [autometrics](crate::autometrics) invocation on `function_b`: those
+/// directives have priority over the ResultLabels annotations.
 pub use autometrics_macros::ResultLabels;
 
 // Optional exports

autometrics/tests/result_labels/pass/macro.rs

@@ -2,9 +2,9 @@
 //!
 //! The goal here is to make sure that the macro has the effect we want.
 //! autometrics (the library) is then responsible for orchestrating the
-//! calls to `result_labels!` correctly when observing
+//! calls to `get_result_labels_for_value!` correctly when observing
 //! function call results for the metrics.
-use autometrics::result_labels;
+use autometrics::get_result_labels_for_value;
 use autometrics_macros::ResultLabels;
 
 #[derive(Clone)]
@@ -34,36 +34,36 @@ enum MyEnum {
 
 fn main() {
     let is_ok = MyEnum::ClientError { inner: Inner {} };
-    let labels = result_labels!(&is_ok);
+    let labels = get_result_labels_for_value!(&is_ok);
     assert_eq!(labels.unwrap().0, "ok");
 
     let err = MyEnum::Empty;
-    let labels = result_labels!(&err);
+    let labels = get_result_labels_for_value!(&err);
     assert_eq!(labels.unwrap().0, "error");
 
     let no_idea = MyEnum::AmbiguousValue(42);
-    let labels = result_labels!(&no_idea);
+    let labels = get_result_labels_for_value!(&no_idea);
     assert_eq!(labels, None);
 
     // Testing behaviour within an Ok() error variant
     let ok: Result<MyEnum, ()> = Ok(is_ok.clone());
-    let labels = result_labels!(&ok);
+    let labels = get_result_labels_for_value!(&ok);
     assert_eq!(
         labels.unwrap().0,
         "ok",
         "When wrapped as the Ok variant of a result, a manually marked 'ok' variant translates to 'ok'."
     );
 
     let ok: Result<MyEnum, ()> = Ok(no_idea.clone());
-    let labels = result_labels!(&ok);
+    let labels = get_result_labels_for_value!(&ok);
     assert_eq!(
         labels.unwrap().0,
         "ok",
         "When wrapped as the Ok variant of a result, an ambiguous variant translates to 'ok'."
     );
 
     let err_in_ok: Result<MyEnum, ()> = Ok(err.clone());
-    let labels = result_labels!(&err_in_ok);
+    let labels = get_result_labels_for_value!(&err_in_ok);
     assert_eq!(
         labels.unwrap().0,
         "error",
@@ -72,23 +72,23 @@ fn main() {
 
     // Testing behaviour within an Err() error variant
     let ok_in_err: Result<(), MyEnum> = Err(is_ok);
-    let labels = result_labels!(&ok_in_err);
+    let labels = get_result_labels_for_value!(&ok_in_err);
     assert_eq!(
         labels.unwrap().0,
         "ok",
         "When wrapped as the Err variant of a result, a manually marked 'ok' variant translates to 'ok'."
     );
 
     let not_ok: Result<(), MyEnum> = Err(err);
-    let labels = result_labels!(&not_ok);
+    let labels = get_result_labels_for_value!(&not_ok);
     assert_eq!(
         labels.unwrap().0,
         "error",
         "When wrapped as the Err variant of a result, a manually marked 'error' variant translates to 'error'."
     );
 
     let ambiguous: Result<(), MyEnum> = Err(no_idea);
-    let labels = result_labels!(&ambiguous);
+    let labels = get_result_labels_for_value!(&ambiguous);
     assert_eq!(
         labels.unwrap().0,
         "error",