Skip to content

Conversation

harry671003
Copy link
Contributor

@harry671003 harry671003 commented Jun 17, 2025

Found a bug in last_over_time for native histogram samples. When handling mixed samples, the function should return the last float sample. However, if there are no float samples and the histogram sample timestamps are less than 0, it incorrectly returns a float with the default value T=0, V=0.0.

Adding the check len(el.Floats) > 0 ensures the function only returns a float when one is actually present in the input.

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
@beorn7 beorn7 requested review from krajorama and beorn7 and removed request for roidelapluie June 20, 2025 16:06
@harry671003 harry671003 changed the title PromQL: Fix native histogram last_over_time with offset PromQL: Fix native histogram last_over_time Jun 20, 2025
@NeerajGartia21
Copy link
Contributor

NeerajGartia21 commented Jun 20, 2025

Thanks for the PR @harry671003 . The code looks good to me. Just a small suggestion: the last_over_time function doesn't explicitly handle the case where there are no samples at all. While this doesn't cause test failures due to earlier checks, adding an explicit condition for empty input could improve readability and make the logic more self-contained. WDYT?

@beorn7
Copy link
Member

beorn7 commented Jun 22, 2025

Thanks for the PR @harry671003 . The code looks good to me. Just a small suggestion: the last_over_time function doesn't explicitly handle the case where there are no samples at all. While this doesn't cause test failures due to earlier checks, adding an explicit condition for empty input could improve readability and make the logic more self-contained. WDYT?

If there aren't any samples at all, the code in question isn't even reached because the range selector doesn't select anything. Right?

In different news, the case that triggered the problem does have samples in the range, namely histograms with a negative timestamp.

Maybe I'm missing something and there is room for improvement, but let's first merge the solution as is to stop the bleeding.

@beorn7 beorn7 merged commit 8ed37d3 into prometheus:main Jun 22, 2025
27 checks passed
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.

4 participants