Skip to content

fix(nh-spec): update histogram_quantile and histogram_fraction docs #2676

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krajorama
Copy link
Member

@krajorama krajorama commented Jun 13, 2025

@krajorama krajorama marked this pull request as draft June 13, 2025 08:40
@krajorama krajorama force-pushed the krajo/nh-quantile-nan branch 2 times, most recently from c490d2a to 52b700e Compare June 13, 2025 09:37
@krajorama krajorama changed the title fix(nh-spec): update histogram_quantile to match implementation fix(nh-spec): update histogram_quantile and histogram_fraction docs Jun 13, 2025
@krajorama krajorama marked this pull request as ready for review June 13, 2025 09:39
Fix TODOs. And align with new implementation.
Related to prometheus/prometheus#16578
Related to prometheus/prometheus#16724
Related to prometheus/prometheus#16580

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama force-pushed the krajo/nh-quantile-nan branch from 52b700e to 7fe1fd3 Compare June 13, 2025 09:42
@krajorama krajorama requested review from beorn7 June 13, 2025 10:40
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. Only a punctuation nit.

with how classic histograms usually treat `NaN` observations (which end up in
the `+Inf` bucket in most implementations). If the quantile falls above all
existing buckets, we return `NaN` due to the fact that we want to be consistent
with `histogram_fraction`, since `histogram_fraction` considers `NaN`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe turn this comma into a semicolon or period? It's hard to dissect this sentence currently.

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.

3 participants