promql(NHCB): histogram_stddev and histogram_stdvar should use arithmetic mean#16444
promql(NHCB): histogram_stddev and histogram_stdvar should use arithmetic mean#16444beorn7 merged 13 commits intoprometheus:mainfrom
Conversation
67b939b to
b1e8593
Compare
sujalshah-bit
left a comment
There was a problem hiding this comment.
Thanks for the work :)
|
We want to keep the geometric mean for the (usual) case of exponential buckets. Only for NHCB buckets, we want to switch to the arithmetic mean. |
|
@beorn7 Now the stddev and stdvar methods use arithmetic mean only for NHCB bucket (i.e. |
NeerajGartia21
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've added some comments and suggestions.
|
@NeerajGartia21, Thanks for the review! Please let me know if it looks good or needs any changes. |
NeerajGartia21
left a comment
There was a problem hiding this comment.
Looks good to me overall. I’ve added another suggestion regarding code structure that could help improve readability.
|
Since #16451 has been merged, @amanycodes, please rebase this. |
Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
Signed-off-by: amanycodes <amanycodes@gmail.com>
0427277 to
3bdff22
Compare
Signed-off-by: amanycodes <amanycodes@gmail.com>
|
I have rebased it with the refactoring @NeerajGartia21 mentioned for better readability. Please let me know if it's fine! |
|
Looks good to me. Leaving final review to @beorn7. One small thing: since the logic for native histograms (without custom buckets) hasn’t changed, the test outputs ideally shouldn’t either. The current diffs seem to be just floating-point noise, so reverting them might help keep the PR clean and avoid confusion. |
beorn7
left a comment
There was a problem hiding this comment.
Thank you very much everyone.
I would like to propose a bit of wordsmithing for the documentation, and have one additional coding style note.
|
Already done, sorry for the noise. |
Signed-off-by: amanycodes <amanycodes@gmail.com>
There was a problem hiding this comment.
LGTM overall, aside from @beorn7's corrections, but there’s a small issue with the arithmetic mean formula. It was addressed in previous commits, but it seems to have been overlooked here. Please fix it. Also, a diff in native_histogram.test seems unnecessary and creates noise. I suggest reverting that as well.
Signed-off-by: amanycodes <amanycodes@gmail.com>
|
I did the changes and I hope it's okay this time :). I'll work on the bonus test and will add the test soon! |
beorn7
left a comment
There was a problem hiding this comment.
Thanks. I don't think we need the extra test because the existing testhistogram3 already has a bucket containing the zero point (and the zero point is not the arithmetic mean of that bucket). So we do test this case already.
* Upgrade mimir-prometheus * Fix breaking change in `NewAPI` * Sync upstream PromQL engine test cases * Modify `histogram_stddev` and `histogram_stdvar` to match new behaviour introduced in prometheus/prometheus#16444 * Simplify code * Remove outdated comment This issue was fixed in prometheus/prometheus#15828. * Remove unnecessary variable * Rename operator and files * Extract `histogram_quantile`-specific logic * Add support for classic histograms in `histogram_fraction` See prometheus/prometheus#16095 * Bring in changes from grafana/mimir-prometheus#871 * Invert build tag logic to match prometheus/prometheus#16436 * Sync query engine tests with upstream * Fix breaking changes * Regenerate OTLP code
fixes: #16439
I will update the
histograms.testandnative_histograms.testto now use the Arithmetic Mean logic for NHCBs.