Skip to content

promql(NHCB): histogram_stddev and histogram_stdvar should use arithmetic mean #16444

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Apr 24, 2025
Merged

promql(NHCB): histogram_stddev and histogram_stdvar should use arithmetic mean #16444

merged 13 commits into from
Apr 24, 2025

Conversation

amanycodes
Copy link
Contributor

fixes: #16439
I will update the histograms.test and native_histograms.test to now use the Arithmetic Mean logic for NHCBs.

@amanycodes amanycodes requested a review from juliusv as a code owner April 17, 2025 12:10
@juliusv juliusv requested a review from beorn7 April 17, 2025 14:26
sujalshah-bit
sujalshah-bit reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@sujalshah-bit sujalshah-bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work :)

@beorn7
Copy link
Member

beorn7 commented Apr 17, 2025

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.

@amanycodes
Copy link
Contributor Author

amanycodes commented Apr 17, 2025 •
edited
Loading

@beorn7 Now the stddev and stdvar methods use arithmetic mean only for NHCB bucket (i.e. sample.H.Schema = -53).
Apologies for the noise.

NeerajGartia21
NeerajGartia21 reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@NeerajGartia21 NeerajGartia21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've added some comments and suggestions.

@amanycodes
Copy link
Contributor Author

@NeerajGartia21, Thanks for the review! Please let me know if it looks good or needs any changes.

NeerajGartia21
NeerajGartia21 reviewed Apr 18, 2025
View reviewed changes
Copy link
Contributor

@NeerajGartia21 NeerajGartia21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. I’ve added another suggestion regarding code structure that could help improve readability.

@NeerajGartia21
Copy link
Contributor

Since #16451 has been merged, @amanycodes, please rebase this.

@amanycodes
histogram_stddev and histogram_stdvar now use AM for NHCB
00833d4 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
updated unit tests for native histograms
735f134 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
updated docs
e8434c2 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
Rebuild
bc9ec87 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
updated logic and tests to use arithmetic mean just for NHCB
f1d7904 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
fixed CI and unit tests
d90beb4 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
fixes
68e7564 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
CI Fix
3bdff22 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
fixed tests and logic
1909516 
Signed-off-by: amanycodes <amanycodes@gmail.com>
@amanycodes
Copy link
Contributor Author

I have rebased it with the refactoring @NeerajGartia21 mentioned for better readability. Please let me know if it's fine!

@NeerajGartia21
Copy link
Contributor

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
beorn7 requested changes Apr 22, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much everyone.

I would like to propose a bit of wordsmithing for the documentation, and have one additional coding style note.

@beorn7
Copy link
Member

beorn7 commented Apr 22, 2025 •
edited
Loading

Also, with #16451 in, could you rebase this to make sure we don't get subtle errors? There is no direct merge conflict, but the resulting code might still be messed up in subtle ways.

Already done, sorry for the noise.

@beorn7 beorn7 mentioned this pull request Apr 22, 2025
Merged
amanycodes and others added 2 commits April 22, 2025 18:58
@amanycodes
Merge branch 'prometheus:main' into promql-nhcb-use-AM
c792987
@amanycodes
more fixes
6d68bca 
Signed-off-by: amanycodes <amanycodes@gmail.com>
beorn7
beorn7 requested changes Apr 22, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there, one more thing…

NeerajGartia21
NeerajGartia21 reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@NeerajGartia21 NeerajGartia21 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@beorn7 beorn7 mentioned this pull request Apr 23, 2025
Closed
amanycodes and others added 2 commits April 23, 2025 22:48
@amanycodes
Copy link
Contributor Author

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
beorn7 approved these changes Apr 24, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@beorn7 beorn7 merged commit 26bddcf into prometheus:main Apr 24, 2025
27 checks passed
@charleskorn charleskorn mentioned this pull request May 1, 2025
Merged
charleskorn added a commit to grafana/mimir that referenced this pull request May 2, 2025
@charleskorn
Modify histogram_stddev and histogram_stdvar to match new behavio…
5e24596 
…ur introduced in prometheus/prometheus#16444
charleskorn added a commit to grafana/mimir that referenced this pull request May 16, 2025
@charleskorn
Modify histogram_stddev and histogram_stdvar to match new behavio…
5962f02 
…ur introduced in prometheus/prometheus#16444
charleskorn added a commit to grafana/mimir that referenced this pull request May 16, 2025
@charleskorn
Upgrade mimir-prometheus (#11362)
f2d8b55 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@roidelapluie roidelapluie Awaiting requested review from roidelapluie roidelapluie is a code owner

@juliusv juliusv Awaiting requested review from juliusv juliusv is a code owner

+2 more reviewers

@sujalshah-bit sujalshah-bit sujalshah-bit left review comments

@NeerajGartia21 NeerajGartia21 NeerajGartia21 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

promql(NHCB): histogram_stddev and histogram_stdvar should use arithmetic mean for NHCB
4 participants
@amanycodes @beorn7 @NeerajGartia21 @sujalshah-bit