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 off-by-one error in funcHistogramQuantile / ensureMonotonic #7393

Conversation

linasm
Copy link
Contributor

@linasm linasm commented Jun 14, 2020

Signed-off-by: Linas Medziunas linas.medziunas@gmail.com

Fixes an off-by-one error in ensureMonotonic (that is used in histogram quantiles calculation):

  • original implementation was unintentionally iterating from 0 to len(buckets) - 2, thus not fixing the last bucket value
  • fixed implementation iterates from 1 to len(buckets) - 1 as intended

@brian-brazil
Copy link
Contributor

Thanks, could we have tests around each of the non-monotonic buckets?

@roidelapluie
Copy link
Member

Working on tsdb test failure at #7394

@roidelapluie
Copy link
Member

When you add the tests Brian asked, would you please rebase on top of master? Thanks!

Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
@linasm linasm force-pushed the linasm/fix-off-by-one-in-histogram-quantile branch from 3fe2786 to b7d302e Compare June 15, 2020 06:53
@linasm
Copy link
Contributor Author

linasm commented Jun 15, 2020

@brian-brazil it is not possible to hit the non-monotonic buckets with binary search directly (because their value is made equal to the value of the preceding bucket). So I have altered the test data to alternate the buckets (increase -> decrease -> increase -> ...) and added test queries to hit every increase -> decrease pair.
Let me know if this is similar to what you were asking for (I'm not 100% sure).

@roidelapluie thanks for fixing the master so quickly.

@brian-brazil brian-brazil merged commit 7eaffa7 into prometheus:master Jun 15, 2020
@brian-brazil
Copy link
Contributor

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants