-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Fix NaN checks in [Float]Histogram.Equals method #12891
Conversation
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
Not really sure about this test failure on Windows in CI, I cannot reproduce it locally (on Mac), could it be a flaky test?
|
That's a good point. The sum can actually be a non-stale NaN (after an observation of NaN), and the question is if we then want to consider those equal to other non-stale NaNs… I'll think about it. And yeah, MS Windows tests have a history of being flaky. |
OK, so currently we only use the We do not yet implement the Another point is if we need to give the same treatment to other floats in the histograms. We have implemented the division operator by now, so you could divide a histogram by zero, and then a Soooo, let's say the same treatment should also apply to |
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
af45b5b
to
3c047a3
Compare
@beorn7 I have made the changes based on your suggestion:
Also, what about comparing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the long review backlog.
I have thought about the buckets. Initially, I thought we can avoid the comparison by making sure that buckets get removed after a division by zero. However, there are weird edge cases where a NaN
bucket value is more or less legitimate. (The one I could come up with: A bucket count could overflow, resulting in +Inf. Then we could multiply this histogram by -1, so the bucket count is -Inf, and finally add the result to the original histogram, resulting in a bucket count of NaN. Obviously, these cases are all pretty academic because a histogram with such an overflow will be mostly useless, but we still have to handle it correctly.)
So I guess we have to do the following: For a float histogram, we also have to compare bucket counts based on their bit pattern. For an integer histogram, nothing changes. Sadly, that mean that the generic bucketsMatch
helper function has to be manually specialized for float and ints.
Does this make sense?
model/histogram/float_histogram.go
Outdated
@@ -307,13 +307,16 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { | |||
// Exact match is when there are no new buckets (even empty) and no missing buckets, | |||
// and all the bucket values match. Spans can have different empty length spans in between, | |||
// but they must represent the same bucket layout to match. | |||
// Non-metadata float fields (Sum, Count, ZeroCount) are compared as binary (using math.Float64bits). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Metadata" is a highly overloaded word.
Suggestion: "Sum, Count, and ZeroCount are compared based on their bit patterns because this method is about data equality rather than mathematical equality."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comments (agree on "metadata").
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
d7e2818
to
ec823d9
Compare
Makes sense. Updated float bucket comparisons to bitwise. Only had to sepcialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Only one tiny comment amendment left.
model/histogram/float_histogram.go
Outdated
@@ -307,13 +307,17 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { | |||
// Exact match is when there are no new buckets (even empty) and no missing buckets, | |||
// and all the bucket values match. Spans can have different empty length spans in between, | |||
// but they must represent the same bucket layout to match. | |||
// Sum, Count, and ZeroCount are compared based on their bit patterns because this method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buckets need to be mentioned here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I always forget those comments. Updated.
Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>
f6b3390
to
62bbb81
Compare
@beorn7 one more thing - at some point I noticed that |
You are right. The |
Added a check for I believe doc comment update is not necessary for |
Oh, now we are getting this failure, and apparently there is no infrastructure (parser?) to set up an expected value for
|
Hmm, yeah, good point. We are really getting deep into the rabbit hole here. Here are my (rabbit hole deep) thoughts: This So after all, let's just ignore the reset hint for now. |
22240ce
to
62bbb81
Compare
Agreed, reverted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
This PR modifies
[Float]Histogram.Equals
method to compareSum
fields at binary level (usingmath.Float64bits
) which would recognize identicalNaN
(among them, staleness markers) values as equal, to make native histogram equality checks consistent with plain (non-histogram) sample equality check:prometheus/tsdb/head_append.go
Lines 406 to 415 in 6dcbd65
Also (for
FloatHistogram
)Count
,ZeroCount
and bucket values are now compared at binary level.