fix(promql): histogram_quantile and histogram_fraction NaN observed in native histogram #16724
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e276fd7 to
abef671
Compare
krajorama
added a commit
to prometheus/docs
that referenced
this pull request
Jun 13, 2025
Fix TODO. Related to prometheus/prometheus#16578 Related to prometheus/prometheus#16724 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
abef671 to
0553341
Compare
4641db0 to
3ff6dce
Compare
krajorama
added a commit
to prometheus/docs
that referenced
this pull request
Jun 13, 2025
Fix TODO. 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
added a commit
to prometheus/docs
that referenced
this pull request
Jun 13, 2025
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>
Fixes: #16578 See the issue for detailed explanation. When a histogram had only NaN observations and no normal observations, we returned 0 from the quantile, which is completely wrong. If there were normal observations but we went over them, we returned the upper bound of the existing buckets, however that contradicts expectations on histogram_fraction. Now we return NaN if the quantile is calculated to be over all normal observations, falling into NaNs (in a virtual +Inf bucket). We also return info level annotations if we see any NaN observations. The annotation calls out if we returned NaN or even if we took the virtual +Inf bucket into account. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Fixes: #16580 According to the specification we should not take NaN observations into account when calculating the fraction. This commit fixes that and adds an info level annotation to let the user know about this. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
3ff6dce to
61bb15a
Compare
krajorama
added a commit
to prometheus/docs
that referenced
this pull request
Jun 13, 2025
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>
carrieedwards
approved these changes
Jun 15, 2025
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7
requested changes
Jun 18, 2025
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7
requested changes
Jun 24, 2025
There was a problem hiding this comment.
Yes, let's keep the old behavior for histograms where the sum is not NaN and only do the NaN return for histograms where the sum is NaN.
The histogram could just be in breach of the spec, of course, but then it is still a defined behavior to return bucket.Upper as we have don so far anyway.
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7
approved these changes
Jun 25, 2025
krajorama
added a commit
to prometheus/docs
that referenced
this pull request
Jun 25, 2025
…2676) * fix(nh-spec): update histogram_quantile and histogram_fraction docs 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #16578
See the issue for detailed explanation.
When a histogram had only NaN observations and no normal observations,
we returned 0 from the quantile, which is completely wrong. If there were
normal observations but we went over them, we returned the upper bound of
the existing buckets, however that contradicts expectations on
histogram_fraction. Now we return NaN if the quantile is calculated to be
over all normal observations, falling into NaNs (in a virtual +Inf bucket).
We also return info level annotations if we see any NaN observations.
The annotation calls out if we returned NaN or even if we took the
virtual +Inf bucket into account.
Fixes: #16580
According to the specification we should not take NaN observations
into account when calculating the fraction. This commit fixes that
and adds an info level annotation to let the user know about this.