Skip to content
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

fix: always use sum aggregator for sum & count #487

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

raghuram-periaswamy
Copy link

sum & count metrics of summary & histogram should always be summed irrespective of the custom aggregator that's mentioned as part of the metric definition.

Previously, aggregating the following two sample sets

set-1

http_duration_seconds{quantile="0.95"} 4
http_duration_seconds_sum 10
http_duration_seconds_count 5

set-2

http_duration_seconds{quantile="0.95"} 6
http_duration_seconds_sum 20
http_duration_seconds_count 5

using average yields the following where every metric is averaged.

http_duration_seconds{quantile="0.95"} 5
http_duration_seconds_sum 15
http_duration_seconds_count 5

The fix will always perform sum for the sum & count yielding,

http_duration_seconds{quantile="0.95"} 5
http_duration_seconds_sum 30
http_duration_seconds_count 10

sum & count metrics of summary & histogram should always be summed irrespective of the custom aggregator
fix: always use sum aggregator for sum & count
@raghuram-periaswamy
Copy link
Author

@SimenB @zbjornson Requesting for the review.

Comment on lines +87 to +95
// Should always `sum` if it's `sum` or `count` that's part of histogram & summary
if (
values[0].metricName === `${result.name}_sum` ||
values[0].metricName === `${result.name}_count`
) {
valObj.value = sum(values);
} else {
valObj.value = aggregatorFn(values);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good find, thanks and sorry for the year-long delay reviewing it.

I think this can wrongly identify custom metrics that are happen to be named something_sum/something_count though. Can you use the type instead? (metrics[0].type)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants