Skip to content

Commit 0d9f883

Browse files
josecelanojosecelano
committed
refactor(metrics): [#1580] convert Sum trait to use associated types for mathematically correct return types
- Replace AggregateValue return type with associated Output type in Sum trait - Counter metrics now return u64 (preserving integer precision) - Gauge metrics now return f64 (avoiding unnecessary wrapper type) - Update all test cases to expect primitive types instead of AggregateValue - Convert primitive results to AggregateValue at collection level for backward compatibility - Use proper floating-point comparison in gauge tests with epsilon tolerance This change ensures each aggregate function returns the mathematically appropriate type while maintaining API compatibility for metric collections.
1 parent 70fd119 commit 0d9f883

File tree

2 files changed

+47
-45
lines changed

2 files changed

+47
-45
lines changed

‎packages/metrics/src/metric/aggregate/sum.rs‎

Lines changed: 38 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,34 @@
1-
use crate::aggregate::AggregateValue;
21
use crate::counter::Counter;
32
use crate::gauge::Gauge;
43
use crate::label::LabelSet;
54
use crate::metric::Metric;
65

76
pub trait Sum {
8-
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue;
7+
type Output;
8+
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output;
99
}
1010

1111
impl Sum for Metric<Counter> {
12-
#[allow(clippy::cast_precision_loss)]
13-
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue {
14-
let sum: f64 = self
15-
.sample_collection
12+
type Output = u64;
13+
14+
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output {
15+
self.sample_collection
1616
.iter()
1717
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
18-
.map(|(_label_set, measurement)| measurement.value().primitive() as f64)
19-
.sum();
20-
21-
sum.into()
18+
.map(|(_label_set, measurement)| measurement.value().primitive())
19+
.sum()
2220
}
2321
}
2422

2523
impl Sum for Metric<Gauge> {
26-
fn sum(&self, label_set_criteria: &LabelSet) -> AggregateValue {
27-
let sum: f64 = self
28-
.sample_collection
24+
type Output = f64;
25+
26+
fn sum(&self, label_set_criteria: &LabelSet) -> Self::Output {
27+
self.sample_collection
2928
.iter()
3029
.filter(|(label_set, _measurement)| label_set.matches(label_set_criteria))
3130
.map(|(_label_set, measurement)| measurement.value().primitive())
32-
.sum();
33-
34-
sum.into()
31+
.sum()
3532
}
3633
}
3734

@@ -40,7 +37,6 @@ mod tests {
4037

4138
use torrust_tracker_primitives::DurationSinceUnixEpoch;
4239

43-
use crate::aggregate::AggregateValue;
4440
use crate::counter::Counter;
4541
use crate::gauge::Gauge;
4642
use crate::label::LabelSet;
@@ -83,22 +79,22 @@ mod tests {
8379
}
8480
}
8581

86-
fn counter_cases() -> Vec<(Metric<Counter>, LabelSet, AggregateValue)> {
82+
fn counter_cases() -> Vec<(Metric<Counter>, LabelSet, u64)> {
8783
// (metric, label set criteria, expected_aggregate_value)
8884
vec![
8985
// Metric with one sample without label set
9086
(
9187
MetricBuilder::default().with_sample(1.into(), &LabelSet::empty()).build(),
9288
LabelSet::empty(),
93-
1.0.into(),
89+
1,
9490
),
9591
// Metric with one sample with a label set
9692
(
9793
MetricBuilder::default()
9894
.with_sample(1.into(), &[("l1", "l1_value")].into())
9995
.build(),
10096
[("l1", "l1_value")].into(),
101-
1.0.into(),
97+
1,
10298
),
10399
// Metric with two samples, different label sets, sum all
104100
(
@@ -107,7 +103,7 @@ mod tests {
107103
.with_sample(2.into(), &[("l2", "l2_value")].into())
108104
.build(),
109105
LabelSet::empty(),
110-
3.0.into(),
106+
3,
111107
),
112108
// Metric with two samples, different label sets, sum one
113109
(
@@ -116,7 +112,7 @@ mod tests {
116112
.with_sample(2.into(), &[("l2", "l2_value")].into())
117113
.build(),
118114
[("l1", "l1_value")].into(),
119-
1.0.into(),
115+
1,
120116
),
121117
// Metric with two samples, same label key, different label values, sum by key
122118
(
@@ -125,7 +121,7 @@ mod tests {
125121
.with_sample(2.into(), &[("l1", "l1_value"), ("lb", "lb_value")].into())
126122
.build(),
127123
[("l1", "l1_value")].into(),
128-
3.0.into(),
124+
3,
129125
),
130126
// Metric with two samples, different label values, sum by subkey
131127
(
@@ -134,54 +130,53 @@ mod tests {
134130
.with_sample(2.into(), &[("l1", "l1_value"), ("lb", "lb_value")].into())
135131
.build(),
136132
[("la", "la_value")].into(),
137-
1.0.into(),
133+
1,
138134
),
139135
// Edge: Metric with no samples at all
140-
(MetricBuilder::default().build(), LabelSet::empty(), 0.0.into()),
136+
(MetricBuilder::default().build(), LabelSet::empty(), 0),
141137
// Edge: Metric with samples but no matching labels
142138
(
143139
MetricBuilder::default()
144140
.with_sample(5.into(), &[("foo", "bar")].into())
145141
.build(),
146142
[("not", "present")].into(),
147-
0.0.into(),
143+
0,
148144
),
149145
// Edge: Metric with zero value
150146
(
151147
MetricBuilder::default()
152148
.with_sample(0.into(), &[("l3", "l3_value")].into())
153149
.build(),
154150
[("l3", "l3_value")].into(),
155-
0.0.into(),
151+
0,
156152
),
157153
// Edge: Metric with a very large value
158154
(
159155
MetricBuilder::default()
160156
.with_sample(u64::MAX.into(), &LabelSet::empty())
161157
.build(),
162158
LabelSet::empty(),
163-
#[allow(clippy::cast_precision_loss)]
164-
(u64::MAX as f64).into(),
159+
u64::MAX,
165160
),
166161
]
167162
}
168163

169-
fn gauge_cases() -> Vec<(Metric<Gauge>, LabelSet, AggregateValue)> {
164+
fn gauge_cases() -> Vec<(Metric<Gauge>, LabelSet, f64)> {
170165
// (metric, label set criteria, expected_aggregate_value)
171166
vec![
172167
// Metric with one sample without label set
173168
(
174169
MetricBuilder::default().with_sample(1.0.into(), &LabelSet::empty()).build(),
175170
LabelSet::empty(),
176-
1.0.into(),
171+
1.0,
177172
),
178173
// Metric with one sample with a label set
179174
(
180175
MetricBuilder::default()
181176
.with_sample(1.0.into(), &[("l1", "l1_value")].into())
182177
.build(),
183178
[("l1", "l1_value")].into(),
184-
1.0.into(),
179+
1.0,
185180
),
186181
// Metric with two samples, different label sets, sum all
187182
(
@@ -190,7 +185,7 @@ mod tests {
190185
.with_sample(2.0.into(), &[("l2", "l2_value")].into())
191186
.build(),
192187
LabelSet::empty(),
193-
3.0.into(),
188+
3.0,
194189
),
195190
// Metric with two samples, different label sets, sum one
196191
(
@@ -199,7 +194,7 @@ mod tests {
199194
.with_sample(2.0.into(), &[("l2", "l2_value")].into())
200195
.build(),
201196
[("l1", "l1_value")].into(),
202-
1.0.into(),
197+
1.0,
203198
),
204199
// Metric with two samples, same label key, different label values, sum by key
205200
(
@@ -208,7 +203,7 @@ mod tests {
208203
.with_sample(2.0.into(), &[("l1", "l1_value"), ("lb", "lb_value")].into())
209204
.build(),
210205
[("l1", "l1_value")].into(),
211-
3.0.into(),
206+
3.0,
212207
),
213208
// Metric with two samples, different label values, sum by subkey
214209
(
@@ -217,25 +212,25 @@ mod tests {
217212
.with_sample(2.0.into(), &[("l1", "l1_value"), ("lb", "lb_value")].into())
218213
.build(),
219214
[("la", "la_value")].into(),
220-
1.0.into(),
215+
1.0,
221216
),
222217
// Edge: Metric with no samples at all
223-
(MetricBuilder::default().build(), LabelSet::empty(), 0.0.into()),
218+
(MetricBuilder::default().build(), LabelSet::empty(), 0.0),
224219
// Edge: Metric with samples but no matching labels
225220
(
226221
MetricBuilder::default()
227222
.with_sample(5.0.into(), &[("foo", "bar")].into())
228223
.build(),
229224
[("not", "present")].into(),
230-
0.0.into(),
225+
0.0,
231226
),
232227
// Edge: Metric with zero value
233228
(
234229
MetricBuilder::default()
235230
.with_sample(0.0.into(), &[("l3", "l3_value")].into())
236231
.build(),
237232
[("l3", "l3_value")].into(),
238-
0.0.into(),
233+
0.0,
239234
),
240235
// Edge: Metric with negative values
241236
(
@@ -244,15 +239,15 @@ mod tests {
244239
.with_sample(3.0.into(), &[("l5", "l5_value")].into())
245240
.build(),
246241
LabelSet::empty(),
247-
1.0.into(),
242+
1.0,
248243
),
249244
// Edge: Metric with a very large value
250245
(
251246
MetricBuilder::default()
252247
.with_sample(f64::MAX.into(), &LabelSet::empty())
253248
.build(),
254249
LabelSet::empty(),
255-
f64::MAX.into(),
250+
f64::MAX,
256251
),
257252
]
258253
}
@@ -274,8 +269,8 @@ mod tests {
274269
for (idx, (metric, criteria, expected_value)) in gauge_cases().iter().enumerate() {
275270
let sum = metric.sum(criteria);
276271

277-
assert_eq!(
278-
sum, *expected_value,
272+
assert!(
273+
(sum - expected_value).abs() <= f64::EPSILON,
279274
"at case {idx}, expected sum to be {expected_value}, got {sum}"
280275
);
281276
}

‎packages/metrics/src/metric_collection/aggregate.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,20 @@ impl Sum for MetricCollection {
2222

2323
impl Sum for MetricKindCollection<Counter> {
2424
fn sum(&self, metric_name: &MetricName, label_set_criteria: &LabelSet) -> Option<AggregateValue> {
25-
self.metrics.get(metric_name).map(|metric| metric.sum(label_set_criteria))
25+
self.metrics.get(metric_name).map(|metric| {
26+
let sum: u64 = metric.sum(label_set_criteria);
27+
#[allow(clippy::cast_precision_loss)]
28+
AggregateValue::new(sum as f64)
29+
})
2630
}
2731
}
2832

2933
impl Sum for MetricKindCollection<Gauge> {
3034
fn sum(&self, metric_name: &MetricName, label_set_criteria: &LabelSet) -> Option<AggregateValue> {
31-
self.metrics.get(metric_name).map(|metric| metric.sum(label_set_criteria))
35+
self.metrics.get(metric_name).map(|metric| {
36+
let sum: f64 = metric.sum(label_set_criteria);
37+
AggregateValue::new(sum)
38+
})
3239
}
3340
}
3441

0 commit comments

Comments
 (0)