Skip to content

Overhaul stats: Refactor AggregateValue in metrics package #1580

@josecelano

Description

@josecelano

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

Metadata

Metadata

Assignees

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions