fix: always use sum aggregator for sum & count#487
Open
raghuram-periaswamy wants to merge 3 commits into
Open
Conversation
sum & count metrics of summary & histogram should always be summed irrespective of the custom aggregator
fix: always use sum aggregator for sum & count
Author
|
@SimenB @zbjornson Requesting for the review. |
zbjornson
suggested changes
Mar 8, 2023
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); | ||
| } |
Contributor
There was a problem hiding this comment.
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
set-2
using
averageyields the following where every metric isaveraged.The fix will always perform
sumfor thesum&countyielding,