Make it more efficient and compatible to use SampleHistogram (#439)

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 <[email protected]>
Signed-off-by: George Krajcsovits <[email protected]>
Co-authored-by: Ganesh Vernekar <[email protected]>

Author: krajorama

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)

model/value_histogram.go

@@ -135,14 +135,18 @@ 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) {
 	t, err := json.Marshal(s.Timestamp)
 	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))
 }

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,
 				},
 			},

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,
 		},