Skip to content

Add compile-time validation for histogram buckets in foundations-macros #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
76 changes: 75 additions & 1 deletion foundations-macros/src/metrics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use syn::{
};

mod parsing;
mod validation;

#[derive(FromMeta)]
struct MacroArgs {
Expand Down Expand Up @@ -279,6 +280,18 @@ fn metric_init(foundations: &Path, fn_: &ItemFn) -> proc_macro2::TokenStream {
Span::call_site(),
);

// Validate histogram buckets at compile time if this is a HistogramBuilder
if let Some(ctor) = ctor {
if let Some(path) = ctor.path.get_ident() {
if path == "HistogramBuilder" {
// Validate the histogram buckets
if let Err(err) = validation::validate_histogram_buckets(ctor) {
return err.to_compile_error();
}
}
}
}

let metric_init = match ctor {
Some(ctor) if args.is_empty() => quote! {
#reexports::prometheus_client::metrics::family::MetricConstructor::new_metric(&(#ctor))
Expand Down Expand Up @@ -378,7 +391,8 @@ fn metric_fn(foundations: &Path, metrics_struct: &Ident, fn_: &ItemFn) -> proc_m
mod tests {
use super::*;
use crate::common::test_utils::{code_str, parse_attr};
use syn::parse_quote;
use crate::metrics::validation;
use syn::{parse_quote, ExprStruct};

#[test]
fn expand_empty() {
Expand Down Expand Up @@ -736,4 +750,64 @@ mod tests {

assert_eq!(actual, expected);
}

#[test]
fn test_valid_histogram_buckets() {
// Valid strictly increasing buckets
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: &[0.1, 0.2, 0.3, 0.4] }
};

assert!(validation::validate_histogram_buckets(&expr).is_ok());
}

#[test]
fn test_empty_histogram_buckets() {
// Empty buckets are valid
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: &[] }
};

assert!(validation::validate_histogram_buckets(&expr).is_ok());
}

#[test]
fn test_invalid_histogram_buckets_equal_values() {
// Invalid buckets with equal values
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: &[0.1, 0.2, 0.2, 0.3] }
};

assert!(validation::validate_histogram_buckets(&expr).is_err());
}

#[test]
fn test_invalid_histogram_buckets_decreasing_values() {
// Invalid buckets with decreasing values
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: &[0.3, 0.2, 0.1] }
};

assert!(validation::validate_histogram_buckets(&expr).is_err());
}

#[test]
fn test_skip_validation_for_variable_buckets() {
// Variable buckets should skip validation
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: MY_BUCKETS }
};

assert!(validation::validate_histogram_buckets(&expr).is_ok());
}

#[test]
fn test_skip_validation_for_function_buckets() {
// Function call buckets should skip validation
let expr: ExprStruct = parse_quote! {
HistogramBuilder { buckets: get_buckets() }
};

assert!(validation::validate_histogram_buckets(&expr).is_ok());
}
}
62 changes: 62 additions & 0 deletions foundations-macros/src/metrics/validation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use syn::{Error, Expr, ExprLit, ExprStruct, Lit, Member, Result};

/// Validates that histogram bucket values are strictly increasing.
/// Returns an error if the buckets are not valid.
pub fn validate_histogram_buckets(expr: &ExprStruct) -> Result<()> {
// Extract the buckets field from the HistogramBuilder struct
let buckets_field = expr
.fields
.iter()
.find(|field| {
if let Member::Named(ident) = &field.member {
ident == "buckets"
} else {
false
}
})
.ok_or_else(|| Error::new_spanned(expr, "HistogramBuilder must have a 'buckets' field"))?;

// Extract the array expression from the buckets field
let array_expr = match &buckets_field.expr {
Expr::Reference(ref_expr) => match &*ref_expr.expr {
Expr::Array(array) => array,
// For non-array literals (like variables or function calls), skip validation
_ => return Ok(()),
},
// For non-reference expressions, skip validation
_ => return Ok(()),
};

// Extract the f64 values from the array
let mut values = Vec::new();
for elem in &array_expr.elems {
if let Expr::Lit(ExprLit {
lit: Lit::Float(lit_float),
..
}) = elem
{
let value = lit_float.base10_parse::<f64>()?;
values.push((value, lit_float.span()));
} else {
// For non-float literals (like variables or expressions), skip validation
return Ok(());
}
}

// Check if the buckets are strictly increasing
if !values.is_empty() {
let mut prev_value = values[0].0;
for (i, (value, span)) in values.iter().enumerate().skip(1) {
if *value <= prev_value {
let message = format!(
"Histogram buckets must be strictly increasing. Found invalid bucket at position {}: {} <= {}",
i, value, prev_value
);
return Err(Error::new(*span, message));
}
prev_value = *value;
}
}

Ok(())
}