Query-tee: added a small tolerance to floating point sample values comparison #2994
Conversation
…mparison Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
|
|
||
| func compareSampleValue(first, second model.SampleValue, tolerance float64) bool { | ||
| if tolerance <= 0 { | ||
| return first == second |
There was a problem hiding this comment.
It's not safe to do equality comparisons on floats due to NaNs, use math.Float64bits
There was a problem hiding this comment.
Thanks for spotting it! The NaN comparison is also wrong when the tolerance is enabled (> 0).
There was a problem hiding this comment.
👍
We sometimes do this: https://github.com/prometheus/prometheus/blob/e6d7cc5fa43716281bdf5d1cf40415f99a0e9623/storage/series.go#L270 to avoid issues (so use Float compare indeed)
| if tolerance <= 0 { | ||
| return first == second | ||
| } else { | ||
| return math.Abs(float64(first)-float64(second)) <= tolerance |
There was a problem hiding this comment.
Tolerance is generally a ratio or number of decimal places, not a absolute difference.
There was a problem hiding this comment.
Yeah. You might want to look at https://github.com/beorn7/floats/blob/master/equal.go (shameless plug).
Signed-off-by: Marco Pracucci <[email protected]>
What this PR does:
While comparing query results between the Cortex chunks and blocks storage, we learned that floating point rounding could be different for the same exact query executed twice due to the non deterministic series ordering in PromQL engine (for more details please refer to prometheus/prometheus#7757).
This PR adds a small (configureable) tolerance to avoid such false positives.
Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]