ValidateHistogram: strict Count check in absence of NaNs #12977

linasm · 2023-10-13T07:00:12Z

This change was inspired by this comment: #12739 (comment)

Prior to this change, a non-strict (>) validation of Histogram.Count had to be done to accommodate edge cases resulting from NaN observations.

After this change, those edge cases will be detected by checking Sum for being NaN, and if it is not, then a strict (!=) validation of Histogram.Count will be performed instead. I assume this makes sense (but really open to feedback in case I have missed something).

Note that I also took the liberty to move the validation code from tsdb to model/histogram package. IMHO validation code being next to the domain object that it validates makes it easier to discover and use (avoiding extra package dependency). Also, consistency: I see that some other domain objects (Labels, RuleNode) have their validation functions under model package. I did this move in a separate commit to make the actual logic change easier to review (also, easy to revert if I'm suggested that this move is not a good idea for some reason, or suggested to separate it out).

linasm · 2023-10-13T07:01:15Z

model/histogram/test_utils.go

+package histogram
+
+// GenerateBigTestHistograms generates a slice of histograms with given number of buckets each.
+func GenerateBigTestHistograms(numHistograms, numBuckets int) []*Histogram {


Moved from tsdb/head_test.go

linasm · 2023-10-13T07:01:48Z

model/histogram/validate.go

+	ErrHistogramCountNotBigEnough    = errors.New("histogram's observation count should be at least the number of observations found in the buckets")
+	ErrHistogramCountMismatch        = errors.New("histogram's observation count should equal the number of observations found in the buckets (in absence of NaN)")
+	ErrHistogramNegativeBucketCount  = errors.New("histogram has a bucket whose observation count is negative")
+	ErrHistogramSpanNegativeOffset   = errors.New("histogram has a span whose offset is negative")
+	ErrHistogramSpansBucketsMismatch = errors.New("histogram spans specify different number of buckets than provided")


Moved from storage/interface.go to avoid dependency cycle.

linasm · 2023-10-13T07:02:22Z

model/histogram/validate.go

+	ErrHistogramSpansBucketsMismatch = errors.New("histogram spans specify different number of buckets than provided")
+)
+
+func ValidateHistogram(h *Histogram) error {


Moved from tsdb/head_append.go

Maybe this could be a method of Histogram now, just called Validate?

Would also be good to give it a doc comment to explain what is validated.

Sure, makes sense. Changed from func ValidateHistogram(h *Histogram) error to func (h *Histogram) Validate() error and added doc comments.

linasm · 2023-10-13T07:02:27Z

model/histogram/validate.go

+	return nil
+}
+
+func ValidateFloatHistogram(h *FloatHistogram) error {


Moved from tsdb/head_append.go

Corresponding comment here:

Maybe this could be a method of FloatHistogram now, just called Validate?

Would also be good to give it a doc comment to explain what is validated.

Same here - changed from func ValidateFloatHistogram(h *FloatHistogram) error to func (h *FloatHistogram) Validate() error and added doc comments.

linasm · 2023-10-13T07:02:41Z

model/histogram/validate.go

+	return nil
+}
+
+func checkHistogramSpans(spans []Span, numBuckets int) error {


Moved from tsdb/head_append.go

linasm · 2023-10-13T07:05:04Z

tsdb/head_append.go

-	return nil
-}
-
-func checkHistogramBuckets[BC histogram.BucketCount, IBC histogram.InternalBucketCount](buckets []IBC, count *BC, deltas bool) error {


Moved to model/histogram/validate.go

linasm · 2023-10-13T07:05:15Z

tsdb/head_test.go

@@ -4827,170 +4823,6 @@ func TestReplayAfterMmapReplayError(t *testing.T) {
 	require.NoError(t, h.Close())
 }

-func TestHistogramValidation(t *testing.T) {


Moved to model/histogram/validate_test.go

linasm · 2023-10-13T07:05:27Z

tsdb/head_test.go

-	}
-}
-
-func BenchmarkHistogramValidation(b *testing.B) {


Moved to model/histogram/validate_test.go

linasm · 2023-10-13T07:05:37Z

tsdb/head_test.go

-	}
-}
-
-func generateBigTestHistograms(numHistograms, numBuckets int) []*histogram.Histogram {


Moved to model/histogram/test_utils.go

linasm · 2023-10-13T07:08:42Z

tsdb/head_test.go

-				{30, 696},
-				{30, 700},
-				{30, 708},
-				{30, 693},
+				{40, 896},
+				{40, 899},
+				{40, 896},
+				{30, 690},
 				{30, 691},
-				{30, 692},
-				{30, 695},
 				{30, 694},
+				{30, 693},


Looks like these changes in the encoded chunks were triggered by fixing how the observationCount is calculated when generating those synthetic histograms:
2acb51e#diff-7c1dfea5c568897587621510c04d322f908a8ed40751e592d63d2f8422b6a78dL4961-R4969

beorn7 · 2023-10-24T11:38:32Z

On my list. Sorry for not noticing this is the last 2 weeks.

beorn7

Looks good. Just an idea about making the validation function methods. Do you think that would make sense?

beorn7 · 2023-11-01T17:46:29Z

model/histogram/validate.go

+	ErrHistogramSpansBucketsMismatch = errors.New("histogram spans specify different number of buckets than provided")
+)
+
+func ValidateHistogram(h *Histogram) error {


Maybe this could be a method of Histogram now, just called Validate?

Would also be good to give it a doc comment to explain what is validated.

beorn7 · 2023-11-01T17:47:50Z

model/histogram/validate.go

+	return nil
+}
+
+func ValidateFloatHistogram(h *FloatHistogram) error {


Corresponding comment here:

Maybe this could be a method of FloatHistogram now, just called Validate?

Would also be good to give it a doc comment to explain what is validated.

Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>

beorn7

Thank you very much.

linasm requested review from roidelapluie and jesusvazquez as code owners October 13, 2023 07:00

linasm commented Oct 13, 2023

View reviewed changes

linasm force-pushed the linasm/strict-validation-of-histogram-without-nans branch 4 times, most recently from 5d23a1d to de67dde Compare October 13, 2023 07:58

beorn7 requested review from beorn7 and removed request for roidelapluie and jesusvazquez October 24, 2023 11:36

beorn7 requested changes Nov 1, 2023

View reviewed changes

linasm added 2 commits November 3, 2023 16:17

ValidateHistogram: strict Count check in absence of NaNs

1cd6c1c

Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>

Move histogram validation code to model/histogram

1f8aea1

Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>

linasm force-pushed the linasm/strict-validation-of-histogram-without-nans branch from de67dde to 184d142 Compare November 3, 2023 14:44

Change Validate to be a method on histogram structs

ebed7d0

Signed-off-by: Linas Medziunas <linas.medziunas@gmail.com>

linasm force-pushed the linasm/strict-validation-of-histogram-without-nans branch from 184d142 to ebed7d0 Compare November 3, 2023 14:48

linasm requested a review from beorn7 November 3, 2023 15:32

beorn7 approved these changes Nov 5, 2023

View reviewed changes

beorn7 merged commit 69c9c29 into prometheus:main Nov 5, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ValidateHistogram: strict Count check in absence of NaNs #12977

ValidateHistogram: strict Count check in absence of NaNs #12977

linasm commented Oct 13, 2023 •

edited

linasm Oct 13, 2023

linasm Oct 13, 2023

linasm Oct 13, 2023

beorn7 Nov 1, 2023

linasm Nov 3, 2023

linasm Oct 13, 2023 •

edited

beorn7 Nov 1, 2023

linasm Nov 3, 2023

linasm Oct 13, 2023

linasm Oct 13, 2023

linasm Oct 13, 2023

linasm Oct 13, 2023

linasm Oct 13, 2023 •

edited

linasm Oct 13, 2023

beorn7 commented Oct 24, 2023

beorn7 left a comment •

edited

beorn7 Nov 1, 2023

beorn7 Nov 1, 2023

beorn7 left a comment

ValidateHistogram: strict Count check in absence of NaNs #12977

ValidateHistogram: strict Count check in absence of NaNs #12977

Conversation

linasm commented Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 24, 2023

beorn7 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

linasm commented Oct 13, 2023 •

edited

linasm Oct 13, 2023 •

edited

linasm Oct 13, 2023 •

edited

beorn7 left a comment •

edited