max_over_time treats NaN as the max

[jacksontj]

Bug Report

What did you do?
a simple query like max_over_time(foo[30m])

What did you expect to see?
I expected to see the largest value

What did you see instead? Under which circumstances?
If there was a single NaN in the time range then the value returned is NaN. I made an example test case (which fails) to showcase the issue: jacksontj@a36f351

Environment

• Prometheus version:

  verified on 2.2, 2.3, and master

[brian-brazil]

Thanks for the report. We must have not fixed this for min/max while we were doing the others.

[jacksontj]

I can submit a PR with the test+fix. What is the desired behavior? Should NaNs be treated as the smallest thing? Or excluded from comparison? Personally seems like they should be excluded, but either way works for me.

[brian-brazil]

We should do the same as max does, and only return a NaN if there's only NaNs.

[jacksontj]

#4386 with the fix and some tests. I noticed that clamp_min and clamp_max still use the math.Max/Min functions, so presumably have similar issues.

[juliusv]

@jacksontj It's not clear to me what the right behavior of clamp_{min,max} would be in the face of NaNs, whether it should simply preserve NaNs or take the clamp value. Probably the current behavior is ok?

[brian-brazil]

I'm not sure what the right behaviour is there either, I'd say we wait until a use case comes up.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.