-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
sparse buckets: Fix handling of +Inf/-Inf/NaN observations #1144
Conversation
11f983a
to
3a8e28f
Compare
Corresponding changes in prometheus/prometheus: prometheus/prometheus#11418 |
Will fix tests momentarily… |
3a8e28f
to
579eca4
Compare
NaN observations now go to no bucket, but increment count (and effectively set sum to NaN, too). ±Inf observations now go to the bucket following the bucket that would have received math.MaxFloat64. The former is now the last bucket that can be created. The getLe is modified to return math.MaxFloat64 for the penultimate possible bucket. Also add a test for getLe. Signed-off-by: beorn7 <beorn@grafana.com>
579eca4
to
6942f9e
Compare
@fstab @codesome with prometheus/prometheus#11418 already reviewed, this one should be easy. Could you have a look? |
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.
Now that I am thinking more clearly, I just have a bunch of questions.
@@ -548,13 +548,13 @@ func TestSparseHistogram(t *testing.T) { | |||
name: "+Inf observation", | |||
observations: []float64{0, 1, 1.2, 1.4, 1.8, 2, math.Inf(+1)}, | |||
factor: 1.2, | |||
want: `sample_count:7 sample_sum:inf schema:2 zero_threshold:2.938735877055719e-39 zero_count:1 positive_span:<offset:0 length:5 > positive_span:<offset:2147483642 length:1 > positive_delta:1 positive_delta:-1 positive_delta:2 positive_delta:-2 positive_delta:2 positive_delta:-1 `, | |||
want: `sample_count:7 sample_sum:inf schema:2 zero_threshold:2.938735877055719e-39 zero_count:1 positive_span:<offset:0 length:5 > positive_span:<offset:4092 length:1 > positive_delta:1 positive_delta:-1 positive_delta:2 positive_delta:-2 positive_delta:2 positive_delta:-1 `, |
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.
IIUC, the last span corresponds to a bucket ID 4093 where le will be +Inf. And the last non-inf bucket is 4092 whose le is MaxFloat64?
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.
Not quite.
The last regular bucket has ID 4096, and the +Inf bucket has ID 4097. Here we have two spans. The first spans starts at ID 0 and has a length of 5. The second span's offset is 4092, hence the ID of the first bucket in the second span is 5+4092=4097 (i.e. the Inf bucket).
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.
has a length of 5
🤦 I totally missed this part when doing the math, my bad
}, | ||
{ | ||
name: "-Inf observation", | ||
observations: []float64{0, 1, 1.2, 1.4, 1.8, 2, math.Inf(-1)}, | ||
factor: 1.2, | ||
want: `sample_count:7 sample_sum:-inf schema:2 zero_threshold:2.938735877055719e-39 zero_count:1 negative_span:<offset:2147483647 length:1 > negative_delta:1 positive_span:<offset:0 length:5 > positive_delta:1 positive_delta:-1 positive_delta:2 positive_delta:-2 positive_delta:2 `, | ||
want: `sample_count:7 sample_sum:-inf schema:2 zero_threshold:2.938735877055719e-39 zero_count:1 negative_span:<offset:4097 length:1 > negative_delta:1 positive_span:<offset:0 length:5 > positive_delta:1 positive_delta:-1 positive_delta:2 positive_delta:-2 positive_delta:2 `, |
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.
Similarly here bucket 4097 as le (or ge here?) of -Inf, while 4096 will have -MaxFloat64? I don't know if I understand why the IDs differ here compared to +Inf above, some signed bit thing?
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.
See explanation above. Here the Inf observation is negative, so it's the only negative observation, we get only one span there, and its offset is therefore 4097 directly.
{ | ||
key: 4096, | ||
schema: 2, | ||
want: math.MaxFloat64, | ||
}, | ||
{ | ||
key: 4097, | ||
schema: 2, | ||
want: math.Inf(+1), | ||
}, |
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.
I am a little confused about 4092 and 4097 from above. How would 4092 fit in this unit test?
@@ -1346,13 +1357,55 @@ func findSmallestKey(m *sync.Map) int { | |||
} | |||
|
|||
func getLe(key int, schema int32) float64 { |
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.
IIUC, we are having this special case because the last bucket overlaps the limitation of the float64? Like the lower bound is less than MaxFloat64 but the upper bound crosses MaxFloat64, hence we do special casing? And since we cannot give a number for a bucket beyond that, we just give it to Inf observation?
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.
Yes, that's the idea.
This is only for the sparsehistogram branch.
Next attempt to solve ±Inf observations.
NaN observations now go to no bucket, but increment count (and effectively set sum to NaN, too).
±Inf observations now go to the bucket following the bucket that would have received math.MaxFloat64. The former is now the last bucket that can be created.
The getLe is modified to return math.MaxFloat64 for the penultimate possible bucket.
Also add a test for getLe.
Signed-off-by: beorn7 beorn@grafana.com