From 1cfc8f458136e0941694a97557de5243c8873f4c Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 11 Jul 2018 18:04:39 +0000 Subject: [PATCH 1/2] Remove First and Last data members from Metrics structs They can be trivially computed when required. --- render/detailed/summary_test.go | 2 -- report/metric_row.go | 4 --- report/metrics.go | 48 ++++++++------------------------- report/metrics_internal_test.go | 29 -------------------- report/metrics_test.go | 24 ++++------------- 5 files changed, 16 insertions(+), 91 deletions(-) delete mode 100644 report/metrics_internal_test.go diff --git a/render/detailed/summary_test.go b/render/detailed/summary_test.go index f35fbf34f..49ace4683 100644 --- a/render/detailed/summary_test.go +++ b/render/detailed/summary_test.go @@ -84,8 +84,6 @@ func TestSummaries(t *testing.T) { Samples: nil, Min: metric.Min, Max: metric.Max, - First: metric.First, - Last: metric.Last, }, } if !reflect.DeepEqual(want, row) { diff --git a/report/metric_row.go b/report/metric_row.go index 2d61e0218..470491c1f 100644 --- a/report/metric_row.go +++ b/report/metric_row.go @@ -82,8 +82,6 @@ func (m *MetricRow) CodecEncodeSelf(encoder *codec.Encoder) { Samples: in.Samples, Min: in.Min, Max: in.Max, - First: in.First, - Last: in.Last, }) } @@ -95,8 +93,6 @@ func (m *MetricRow) CodecDecodeSelf(decoder *codec.Decoder) { Samples: in.Samples, Min: in.Min, Max: in.Max, - First: in.First, - Last: in.Last, } metric := w.FromIntermediate() *m = MetricRow{ diff --git a/report/metrics.go b/report/metrics.go index 2e1d1f7fd..1d8fe50ad 100644 --- a/report/metrics.go +++ b/report/metrics.go @@ -48,11 +48,17 @@ func (m Metrics) Copy() Metrics { // Metric is a list of timeseries data with some metadata. Clients must use the // Add method to add values. Metrics are immutable. type Metric struct { - Samples []Sample - Min, Max float64 - First, Last time.Time + Samples []Sample + Min, Max float64 } +// Following two functions are exported for testing only: make sure there are samples before calling. +// First gives the timestamp of the first sample +func (m Metric) First() time.Time { return m.Samples[0].Timestamp } + +// Last gives the timestamp of the last sample +func (m Metric) Last() time.Time { return m.Samples[len(m.Samples)-1].Timestamp } + // Sample is a single datapoint of a metric. type Sample struct { Timestamp time.Time `json:"date"` @@ -65,8 +71,6 @@ func MakeSingletonMetric(t time.Time, v float64) Metric { Samples: []Sample{{t, v}}, Min: v, Max: v, - First: t, - Last: t, } } @@ -97,8 +101,6 @@ func MakeMetric(samples []Sample) Metric { Samples: samples, Min: min, Max: max, - First: samples[0].Timestamp, - Last: samples[len(samples)-1].Timestamp, } } @@ -108,8 +110,6 @@ func (m Metric) WithMax(max float64) Metric { Samples: m.Samples, Max: max, Min: m.Min, - First: m.First, - Last: m.Last, } } @@ -118,20 +118,6 @@ func (m Metric) Len() int { return len(m.Samples) } -func first(t1, t2 time.Time) time.Time { - if t2.IsZero() || (!t1.IsZero() && t1.Before(t2)) { - return t1 - } - return t2 -} - -func last(t1, t2 time.Time) time.Time { - if t2.IsZero() || (!t1.IsZero() && t1.After(t2)) { - return t1 - } - return t2 -} - // Merge combines the two Metrics and returns a new result. func (m Metric) Merge(other Metric) Metric { @@ -141,7 +127,7 @@ func (m Metric) Merge(other Metric) Metric { return other case len(other.Samples) == 0: return m - case other.First.After(m.Last): + case other.First().After(m.Last()): samplesOut := make([]Sample, len(m.Samples)+len(other.Samples)) copy(samplesOut, m.Samples) copy(samplesOut[len(m.Samples):], other.Samples) @@ -149,10 +135,8 @@ func (m Metric) Merge(other Metric) Metric { Samples: samplesOut, Max: math.Max(m.Max, other.Max), Min: math.Min(m.Min, other.Min), - First: m.First, - Last: other.Last, } - case m.First.After(other.Last): + case m.First().After(other.Last()): samplesOut := make([]Sample, len(m.Samples)+len(other.Samples)) copy(samplesOut, other.Samples) copy(samplesOut[len(other.Samples):], m.Samples) @@ -160,8 +144,6 @@ func (m Metric) Merge(other Metric) Metric { Samples: samplesOut, Max: math.Max(m.Max, other.Max), Min: math.Min(m.Min, other.Min), - First: other.First, - Last: m.Last, } } @@ -194,8 +176,6 @@ func (m Metric) Merge(other Metric) Metric { Samples: samplesOut, Max: math.Max(m.Max, other.Max), Min: math.Min(m.Min, other.Min), - First: first(m.First, other.First), - Last: last(m.Last, other.Last), } } @@ -214,8 +194,6 @@ type WireMetrics struct { Samples []Sample `json:"samples,omitempty"` Min float64 `json:"min"` Max float64 `json:"max"` - First string `json:"first,omitempty"` - Last string `json:"last,omitempty"` dummySelfer } @@ -241,8 +219,6 @@ func (m Metric) ToIntermediate() WireMetrics { Samples: m.Samples, Max: m.Max, Min: m.Min, - First: renderTime(m.First), - Last: renderTime(m.Last), } } @@ -253,8 +229,6 @@ func (m WireMetrics) FromIntermediate() Metric { Samples: m.Samples, Max: m.Max, Min: m.Min, - First: parseTime(m.First), - Last: parseTime(m.Last), } } diff --git a/report/metrics_internal_test.go b/report/metrics_internal_test.go deleted file mode 100644 index 0929ba098..000000000 --- a/report/metrics_internal_test.go +++ /dev/null @@ -1,29 +0,0 @@ -package report - -import ( - "testing" - "time" -) - -func TestFirstLast(t *testing.T) { - zero, t1, t2 := time.Time{}, time.Now(), time.Now().Add(1*time.Minute) - tests := []struct { - arg1, arg2, first, last time.Time - }{ - {zero, zero, zero, zero}, - {t1, zero, t1, t1}, - {zero, t1, t1, t1}, - {t1, t1, t1, t1}, - {t1, t2, t1, t2}, - {t2, t1, t1, t2}, - } - for _, test := range tests { - if got := first(test.arg1, test.arg2); !got.Equal(test.first) { - t.Errorf("first(%q, %q) => %q, Expected: %q", test.arg1, test.arg2, got, test.first) - } - - if got := last(test.arg1, test.arg2); !got.Equal(test.last) { - t.Errorf("last(%q, %q) => %q, Expected: %q", test.arg1, test.arg2, got, test.last) - } - } -} diff --git a/report/metrics_test.go b/report/metrics_test.go index a04ced418..08ffc40ac 100644 --- a/report/metrics_test.go +++ b/report/metrics_test.go @@ -50,11 +50,11 @@ func TestMetricsCopy(t *testing.T) { } func checkMetric(t *testing.T, metric report.Metric, first, last time.Time, min, max float64) { - if !metric.First.Equal(first) { - t.Errorf("Expected metric.First == %q, but was: %q", first, metric.First) + if !metric.First().Equal(first) { + t.Errorf("Expected metric.First == %q, but was: %q", first, metric.First()) } - if !metric.Last.Equal(last) { - t.Errorf("Expected metric.Last == %q, but was: %q", last, metric.Last) + if !metric.Last().Equal(last) { + t.Errorf("Expected metric.Last == %q, but was: %q", last, metric.Last()) } if metric.Min != min { t.Errorf("Expected metric.Min == %f, but was: %f", min, metric.Min) @@ -65,9 +65,6 @@ func checkMetric(t *testing.T, metric report.Metric, first, last time.Time, min, } func TestMetricFirstLastMinMax(t *testing.T) { - - checkMetric(t, report.MakeMetric(nil), time.Time{}, time.Time{}, 0.0, 0.0) - t1 := time.Now() t2 := time.Now().Add(1 * time.Minute) @@ -103,18 +100,7 @@ func TestMetricMerge(t *testing.T) { } // Check it didn't modify metric1 - if !metric1.First.Equal(t2) { - t.Errorf("Expected metric1.First == %q, but was: %q", t2, metric1.First) - } - if !metric1.Last.Equal(t3) { - t.Errorf("Expected metric1.Last == %q, but was: %q", t3, metric1.Last) - } - if metric1.Min != 0.2 { - t.Errorf("Expected metric1.Min == %f, but was: %f", 0.2, metric1.Min) - } - if metric1.Max != 0.31 { - t.Errorf("Expected metric1.Max == %f, but was: %f", 0.31, metric1.Max) - } + checkMetric(t, metric1, t2, t3, 0.2, 0.31) // Check the result is not the same instance as metric1 if &metric1 == &have { From fdfbedeea920ece5d0db3974a5d53e824b89363f Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 12 Jul 2018 19:34:05 +0000 Subject: [PATCH 2/2] Stop exporting Metrics.First() and Last() They were exported for testing only, and the test isn't very useful. --- report/metrics.go | 12 ++++-------- report/metrics_test.go | 26 ++++++-------------------- 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/report/metrics.go b/report/metrics.go index 1d8fe50ad..2e0458c11 100644 --- a/report/metrics.go +++ b/report/metrics.go @@ -52,12 +52,8 @@ type Metric struct { Min, Max float64 } -// Following two functions are exported for testing only: make sure there are samples before calling. -// First gives the timestamp of the first sample -func (m Metric) First() time.Time { return m.Samples[0].Timestamp } - -// Last gives the timestamp of the last sample -func (m Metric) Last() time.Time { return m.Samples[len(m.Samples)-1].Timestamp } +func (m Metric) first() time.Time { return m.Samples[0].Timestamp } +func (m Metric) last() time.Time { return m.Samples[len(m.Samples)-1].Timestamp } // Sample is a single datapoint of a metric. type Sample struct { @@ -127,7 +123,7 @@ func (m Metric) Merge(other Metric) Metric { return other case len(other.Samples) == 0: return m - case other.First().After(m.Last()): + case other.first().After(m.last()): samplesOut := make([]Sample, len(m.Samples)+len(other.Samples)) copy(samplesOut, m.Samples) copy(samplesOut[len(m.Samples):], other.Samples) @@ -136,7 +132,7 @@ func (m Metric) Merge(other Metric) Metric { Max: math.Max(m.Max, other.Max), Min: math.Min(m.Min, other.Min), } - case m.First().After(other.Last()): + case m.first().After(other.last()): samplesOut := make([]Sample, len(m.Samples)+len(other.Samples)) copy(samplesOut, other.Samples) copy(samplesOut[len(other.Samples):], m.Samples) diff --git a/report/metrics_test.go b/report/metrics_test.go index 08ffc40ac..1eaa6bafc 100644 --- a/report/metrics_test.go +++ b/report/metrics_test.go @@ -49,13 +49,7 @@ func TestMetricsCopy(t *testing.T) { } } -func checkMetric(t *testing.T, metric report.Metric, first, last time.Time, min, max float64) { - if !metric.First().Equal(first) { - t.Errorf("Expected metric.First == %q, but was: %q", first, metric.First()) - } - if !metric.Last().Equal(last) { - t.Errorf("Expected metric.Last == %q, but was: %q", last, metric.Last()) - } +func checkMetric(t *testing.T, metric report.Metric, min, max float64) { if metric.Min != min { t.Errorf("Expected metric.Min == %f, but was: %f", min, metric.Min) } @@ -70,16 +64,16 @@ func TestMetricFirstLastMinMax(t *testing.T) { metric1 := report.MakeMetric([]report.Sample{{Timestamp: t1, Value: -0.1}, {Timestamp: t2, Value: 0.2}}) - checkMetric(t, metric1, t1, t2, -0.1, 0.2) - checkMetric(t, metric1.Merge(metric1), t1, t2, -0.1, 0.2) + checkMetric(t, metric1, -0.1, 0.2) + checkMetric(t, metric1.Merge(metric1), -0.1, 0.2) t3 := time.Now().Add(2 * time.Minute) t4 := time.Now().Add(3 * time.Minute) metric2 := report.MakeMetric([]report.Sample{{Timestamp: t3, Value: 0.31}, {Timestamp: t4, Value: 0.4}}) - checkMetric(t, metric2, t3, t4, 0.31, 0.4) - checkMetric(t, metric1.Merge(metric2), t1, t4, -0.1, 0.4) - checkMetric(t, metric2.Merge(metric1), t1, t4, -0.1, 0.4) + checkMetric(t, metric2, 0.31, 0.4) + checkMetric(t, metric1.Merge(metric2), -0.1, 0.4) + checkMetric(t, metric2.Merge(metric1), -0.1, 0.4) } func TestMetricMerge(t *testing.T) { @@ -98,14 +92,6 @@ func TestMetricMerge(t *testing.T) { if !reflect.DeepEqual(want, have) { t.Errorf("diff: %s", test.Diff(want, have)) } - - // Check it didn't modify metric1 - checkMetric(t, metric1, t2, t3, 0.2, 0.31) - - // Check the result is not the same instance as metric1 - if &metric1 == &have { - t.Errorf("Expected different pointers for metric1 and have, but both were: %p", &have) - } } func TestMetricMarshalling(t *testing.T) {