-
Notifications
You must be signed in to change notification settings - Fork 49
Description
In our last weekly meeting, we discussed refactoring the AggregateValue type.
#[derive(Debug, Display, Clone, Copy, PartialEq, Default)]
pub struct AggregateValue(f64);
impl AggregateValue {
#[must_use]
pub fn new(value: f64) -> Self {
Self(value)
}
#[must_use]
pub fn value(&self) -> f64 {
self.0
}
}
impl From<f64> for AggregateValue {
fn from(value: f64) -> Self {
Self(value)
}
}
impl From<AggregateValue> for f64 {
fn from(value: AggregateValue) -> Self {
value.0
}
}We will implement a different solution where each aggregate function can return an integer or float aggregate value, instead of always converting all values to float.
For example, the "sum" aggregate function for counters should be an integer, while for gauges it should be a float:
pub trait Sum {
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue;
}
impl Sum for Metric<Counter> {
#[allow(clippy::cast_precision_loss)]
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue {
let sum: f64 = self
.sample_collection
.iter()
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
.map(|(_label_set, measurement)| measurement.value().primitive() as f64)
.sum();
sum.into()
}
}
impl Sum for Metric<Gauge> {
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue {
let sum: f64 = self
.sample_collection
.iter()
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
.map(|(_label_set, measurement)| measurement.value().primitive())
.sum();
sum.into()
}
}Note: These alternatives were generated by GitHub Copilot using the Claude Sonnet 4 model.
Type-Safe Aggregate Functions for Metrics
Looking at the current Sum implementation, I see that both Counter and Gauge metrics return AggregateValue (which wraps f64), but this loses type information about the natural result type of the operation.
The current code converts counter values (internally u64) to f64 with a precision loss warning, when ideally summing counters should return an integer, while summing gauges naturally returns a float.
Proposed Solutions
Here are several alternatives to express this better:
Option 1: Generic Associated Type (Recommended)
Make the Sum trait generic with an associated type:
pub trait Sum {
type Output;
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output;
}
impl Sum for Metric<Counter> {
type Output = u64;
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output {
self.sample_collection
.iter()
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
.map(|(_label_set, measurement)| measurement.value().primitive())
.sum()
}
}
impl Sum for Metric<Gauge> {
type Output = f64;
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output {
self.sample_collection
.iter()
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
.map(|(_label_set, measurement)| measurement.value().primitive())
.sum()
}
}Option 2: Separate Traits
Create separate traits for different return types:
pub trait IntegerSum {
fn sum(&self, label_set_criteria: &LabelSet) -> u64;
}
pub trait FloatSum {
fn sum(&self, label_set_criteria: &LabelSet) -> f64;
}
impl IntegerSum for Metric<Counter> {
fn sum(&self, label_set_criteria: &LabelSet) -> u64 {
// implementation
}
}
impl FloatSum for Metric<Gauge> {
fn sum(&self, label_set_criteria: &LabelSet) -> f64 {
// implementation
}
}Option 3: Enum Result Type
Create a more expressive aggregate result type:
#[derive(Debug, Clone, PartialEq)]
pub enum AggregateResult {
Integer(u64),
Float(f64),
}
impl From<u64> for AggregateResult {
fn from(value: u64) -> Self {
Self::Integer(value)
}
}
impl From<f64> for AggregateResult {
fn from(value: f64) -> Self {
Self::Float(value)
}
}
pub trait Sum {
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateResult;
}Benefits of Option 1 (Associated Types)
- Type Safety: The return type is determined at compile time based on the metric type
- No Precision Loss: Counter sums remain as integers
- Simplicity: Single trait with clear semantics
- Extensibility: Easy to add other aggregate functions with appropriate return types
- Ergonomics: Natural to use - metric.sum() returns the appropriate type
Future Considerations
This approach also handles future aggregate functions well:
- AVERAGE for counters could return f64 even though counters store integers
- MIN/MAX would return the same type as the metric's primitive type
- Complex aggregations could return appropriate numeric types
Recommendation
I recommend Option 1 (Generic Associated Type) as it provides the best balance of type safety, expressiveness, and usability for the metrics aggregation system while eliminating the precision loss warning and maintaining natural type semantics.
cc @da2ce7