-
Notifications
You must be signed in to change notification settings - Fork 378
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
Histogram zero fix #803
Histogram zero fix #803
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Thanks for the change and the detailed explanation. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
==========================================
- Coverage 57.20% 56.76% -0.44%
==========================================
Files 66 66
Lines 6928 6923 -5
==========================================
- Hits 3963 3930 -33
- Misses 2696 2724 +28
Partials 269 269 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
Benchmark ResultBenchmark diff with base
|
Currently, mtail checks both lower and upper bound when searching for a histogram bucket. This is not only unnecessary (because buckets are constructed so that they are sorted and continuous), it leads to bugs like #226 (fixed in the past), #675 (still open) and the fact that negative values will not be counted in any bucket unless you explicitly add negative bucket that is lower than any value that will be observed (which is not a problem for timings, but histograms could be used for other things).
Behavior of other prometheus clients is to look for first bucket where observed value less or equal to its upper bound. This MR makes mtail to also behave this way, which ensures any observation will always be counted in some bucket. I also changed integration test to check observations of zero are counted and simplified the histogram bucket initialization a bit.