Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use spez to determine result labels
  • Loading branch information
Gerry Agbobada committed May 3, 2023
commit dba82141a82505dd5dc305759c0897ca9162f2c5
1 change: 1 addition & 0 deletions autometrics-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ percent-encoding = "2.2"
proc-macro2 = "1"
quote = "1"
syn = { version = "1", features = ["full"] }
spez = { version = "0.1.2" }
12 changes: 7 additions & 5 deletions autometrics-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use quote::quote;
use std::env;
use syn::{parse_macro_input, ImplItem, ItemFn, ItemImpl, Result};

mod result_labels;
mod parse;
mod result_labels;

const COUNTER_NAME_PROMETHEUS: &str = "function_calls_count";
const HISTOGRAM_BUCKET_NAME_PROMETHEUS: &str = "function_calls_duration_bucket";
Expand Down Expand Up @@ -106,12 +106,14 @@ fn instrument_function(args: &AutometricsArgs, item: ItemFn) -> Result<TokenStre
}
}
} else {
// This will use the traits defined in the `labels` module to determine if
// the return value was a `Result` and, if so, assign the appropriate labels
let labels = quote! {
autometrics::result_labels!(&result)
};

quote! {
{
use autometrics::__private::{CALLER, CounterLabels, GetLabels, GetLabelsFromResult};
let result_labels = (&result).__autometrics_get_labels();
use autometrics::__private::{CALLER, CounterLabels, GetLabels};
let result_labels = #labels;
CounterLabels::new(
#function_name,
module_path!(),
Expand Down
17 changes: 2 additions & 15 deletions autometrics-macros/src/result_labels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,10 @@ pub(crate) fn expand(input: DeriveInput) -> Result<TokenStream> {
// is a reference &T, and the blanket impl of GetResultLabel for &T will be used instead of
// the implementation we just wrote.
Ok(quote! {
#[automatically_derived]
impl #impl_generics ::autometrics::__private::GetResultLabel for #enum_name #ty_generics #where_clause {
fn __autometrics_get_result_label(&self) -> Option<&'static str> {
#(#conditional_clauses_for_labels)*
}
}

#[automatically_derived]
impl #impl_generics ::autometrics::__private::GetLabels for #enum_name #ty_generics #where_clause {
fn __autometrics_get_labels(&self) -> Option<::autometrics::__private::ResultAndReturnTypeLabels> {
use ::autometrics::__private::GetStaticStr;

let result_label = {
#(#conditional_clauses_for_labels)*
};

result_label.map(|label| (label, label.__autometrics_static_str()))
fn __autometrics_get_labels(&self) -> Option<&'static str> {
#(#conditional_clauses_for_labels)*
}
}
})
Expand Down
1 change: 1 addition & 0 deletions autometrics/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ custom-objective-latency = []

[dependencies]
autometrics-macros = { workspace = true }
spez = { version = "0.1.2" }

# Used for opentelemetry feature
opentelemetry_api = { version = "0.18", default-features = false, features = ["metrics"], optional = true }
Expand Down
105 changes: 72 additions & 33 deletions autometrics/src/labels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,26 +149,6 @@ impl GaugeLabels {
// and this answer explains why it works:
// https://users.rust-lang.org/t/how-to-check-types-within-macro/33803/8

pub trait GetLabelsFromResult {
fn __autometrics_get_labels(&self) -> Option<ResultAndReturnTypeLabels> {
None
}
}

impl<T, E> GetLabelsFromResult for Result<T, E> {
fn __autometrics_get_labels(&self) -> Option<ResultAndReturnTypeLabels> {
match self {
Ok(ok) => Some((
ok.__autometrics_get_result_label().unwrap_or(OK_KEY),
ok.__autometrics_static_str(),
)),
Err(err) => Some((
err.__autometrics_get_result_label().unwrap_or(ERROR_KEY),
err.__autometrics_static_str(),
)),
}
}
}
pub enum LabelArray {
Three([Label; 3]),
Four([Label; 4]),
Expand All @@ -188,9 +168,7 @@ impl Deref for LabelArray {
}

pub trait GetLabels {
fn __autometrics_get_labels(&self) -> Option<ResultAndReturnTypeLabels> {
None
}
fn __autometrics_get_labels(&self) -> Option<&'static str>;
}

/// Implement the given trait for &T and all primitive types.
Expand Down Expand Up @@ -230,8 +208,6 @@ macro_rules! impl_trait_for_types {
};
}

impl_trait_for_types!(GetLabels);

pub trait GetStaticStrFromIntoStaticStr<'a> {
fn __autometrics_static_str(&'a self) -> Option<&'static str>;
}
Expand All @@ -252,12 +228,75 @@ pub trait GetStaticStr {
}
impl_trait_for_types!(GetStaticStr);

/// Implementation detail to get enum variants to specify their own
/// "result" label
pub trait GetResultLabel {
/// Return the value to use for the [result](RESULT_KEY) value in the reported metrics
fn __autometrics_get_result_label(&self) -> Option<&'static str> {
None
}
#[macro_export]
macro_rules! result_labels {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of this should be in the macros crate, no? Why not just move this into the implementation of the autometrics proc macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the macro separate allows to properly test for it (and the ResultLabels attribute macro) in the trybuild tests. Inlining everything would make the main instrument_function code harder to read and make it impossible to test the label logic in isolation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it's not in the macros crate is mostly that it's just expanding code and that I wanted the macro to access values from autometrics (the constants, the labels, the traits): since the macro is not a proc macro returning an arbitrary TokenStream, we need to have proper access to the crate (which we don't have inside autometrics-macros)

($e:expr) => {{
use $crate::__private::{
GetLabels, GetStaticStr, ResultAndReturnTypeLabels, ERROR_KEY, OK_KEY,
};
$crate::__private::spez! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we could call spez directly in the proc macro code and insert the token stream directly into the code the macro generates, instead of making it a dependency of the main autometrics crate and having the macro generate code that calls spez (unless there's a reason that cannot work)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling the function spez::spez to get a proc_macro::TokenStream would probably work but it doesn't really bring advantages: calling spez::spez means that there would still be a dependency to spez (even if the macro is moved to autometrics-macros, it would be AM needs AM-macros needs spez), and preparing an input TokenStream from code is one additional step that would be unnecessary added complexity in the macro.

Generating the match Result… TokenStream in the instrument_function directly has the drawback of having bad testability and makes instrument_function readability worse

for val = $e;

match<T: GetLabels, E: GetLabels> &Result<T, E> -> Option<ResultAndReturnTypeLabels> {
match val {
Ok(ok) => Some((
ok.__autometrics_get_labels().unwrap_or(OK_KEY),
ok.__autometrics_static_str(),
)),
Err(err) => Some((
err.__autometrics_get_labels().unwrap_or(ERROR_KEY),
err.__autometrics_static_str(),
)),
}
}

match<T: GetLabels, E> &Result<T, E> -> Option<ResultAndReturnTypeLabels> {
match val {
Ok(ok) => Some((
ok.__autometrics_get_labels().unwrap_or(OK_KEY),
ok.__autometrics_static_str(),
)),
Err(err) => Some((
ERROR_KEY,
err.__autometrics_static_str(),
)),
}
}

match<T, E: GetLabels> &Result<T, E> -> Option<ResultAndReturnTypeLabels> {
match val {
Ok(ok) => Some((
OK_KEY,
ok.__autometrics_static_str(),
)),
Err(err) => Some((
err.__autometrics_get_labels().unwrap_or(ERROR_KEY),
err.__autometrics_static_str(),
)),
}
}

match<T, E> &Result<T, E> -> Option<ResultAndReturnTypeLabels> {
match val {
Ok(ok) => Some((
OK_KEY,
ok.__autometrics_static_str(),
)),
Err(err) => Some((
ERROR_KEY,
err.__autometrics_static_str(),
)),
}
}

match<T: GetLabels> &T -> Option<ResultAndReturnTypeLabels> {
val.__autometrics_get_labels().map(|label|
(label, val.__autometrics_static_str()))
}

match<T> &T -> Option<ResultAndReturnTypeLabels> {
None
}
}
}};
}
impl_trait_for_types!(GetResultLabel);
3 changes: 3 additions & 0 deletions autometrics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ pub mod __private {
use crate::task_local::LocalKey;
use std::{cell::RefCell, thread_local};

pub use crate::constants::*;
pub use crate::labels::*;
pub use crate::tracker::{AutometricsTracker, TrackMetrics};

Expand All @@ -213,4 +214,6 @@ pub mod __private {

LocalKey { inner: CALLER_KEY }
};

pub use spez::spez;
}
70 changes: 20 additions & 50 deletions autometrics/tests/result_labels/pass/macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 `__autometrics_get_result_label` correctly when observing
//! calls to `result_labels!` correctly when observing
//! function call results for the metrics.
use autometrics::__private::{GetLabels, GetLabelsFromResult, GetResultLabel};
use autometrics::result_labels;
use autometrics_macros::ResultLabels;

#[derive(Clone)]
Expand Down Expand Up @@ -34,93 +34,63 @@ enum MyEnum {

fn main() {
let is_ok = MyEnum::ClientError { inner: Inner {} };
assert_eq!(is_ok.__autometrics_get_result_label().unwrap(), "ok");
assert_eq!((&is_ok).__autometrics_get_result_label().unwrap(), "ok");
assert_eq!(is_ok.__autometrics_get_labels().unwrap().0, "ok");
assert_eq!((&is_ok).__autometrics_get_labels().unwrap().0, "ok");
let labels = result_labels!(&is_ok);
assert_eq!(labels.unwrap().0, "ok");

let err = MyEnum::Empty;
assert_eq!(err.__autometrics_get_result_label().unwrap(), "error");
assert_eq!((&err).__autometrics_get_result_label().unwrap(), "error");
assert_eq!(err.__autometrics_get_labels().unwrap().0, "error");
assert_eq!((&err).__autometrics_get_labels().unwrap().0, "error");
let labels = result_labels!(&err);
assert_eq!(labels.unwrap().0, "error");

let no_idea = MyEnum::AmbiguousValue(42);
assert_eq!(no_idea.__autometrics_get_result_label(), None);
assert_eq!((&no_idea).__autometrics_get_result_label(), None);
assert_eq!(no_idea.__autometrics_get_labels(), None);
assert_eq!((&no_idea).__autometrics_get_labels(), None);
let labels = result_labels!(&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);
assert_eq!(
ok.__autometrics_get_labels().unwrap().0,
"ok",
"When wrapped as the Ok variant of a result, a manually marked 'ok' variant translates to 'ok'."
);
assert_eq!(
(&ok).__autometrics_get_labels().unwrap().0,
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);
assert_eq!(
ok.__autometrics_get_labels().unwrap().0,
"ok",
"When wrapped as the Ok variant of a result, an ambiguous variant translates to 'ok'."
);
assert_eq!(
(&ok).__autometrics_get_labels().unwrap().0,
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);
assert_eq!(
err_in_ok.__autometrics_get_labels().unwrap().0,
"error",
"When wrapped as the Ok variant of a result, a manually marked 'error' variant translates to 'error'."
);
assert_eq!(
(&err_in_ok).__autometrics_get_labels().unwrap().0,
labels.unwrap().0,
"error",
"When wrapped as the Ok variant of a result, a manually marked 'error' variant translates to 'error'."
);

// Testing behaviour within an Err() error variant
let ok_in_err: Result<(), MyEnum> = Err(is_ok);
let labels = result_labels!(&ok_in_err);
assert_eq!(
ok_in_err.__autometrics_get_labels().unwrap().0,
"ok",
"When wrapped as the Err variant of a result, a manually marked 'ok' variant translates to 'ok'."
);
assert_eq!(
(&ok_in_err).__autometrics_get_labels().unwrap().0,
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);
assert_eq!(
not_ok.__autometrics_get_labels().unwrap().0,
"error",
"When wrapped as the Err variant of a result, a manually marked 'error' variant translates to 'error'."
);
assert_eq!(
(&not_ok).__autometrics_get_labels().unwrap().0,
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);
assert_eq!(
ambiguous.__autometrics_get_labels().unwrap().0,
"error",
"When wrapped as the Err variant of a result, an ambiguous variant translates to 'error'."
);
assert_eq!(
(&ambiguous).__autometrics_get_labels().unwrap().0,
labels.unwrap().0,
"error",
"When wrapped as the Err variant of a result, an ambiguous variant translates to 'error'."
);
Expand Down