Remove Metric Add() method

* Helps reduce garbage (MakeMetric() now takes a slice and there's a shorter version MakeSingletonMetric()) * Fixes bug computing Max (Min) in samples since using MakeMetric() was causing a default Max/Min of zero. * Simplifies code a bit
2026-08-19 04:16:21 +00:00 · 2016-08-01 16:58:11 +00:00
parent 3e000662f4
commit 8a950a59d6
15 changed files with 152 additions and 193 deletions
@@ -52,8 +52,8 @@ func (cs Controls) AddControls(controls []Control) {
 // node at a given point in time.  It's immutable. A zero-value for Timestamp
 // indicated this NodeControls is 'not set'.
 type NodeControls struct {
-	Timestamp time.Time `json:"timestamp,omitempty"`
-	Controls  StringSet `json:"controls,omitempty"`
+	Timestamp time.Time
+	Controls  StringSet
 }

 // MakeNodeControls makes a new NodeControls
@@ -85,15 +85,17 @@ func (nc NodeControls) Add(ids ...string) NodeControls {
 	}
 }

-// WireNodeControls is the intermediate type for json encoding.
-type WireNodeControls struct {
+// WireNodeControls is the intermediate type for encoding/decoding.
+// Only needed for backwards compatibility with probes
+// (time.Time is encoded in binary in MsgPack)
+type wireNodeControls struct {
 	Timestamp string    `json:"timestamp,omitempty"`
 	Controls  StringSet `json:"controls,omitempty"`
 }

 // CodecEncodeSelf implements codec.Selfer
 func (nc *NodeControls) CodecEncodeSelf(encoder *codec.Encoder) {
-	encoder.Encode(WireNodeControls{
+	encoder.Encode(wireNodeControls{
 		Timestamp: renderTime(nc.Timestamp),
 		Controls:  nc.Controls,
 	})
@@ -101,7 +103,7 @@ func (nc *NodeControls) CodecEncodeSelf(encoder *codec.Encoder) {

 // CodecDecodeSelf implements codec.Selfer
 func (nc *NodeControls) CodecDecodeSelf(decoder *codec.Decoder) {
-	in := WireNodeControls{}
+	in := wireNodeControls{}
 	if err := decoder.Decode(&in); err != nil {
 		return
 	}
@@ -54,6 +54,8 @@ func (*MetricRow) UnmarshalJSON(b []byte) error {
 	panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead")
 }

+// Needed to flatten the fields for backwards compatibility with probes
+// (time.Time is encoded in binary in MsgPack)
 type wiredMetricRow struct {
 	ID       string   `json:"id"`
 	Label    string   `json:"label"`
@@ -21,7 +21,11 @@ func (m Metrics) Lookup(key string) (Metric, bool) {
 func (m Metrics) Merge(other Metrics) Metrics {
 	result := m.Copy()
 	for k, v := range other {
-		result[k] = result[k].Merge(v)
+		if rv, ok := result[k]; ok {
+			result[k] = rv.Merge(v)
+		} else {
+			result[k] = v.Copy()
+		}
 	}
 	return result
 }
@@ -49,10 +53,45 @@ type Sample struct {
 	Value     float64   `json:"value"`
 }

-// MakeMetric makes a new Metric.
-// TODO: Specialized version adding the first sample to avoid generating garbage?
-func MakeMetric() Metric {
-	return Metric{}
+// MakeSingletonMetric makes a metric with a single value
+func MakeSingletonMetric(t time.Time, v float64) Metric {
+	return Metric{
+		Samples: []Sample{{t, v}},
+		Min:     v,
+		Max:     v,
+		First:   t,
+		Last:    t,
+	}
+
+}
+
+// MakeMetric makes a new Metric from unique samples incrementally ordered in
+// time.
+func MakeMetric(samples []Sample) Metric {
+	if len(samples) < 1 {
+		return Metric{}
+	}
+
+	var (
+		min = samples[0].Value
+		max = samples[0].Value
+	)
+
+	for i := 1; i < len(samples); i++ {
+		if samples[i].Value < min {
+			min = samples[i].Value
+		} else if samples[i].Value > max {
+			max = samples[i].Value
+		}
+	}
+
+	return Metric{
+		Samples: samples,
+		Min:     min,
+		Max:     max,
+		First:   samples[0].Timestamp,
+		Last:    samples[len(samples)-1].Timestamp,
+	}
 }

 // Copy returns a copy of the Metric.
@@ -107,42 +146,6 @@ func last(t1, t2 time.Time) time.Time {
 	return t2
 }

-// Add returns a new Metric with (t, v) added to its Samples. Add is the only
-// valid way to grow a Metric.
-// TODO: join t and v into a Sample to avoid extra allocations?
-// TODO: This seems to be too elaborate, Add() only seems to be used to add ordered Samples.
-//       Replace this by a specialized version getting a slice of ordered Samples
-//       without duplicates?
-func (m Metric) Add(t time.Time, v float64) Metric {
-	// Find the first element which is before you element, and insert
-	// your new element in the list.  NB we want to dedupe entries with
-	// equal timestamps.
-	samplesOut := make([]Sample, 0, len(m.Samples)+1)
-	var i int
-	// TODO: use binary search + copy() to improve performance
-	for i = 0; i < len(m.Samples); i++ {
-		if m.Samples[i].Timestamp.Equal(t) {
-			i++
-			break
-		}
-		if m.Samples[i].Timestamp.After(t) {
-			break
-		}
-		samplesOut = append(samplesOut, m.Samples[i])
-	}
-	samplesOut = append(samplesOut, Sample{t, v})
-	if i < len(m.Samples) {
-		samplesOut = append(samplesOut, m.Samples[i:]...)
-	}
-	return Metric{
-		Samples: samplesOut,
-		Max:     math.Max(m.Max, v),
-		Min:     math.Min(m.Min, v),
-		First:   first(m.First, t),
-		Last:    last(m.Last, t),
-	}
-}
-
 // Merge combines the two Metrics and returns a new result.
 func (m Metric) Merge(other Metric) Metric {
 	// Merge two lists of Samples in O(n)
@@ -208,9 +211,11 @@ func (m Metric) LastSample() (Sample, bool) {
 }

 // WireMetrics is the on-the-wire representation of Metrics.
+// Only needed for backwards compatibility with probes
+// (time.Time is encoded in binary in MsgPack)
 type WireMetrics struct {
-	Samples []Sample `json:"samples,omitempty"` // On the wire, samples are sorted oldest to newest,
-	Min     float64  `json:"min"`               // the opposite order to how we store them internally.
+	Samples []Sample `json:"samples,omitempty"`
+	Min     float64  `json:"min"`
 	Max     float64  `json:"max"`
 	First   string   `json:"first,omitempty"`
 	Last    string   `json:"last,omitempty"`
@@ -19,17 +19,17 @@ func TestMetricsMerge(t *testing.T) {
 	t4 := time.Now().Add(3 * time.Minute)

 	metrics1 := report.Metrics{
-		"metric1": report.MakeMetric().Add(t1, 0.1).Add(t2, 0.2),
-		"metric2": report.MakeMetric().Add(t3, 0.3),
+		"metric1": report.MakeMetric([]report.Sample{{t1, 0.1}, {t2, 0.2}}),
+		"metric2": report.MakeSingletonMetric(t3, 0.3),
 	}
 	metrics2 := report.Metrics{
-		"metric2": report.MakeMetric().Add(t4, 0.4),
-		"metric3": report.MakeMetric().Add(t1, 0.1).Add(t2, 0.2),
+		"metric2": report.MakeSingletonMetric(t4, 0.4),
+		"metric3": report.MakeMetric([]report.Sample{{t1, 0.1}, {t2, 0.2}}),
 	}
 	want := report.Metrics{
-		"metric1": report.MakeMetric().Add(t1, 0.1).Add(t2, 0.2),
-		"metric2": report.MakeMetric().Add(t3, 0.3).Add(t4, 0.4),
-		"metric3": report.MakeMetric().Add(t1, 0.1).Add(t2, 0.2),
+		"metric1": report.MakeMetric([]report.Sample{{t1, 0.1}, {t2, 0.2}}),
+		"metric2": report.MakeMetric([]report.Sample{{t3, 0.3}, {t4, 0.4}}),
+		"metric3": report.MakeMetric([]report.Sample{{t1, 0.1}, {t2, 0.2}}),
 	}
 	have := metrics1.Merge(metrics2)
 	if !reflect.DeepEqual(want, have) {
@@ -40,7 +40,7 @@ func TestMetricsMerge(t *testing.T) {
 func TestMetricsCopy(t *testing.T) {
 	t1 := time.Now()
 	want := report.Metrics{
-		"metric1": report.MakeMetric().Add(t1, 0.1),
+		"metric1": report.MakeSingletonMetric(t1, 0.1),
 	}
 	delete(want.Copy(), "metric1") // Modify a copy
 	have := want.Copy()            // Check the original wasn't affected
@@ -65,66 +65,24 @@ func checkMetric(t *testing.T, metric report.Metric, first, last time.Time, min,
 }

 func TestMetricFirstLastMinMax(t *testing.T) {
-	metric := report.MakeMetric()
-	var zero time.Time
+
+	checkMetric(t, report.MakeMetric(nil), time.Time{}, time.Time{}, 0.0, 0.0)
+
 	t1 := time.Now()
 	t2 := time.Now().Add(1 * time.Minute)
+
+	metric1 := report.MakeMetric([]report.Sample{{t1, -0.1}, {t2, 0.2}})
+
+	checkMetric(t, metric1, t1, t2, -0.1, 0.2)
+	checkMetric(t, metric1.Merge(metric1), t1, t2, -0.1, 0.2)
+
 	t3 := time.Now().Add(2 * time.Minute)
 	t4 := time.Now().Add(3 * time.Minute)
-	other := report.MakeMetric()
-	other.Max = 5
-	other.Min = -5
-	other.First = t1.Add(-1 * time.Minute)
-	other.Last = t4.Add(1 * time.Minute)
+	metric2 := report.MakeMetric([]report.Sample{{t3, 0.31}, {t4, 0.4}})

-	tests := []struct {
-		f           func(report.Metric) report.Metric
-		first, last time.Time
-		min, max    float64
-	}{
-		{nil, zero, zero, 0, 0},
-		{func(m report.Metric) report.Metric { return m.Add(t2, 2) }, t2, t2, 0, 2},
-		{func(m report.Metric) report.Metric { return m.Add(t1, 1) }, t1, t2, 0, 2},
-		{func(m report.Metric) report.Metric { return m.Add(t3, -1) }, t1, t3, -1, 2},
-		{func(m report.Metric) report.Metric { return m.Add(t4, 3) }, t1, t4, -1, 3},
-		{func(m report.Metric) report.Metric { return m.Merge(other) }, t1.Add(-1 * time.Minute), t4.Add(1 * time.Minute), -5, 5},
-	}
-	for _, test := range tests {
-		oldFirst, oldLast, oldMin, oldMax := metric.First, metric.Last, metric.Min, metric.Max
-		oldMetric := metric
-		if test.f != nil {
-			metric = test.f(metric)
-		}
-
-		// Check it didn't modify the old one
-		checkMetric(t, oldMetric, oldFirst, oldLast, oldMin, oldMax)
-
-		// Check the new one is as expected
-		checkMetric(t, metric, test.first, test.last, test.min, test.max)
-	}
-}
-
-func TestMetricAdd(t *testing.T) {
-	s := []report.Sample{
-		{time.Now(), 0.1},
-		{time.Now().Add(1 * time.Minute), 0.2},
-		{time.Now().Add(2 * time.Minute), 0.3},
-	}
-
-	have := report.MakeMetric().
-		Add(s[0].Timestamp, s[0].Value).
-		Add(s[2].Timestamp, s[2].Value). // Keeps sorted
-		Add(s[1].Timestamp, s[1].Value).
-		Add(s[2].Timestamp, 0.5) // Overwrites duplicate timestamps
-
-	want := report.MakeMetric().
-		Add(s[0].Timestamp, s[0].Value).
-		Add(s[1].Timestamp, s[1].Value).
-		Add(s[2].Timestamp, 0.5)
-
-	if !reflect.DeepEqual(want, have) {
-		t.Errorf("diff: %s", test.Diff(want, have))
-	}
+	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)
 }

 func TestMetricMerge(t *testing.T) {
@@ -133,20 +91,12 @@ func TestMetricMerge(t *testing.T) {
 	t3 := time.Now().Add(2 * time.Minute)
 	t4 := time.Now().Add(3 * time.Minute)

-	metric1 := report.MakeMetric().
-		Add(t2, 0.2).
-		Add(t3, 0.31)
+	metric1 := report.MakeMetric([]report.Sample{{t2, 0.2}, {t3, 0.31}})

-	metric2 := report.MakeMetric().
-		Add(t1, -0.1).
-		Add(t3, 0.3).
-		Add(t4, 0.4)
+	metric2 := report.MakeMetric([]report.Sample{{t1, -0.1}, {t3, 0.3}, {t4, 0.4}})
+
+	want := report.MakeMetric([]report.Sample{{t1, -0.1}, {t2, 0.2}, {t3, 0.31}, {t4, 0.4}})

-	want := report.MakeMetric().
-		Add(t1, -0.1).
-		Add(t2, 0.2).
-		Add(t3, 0.31).
-		Add(t4, 0.4)
 	have := metric1.Merge(metric2)
 	if !reflect.DeepEqual(want, have) {
 		t.Errorf("diff: %s", test.Diff(want, have))
@@ -159,8 +109,8 @@ func TestMetricMerge(t *testing.T) {
 	if !metric1.Last.Equal(t3) {
 		t.Errorf("Expected metric1.Last == %q, but was: %q", t3, metric1.Last)
 	}
-	if metric1.Min != 0.0 {
-		t.Errorf("Expected metric1.Min == %f, but was: %f", 0.0, metric1.Min)
+	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)
@@ -173,13 +123,13 @@ func TestMetricMerge(t *testing.T) {
 }

 func TestMetricCopy(t *testing.T) {
-	want := report.MakeMetric()
+	want := report.MakeMetric(nil)
 	have := want.Copy()
 	if !reflect.DeepEqual(want, have) {
 		t.Errorf("diff: %s", test.Diff(want, have))
 	}

-	want = report.MakeMetric().Add(time.Now(), 1)
+	want = report.MakeSingletonMetric(time.Now(), 1)
 	have = want.Copy()
 	if !reflect.DeepEqual(want, have) {
 		t.Errorf("diff: %s", test.Diff(want, have))
@@ -190,12 +140,8 @@ func TestMetricDiv(t *testing.T) {
 	t1 := time.Now()
 	t2 := time.Now().Add(1 * time.Minute)

-	want := report.MakeMetric().
-		Add(t1, -2).
-		Add(t2, 2)
-	beforeDiv := report.MakeMetric().
-		Add(t1, -2048).
-		Add(t2, 2048)
+	want := report.MakeMetric([]report.Sample{{t1, -2}, {t2, 2}})
+	beforeDiv := report.MakeMetric([]report.Sample{{t1, -2048}, {t2, 2048}})
 	have := beforeDiv.Div(1024)
 	if !reflect.DeepEqual(want, have) {
 		t.Errorf("diff: %s", test.Diff(want, have))
@@ -218,10 +164,7 @@ func TestMetricMarshalling(t *testing.T) {
 		{Timestamp: t4, Value: 0.4},
 	}

-	want := report.MakeMetric()
-	for _, sample := range wantSamples {
-		want = want.Add(sample.Timestamp, sample.Value)
-	}
+	want := report.MakeMetric(wantSamples)

 	for _, h := range []codec.Handle{
 		codec.Handle(&codec.MsgpackHandle{}),