From 432723552bcdcab4bf5b7637c5e0583b79c9b627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Thu, 26 Jan 2023 12:32:39 +0100 Subject: [PATCH 1/5] Make it more efficient and compatible to use SampleHistogram MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- model/value.go | 4 ++-- model/value_histogram.go | 4 ++-- model/value_histogram_test.go | 17 ++++++----------- model/value_test.go | 10 +++++----- 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/model/value.go b/model/value.go index 543399e3..6b2842d0 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 150c6db5..1b1b1886 100644 --- a/model/value_histogram.go +++ b/model/value_histogram.go @@ -135,7 +135,7 @@ func (s *SampleHistogram) Equal(o *SampleHistogram) bool { type SampleHistogramPair struct { Timestamp Time - Histogram SampleHistogram + Histogram *SampleHistogram } func (s SampleHistogramPair) MarshalJSON() ([]byte, error) { @@ -167,5 +167,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)) } diff --git a/model/value_histogram_test.go b/model/value_histogram_test.go index 4b7d45ca..dd86d759 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 @@ -218,7 +213,7 @@ func TestSampleHistogramJSON(t *testing.T) { Metric: Metric{ MetricNameLabel: "test_metric", }, - Histogram: genSampleHistogramPtr(), + Histogram: genSampleHistogram(), Timestamp: 1234567, }, }, @@ -312,7 +307,7 @@ func TestVectorHistogramJSON(t *testing.T) { Metric: Metric{ MetricNameLabel: "test_metric", }, - Histogram: genSampleHistogramPtr(), + Histogram: genSampleHistogram(), Timestamp: 1234567, }}, }, @@ -424,14 +419,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 b120264b..1eeae338 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, }, From 3520916459aedd4be60b64d3f3377a0dcfbc5345 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Thu, 26 Jan 2023 13:36:11 +0100 Subject: [PATCH 2/5] Address review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- model/value_histogram.go | 4 ++++ model/value_histogram_test.go | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/model/value_histogram.go b/model/value_histogram.go index 1b1b1886..69243d86 100644 --- a/model/value_histogram.go +++ b/model/value_histogram.go @@ -135,6 +135,7 @@ func (s *SampleHistogram) Equal(o *SampleHistogram) bool { type SampleHistogramPair struct { Timestamp Time + // Histogram should never bill nil, it's only stored as pointer for efficiency Histogram *SampleHistogram } @@ -143,6 +144,9 @@ func (s SampleHistogramPair) MarshalJSON() ([]byte, error) { if err != nil { return nil, err } + if s.Histogram == nil { + return nil, fmt.Errorf("SampleHistogramPair.MarshalJSON: cannot marshal nil Histogram") + } v, err := json.Marshal(s.Histogram) if err != nil { return nil, err diff --git a/model/value_histogram_test.go b/model/value_histogram_test.go index dd86d759..3d2939bd 100644 --- a/model/value_histogram_test.go +++ b/model/value_histogram_test.go @@ -153,6 +153,17 @@ 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)) + } +} + func TestSampleHistogramJSON(t *testing.T) { input := []struct { plain string From 0e2c72554e00d244528088d0703bbee19a76fc2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Thu, 26 Jan 2023 13:43:33 +0100 Subject: [PATCH 3/5] Check unmarshal for errors as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- model/value_histogram.go | 3 +++ model/value_histogram_test.go | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/model/value_histogram.go b/model/value_histogram.go index 69243d86..adf788d3 100644 --- a/model/value_histogram.go +++ b/model/value_histogram.go @@ -163,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 } diff --git a/model/value_histogram_test.go b/model/value_histogram_test.go index 3d2939bd..db75630d 100644 --- a/model/value_histogram_test.go +++ b/model/value_histogram_test.go @@ -162,6 +162,13 @@ func TestInvalidSampleHistogramPairJSON(t *testing.T) { 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) { From 3a39cabb26c60bd88e93993b2b393d96c8eb7e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Thu, 26 Jan 2023 13:44:56 +0100 Subject: [PATCH 4/5] simpler wording in error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- model/value_histogram.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/value_histogram.go b/model/value_histogram.go index adf788d3..cfb818a2 100644 --- a/model/value_histogram.go +++ b/model/value_histogram.go @@ -145,7 +145,7 @@ func (s SampleHistogramPair) MarshalJSON() ([]byte, error) { return nil, err } if s.Histogram == nil { - return nil, fmt.Errorf("SampleHistogramPair.MarshalJSON: cannot marshal nil Histogram") + return nil, fmt.Errorf("histogram is nil") } v, err := json.Marshal(s.Histogram) if err != nil { From fd1c4017053d205d26d15a4f8e270b53c6e5ad7a Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Thu, 26 Jan 2023 15:02:54 +0100 Subject: [PATCH 5/5] Update model/value_histogram.go Co-authored-by: Ganesh Vernekar Signed-off-by: George Krajcsovits --- model/value_histogram.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/value_histogram.go b/model/value_histogram.go index cfb818a2..d20b1b74 100644 --- a/model/value_histogram.go +++ b/model/value_histogram.go @@ -135,7 +135,7 @@ func (s *SampleHistogram) Equal(o *SampleHistogram) bool { type SampleHistogramPair struct { Timestamp Time - // Histogram should never bill nil, it's only stored as pointer for efficiency + // Histogram should never be nil, it's only stored as pointer for efficiency. Histogram *SampleHistogram }