Repository: prometheus/common
Pull request #439 (2023-01-26 11:33:43)

Make it more efficient and compatible to use SampleHistogram
Store SampleHistogram in SampleHistogramPair as pointer.
More compatible with what protobuf generates.
More efficient when taking the histogram from the pair.
Simpler test code.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>


Patch:
diff --git a/model/value.go b/model/value.go
index 543399e..6b2842d 100644
--- a/model/value.go
+++ b/model/value.go
@@ -63,7 +63,7 @@ func (s Sample) String() string {
 	if s.Histogram != nil {
 		return fmt.Sprintf("%s => %s", s.Metric, SampleHistogramPair{
 			Timestamp: s.Timestamp,
-			Histogram: *s.Histogram,
+			Histogram: s.Histogram,
 		})
 	}
 	return fmt.Sprintf("%s => %s", s.Metric, SamplePair{
@@ -82,7 +82,7 @@ func (s Sample) MarshalJSON() ([]byte, error) {
 			Metric: s.Metric,
 			Histogram: SampleHistogramPair{
 				Timestamp: s.Timestamp,
-				Histogram: *s.Histogram,
+				Histogram: s.Histogram,
 			},
 		}
 		return json.Marshal(&v)
diff --git a/model/value_histogram.go b/model/value_histogram.go
index 150c6db..d20b1b7 100644
--- a/model/value_histogram.go
+++ b/model/value_histogram.go
@@ -135,7 +135,8 @@ func (s *SampleHistogram) Equal(o *SampleHistogram) bool {
 
 type SampleHistogramPair struct {
 	Timestamp Time
-	Histogram SampleHistogram
+	// Histogram should never be nil, it's only stored as pointer for efficiency.
+	Histogram *SampleHistogram
 }
 
 func (s SampleHistogramPair) MarshalJSON() ([]byte, error) {
@@ -143,6 +144,9 @@ func (s SampleHistogramPair) MarshalJSON() ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
+	if s.Histogram == nil {
+		return nil, fmt.Errorf("histogram is nil")
+	}
 	v, err := json.Marshal(s.Histogram)
 	if err != nil {
 		return nil, err
@@ -159,6 +163,9 @@ func (s *SampleHistogramPair) UnmarshalJSON(buf []byte) error {
 	if gotLen := len(tmp); gotLen != wantLen {
 		return fmt.Errorf("wrong number of fields: %d != %d", gotLen, wantLen)
 	}
+	if s.Histogram == nil {
+		return fmt.Errorf("histogram is null")
+	}
 	return nil
 }
 
@@ -167,5 +174,5 @@ func (s SampleHistogramPair) String() string {
 }
 
 func (s *SampleHistogramPair) Equal(o *SampleHistogramPair) bool {
-	return s == o || (s.Histogram.Equal(&o.Histogram) && s.Timestamp.Equal(o.Timestamp))
+	return s == o || (s.Histogram.Equal(o.Histogram) && s.Timestamp.Equal(o.Timestamp))
 }


Test patch:
diff --git a/model/value_histogram_test.go b/model/value_histogram_test.go
index 4b7d45c..db75630 100644
--- a/model/value_histogram_test.go
+++ b/model/value_histogram_test.go
@@ -24,8 +24,8 @@ var (
 	noWhitespace = regexp.MustCompile(`\s`)
 )
 
-func genSampleHistogram() SampleHistogram {
-	return SampleHistogram{
+func genSampleHistogram() *SampleHistogram {
+	return &SampleHistogram{
 		Count: 6,
 		Sum:   3897,
 		Buckets: HistogramBuckets{
@@ -69,11 +69,6 @@ func genSampleHistogram() SampleHistogram {
 	}
 }
 
-func genSampleHistogramPtr() *SampleHistogram {
-	h := genSampleHistogram()
-	return &h
-}
-
 func TestSampleHistogramPairJSON(t *testing.T) {
 	input := []struct {
 		plain string
@@ -158,6 +153,24 @@ func TestSampleHistogramPairJSON(t *testing.T) {
 	}
 }
 
+func TestInvalidSampleHistogramPairJSON(t *testing.T) {
+	s1 := SampleHistogramPair{
+		Timestamp: 1,
+		Histogram: nil,
+	}
+	d, err := json.Marshal(s1)
+	if err == nil {
+		t.Errorf("expected error when trying to marshal invalid SampleHistogramPair %s", string(d))
+	}
+
+	var s2 SampleHistogramPair
+	plain := "[0.001,null]"
+	err = json.Unmarshal([]byte(plain), &s2)
+	if err == nil {
+		t.Errorf("expected error when trying to unmarshal invalid SampleHistogramPair %s", plain)
+	}
+}
+
 func TestSampleHistogramJSON(t *testing.T) {
 	input := []struct {
 		plain string
@@ -218,7 +231,7 @@ func TestSampleHistogramJSON(t *testing.T) {
 				Metric: Metric{
 					MetricNameLabel: "test_metric",
 				},
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 				Timestamp: 1234567,
 			},
 		},
@@ -312,7 +325,7 @@ func TestVectorHistogramJSON(t *testing.T) {
 				Metric: Metric{
 					MetricNameLabel: "test_metric",
 				},
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 				Timestamp: 1234567,
 			}},
 		},
@@ -424,14 +437,14 @@ func TestVectorHistogramJSON(t *testing.T) {
 					Metric: Metric{
 						MetricNameLabel: "test_metric",
 					},
-					Histogram: genSampleHistogramPtr(),
+					Histogram: genSampleHistogram(),
 					Timestamp: 1234567,
 				},
 				&Sample{
 					Metric: Metric{
 						"foo": "bar",
 					},
-					Histogram: genSampleHistogramPtr(),
+					Histogram: genSampleHistogram(),
 					Timestamp: 1234,
 				},
 			},
diff --git a/model/value_test.go b/model/value_test.go
index b120264..1eeae33 100644
--- a/model/value_test.go
+++ b/model/value_test.go
@@ -64,12 +64,12 @@ func TestEqualSamples(t *testing.T) {
 			in1: &Sample{
 				Metric:    Metric{"foo": "bar"},
 				Timestamp: 0,
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 			},
 			in2: &Sample{
 				Metric:    Metric{"foo": "bar"},
 				Timestamp: 0,
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 			},
 			want: true,
 		},
@@ -93,7 +93,7 @@ func TestEqualSamples(t *testing.T) {
 			in2: &Sample{
 				Metric:    Metric{"foo": "bar"},
 				Timestamp: 0,
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 			},
 			want: false,
 		},
@@ -117,7 +117,7 @@ func TestEqualSamples(t *testing.T) {
 			in2: &Sample{
 				Metric:    Metric{"foo": "bar"},
 				Timestamp: 0,
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 			},
 			want: false,
 		},
@@ -141,7 +141,7 @@ func TestEqualSamples(t *testing.T) {
 			in2: &Sample{
 				Metric:    Metric{"foo": "bar"},
 				Timestamp: 0,
-				Histogram: genSampleHistogramPtr(),
+				Histogram: genSampleHistogram(),
 			},
 			want: false,
 		},