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

In histogram_quantile merge buckets with equivalent le values #5158

Merged
merged 1 commit into from Feb 1, 2019

Conversation

Projects
None yet
3 participants
@brian-brazil
Copy link
Member

brian-brazil commented Jan 30, 2019

This makes things generally more resilient, and will
help with OpenMetrics transitions (and inconsistencies).

Signed-off-by: Brian Brazil brian.brazil@robustperception.io

@@ -83,6 +83,7 @@ func bucketQuantile(q float64, buckets buckets) float64 {
return math.NaN()
}

buckets = coalesceBuckets(buckets)

This comment has been minimized.

Copy link
@juliusv

juliusv Jan 30, 2019

Member

This is done after the len(buckets) > 2 check, but coalesceBuckets() may remove buckets, causing the number of buckets to be <2 again. It seems due to the extra +Inf check, this can only happen if the input bucket list is an edge case like this:

["+inf", "+Inf"]

Not sure if the following code then actually has problems with this, but adding a test case for this would be good.

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Jan 31, 2019

Author Member

Good point, fixed.

In histogram_quantile merge buckets with equivalent le values
This makes things generally more resilient, and will
help with OpenMetrics transitions (and inconsistencies).

Signed-off-by: Brian Brazil <brian.brazil@robustperception.io>

@brian-brazil brian-brazil force-pushed the histo-rep branch from 6acdad9 to 64b3801 Jan 31, 2019

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jan 31, 2019

👍

@brian-brazil brian-brazil merged commit c66aeb3 into master Feb 1, 2019

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brian-brazil brian-brazil deleted the histo-rep branch Feb 1, 2019

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Feb 4, 2019

Academic note: This PR assumes that sum is the only reasonably aggregation to be done before feeding the histogram to histogram_quantile (as it sums up equivalent buckets). I think that's a reasonable assumption, but it raises the question why histogram_quantile isn't an aggregation operator itself, sparing users the effort to sum explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.