Skip to content

Commit 813aa42

Browse files
committed
cbits: fix min/max combine logic
The internal `combine` function that is used to read the value of a `Distribution` contains `min` and `max` values, and as the name implies, combines them in the Semigroup sense: to combine two samples `a` and `b`, for `max` you simply calculate `max(a->max, b->max)`, likewise for `min` While this is fine in theory, in practice it is flawed for this case: to read a metric's current values, you must combine it with an empty sample, where `min = max = 0`. This logic is incorrect, because any value that was previously seen that is greater than zero will be overwritten by zero every time you read the sample. This is a flat out bug. (Likewise for `max` and values less-than zero). In more abstract terms, the identity members of the `min/max` monoids are *not* zero, but `maxBound/minBound`! However, there's an easier way to go forward here: `combine` is *only* ever used with a zero structure on the right hand side, and it is completely internal. Therefore the logic is trivial: just copy the `max/min` values from the old structure into the new structure, since there's no point in even doing the comparison anyway. Signed-off-by: Austin Seipp <aseipp@pobox.com>
1 parent c0ed981 commit 813aa42

File tree

1 file changed

+2
-2
lines changed

1 file changed

+2
-2
lines changed

cbits/distrib.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ void hs_distrib_combine(struct distrib* b, struct distrib* a) {
3838
a->mean = mean;
3939
a->sum_sq_delta = sum_sq_delta;
4040
a->sum = a->sum + b->sum;
41-
a->min = b->min < a->min ? b->min : a->min;
42-
a->max = b->max > a->max ? b->max : a->max;
41+
a->min = b->min;
42+
a->max = b->max;
4343
hs_unlock(&b->lock);
4444
}

0 commit comments

Comments
 (0)