Use slices instead of linked lists for Metric

Also:

* Remove Gob encoder/decoder
* Stop using custom encoders/decoders for Timestamps (both ugorji and the Golang JSON codecs use nanosecond precision).
* Use idiomatic way to check for existence in metric.LastSample()

app/server_helpers.go

@@ -21,6 +21,6 @@ func respondWith(w http.ResponseWriter, code int, response interface{}) {
 	w.WriteHeader(code)
 	encoder := codec.NewEncoder(w, &codec.JsonHandle{})
 	if err := encoder.Encode(response); err != nil {
-		log.Errorf("Error encdoing response: %v", err)
+		log.Errorf("Error encoding response: %v", err)
 	}
 }

probe/host/reporter_test.go

@@ -88,10 +88,10 @@ func TestReporter(t *testing.T) {
 
 	// Should have metrics
 	for key, want := range metrics {
-		wantSample := want.LastSample()
+		wantSample, _ := want.LastSample()
 		if metric, ok := node.Metrics[key]; !ok {
 			t.Errorf("Expected %s metric, but not found", key)
-		} else if sample := metric.LastSample(); sample == nil {
+		} else if sample, ok := metric.LastSample(); !ok {
 			t.Errorf("Expected %s metric to have a sample, but there were none", key)
 		} else if sample.Value != wantSample.Value {
 			t.Errorf("Expected %s metric sample %f, got %f", key, wantSample, sample.Value)

probe/probe_internal_test.go

@@ -69,9 +69,6 @@ func (m mockPublisher) Stop() {
 }
 
 func TestProbe(t *testing.T) {
-	// marshalling->unmarshaling is not idempotent due to `json:"omitempty"`
-	// tags, transforming empty slices into nils. So, we make DeepEqual
-	// happy by setting empty `json:"omitempty"` entries to nil
 	const probeID = "probeid"
 	now := time.Now()
 	mtime.NowForce(now)
@@ -79,8 +76,15 @@ func TestProbe(t *testing.T) {
 
 	want := report.MakeReport()
 	node := report.MakeNodeWith("a", map[string]string{"b": "c"})
-	node.Metrics = nil // omitempty
+
-	// omitempty
+	// DeepEqual doesn't handle the location of timestamps correctly
+	// See https://github.com/golang/go/issues/10089
+	node.Controls.Timestamp = time.Now()
+
+	// marshalling->unmarshaling is not idempotent due to `json:"omitempty"`
+	// tags, transforming empty slices into nils. So, we make DeepEqual
+	// happy by setting empty `json:"omitempty"` entries to nil
+	node.Metrics = nil
 	want.Endpoint.Controls = nil
 	want.Process.Controls = nil
 	want.Container.Controls = nil

probe/process/reporter_test.go

@@ -62,7 +62,7 @@ func TestReporter(t *testing.T) {
 	}
 	if memoryUsage, ok := node.Metrics[process.MemoryUsage]; !ok {
 		t.Errorf("Expected memory usage metric, but not found")
-	} else if sample := memoryUsage.LastSample(); sample == nil {
+	} else if sample, ok := memoryUsage.LastSample(); !ok {
 		t.Errorf("Expected memory usage metric to have a sample, but there were none")
 	} else if sample.Value != 0. {
 		t.Errorf("Expected memory usage metric sample %f, got %f", 0., sample.Value)

report/controls.go

@@ -3,8 +3,6 @@ package report
 import (
 	"time"
 
-	"github.com/ugorji/go/codec"
-
 	"github.com/weaveworks/scope/common/mtime"
 )
 
@@ -50,11 +48,11 @@ func (cs Controls) AddControls(controls []Control) {
 }
 
 // NodeControls represent the individual controls that are valid for a given
-// node at a given point in time.  Its is immutable. A zero-value for Timestamp
+// 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
+	Timestamp time.Time `json:"timestamp,omitempty"`
-	Controls  StringSet
+	Controls  StringSet `json:"controls,omitempty"`
 }
 
 // MakeNodeControls makes a new NodeControls
@@ -85,39 +83,3 @@ func (nc NodeControls) Add(ids ...string) NodeControls {
 		Controls:  nc.Controls.Add(ids...),
 	}
 }
-
-// WireNodeControls is the intermediate type for json encoding.
-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{
-		Timestamp: renderTime(nc.Timestamp),
-		Controls:  nc.Controls,
-	})
-}
-
-// CodecDecodeSelf implements codec.Selfer
-func (nc *NodeControls) CodecDecodeSelf(decoder *codec.Decoder) {
-	in := WireNodeControls{}
-	if err := decoder.Decode(&in); err != nil {
-		return
-	}
-	*nc = NodeControls{
-		Timestamp: parseTime(in.Timestamp),
-		Controls:  in.Controls,
-	}
-}
-
-// MarshalJSON shouldn't be used, use CodecEncodeSelf instead
-func (NodeControls) MarshalJSON() ([]byte, error) {
-	panic("MarshalJSON shouldn't be used, use CodecEncodeSelf instead")
-}
-
-// UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead
-func (*NodeControls) UnmarshalJSON(b []byte) error {
-	panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead")
-}

report/metric_row.go

@@ -1,6 +1,8 @@
 package report
 
 import (
+	"time"
+
 	"github.com/ugorji/go/codec"
 )
 
@@ -55,23 +57,22 @@ func (*MetricRow) UnmarshalJSON(b []byte) error {
 }
 
 type wiredMetricRow struct {
-	ID       string   `json:"id"`
+	ID       string    `json:"id"`
-	Label    string   `json:"label"`
+	Label    string    `json:"label"`
-	Format   string   `json:"format,omitempty"`
+	Format   string    `json:"format,omitempty"`
-	Group    string   `json:"group,omitempty"`
+	Group    string    `json:"group,omitempty"`
-	Value    float64  `json:"value"`
+	Value    float64   `json:"value"`
-	Priority float64  `json:"priority,omitempty"`
+	Priority float64   `json:"priority,omitempty"`
-	Samples  []Sample `json:"samples"`
+	Samples  []Sample  `json:"samples"`
-	Min      float64  `json:"min"`
+	Min      float64   `json:"min"`
-	Max      float64  `json:"max"`
+	Max      float64   `json:"max"`
-	First    string   `json:"first,omitempty"`
+	First    time.Time `json:"first,omitempty"`
-	Last     string   `json:"last,omitempty"`
+	Last     time.Time `json:"last,omitempty"`
 }
 
 // CodecEncodeSelf marshals this MetricRow. It takes the basic Metric
 // rendering, then adds some row-specific fields.
 func (m *MetricRow) CodecEncodeSelf(encoder *codec.Encoder) {
-	in := m.Metric.ToIntermediate()
 	encoder.Encode(wiredMetricRow{
 		ID:       m.ID,
 		Label:    m.Label,
@@ -79,11 +80,11 @@ func (m *MetricRow) CodecEncodeSelf(encoder *codec.Encoder) {
 		Group:    m.Group,
 		Value:    m.Value,
 		Priority: m.Priority,
-		Samples:  in.Samples,
+		Samples:  m.Metric.Samples,
-		Min:      in.Min,
+		Min:      m.Metric.Min,
-		Max:      in.Max,
+		Max:      m.Metric.Max,
-		First:    in.First,
+		First:    m.Metric.First,
-		Last:     in.Last,
+		Last:     m.Metric.Last,
 	})
 }
 
@@ -91,14 +92,6 @@ func (m *MetricRow) CodecEncodeSelf(encoder *codec.Encoder) {
 func (m *MetricRow) CodecDecodeSelf(decoder *codec.Decoder) {
 	var in wiredMetricRow
 	decoder.Decode(&in)
-	w := WireMetrics{
-		Samples: in.Samples,
-		Min:     in.Min,
-		Max:     in.Max,
-		First:   in.First,
-		Last:    in.Last,
-	}
-	metric := w.FromIntermediate()
 	*m = MetricRow{
 		ID:       in.ID,
 		Label:    in.Label,
@@ -106,7 +99,13 @@ func (m *MetricRow) CodecDecodeSelf(decoder *codec.Decoder) {
 		Group:    in.Group,
 		Value:    in.Value,
 		Priority: in.Priority,
-		Metric:   &metric,
+		Metric: &Metric{
+			Samples: in.Samples,
+			Min:     in.Min,
+			Max:     in.Max,
+			First:   in.First,
+			Last:    in.Last,
+		},
 	}
 }
 

report/metric_template.go

@@ -28,7 +28,7 @@ func (t MetricTemplate) MetricRows(n Node) []MetricRow {
 		Priority: t.Priority,
 		Metric:   &metric,
 	}
-	if s := metric.LastSample(); s != nil {
+	if s, ok := metric.LastSample(); ok {
 		row.Value = toFixed(s.Value, 2)
 	}
 	return []MetricRow{row}

report/metrics.go

@@ -1,13 +1,8 @@
 package report
 
 import (
-	"bytes"
-	"encoding/gob"
 	"math"
 	"time"
-
-	"github.com/ugorji/go/codec"
-	"github.com/weaveworks/ps"
 )
 
 // Metrics is a string->metric map.
@@ -41,9 +36,11 @@ 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     ps.List
+	Samples []Sample  `json:"samples"`
-	Min, Max    float64
+	Min     float64   `json:"min"`
-	First, Last time.Time
+	Max     float64   `json:"max"`
+	First   time.Time `json:"first"`
+	Last    time.Time `json:"last"`
 }
 
 // Sample is a single datapoint of a metric.
@@ -52,20 +49,24 @@ type Sample struct {
 	Value     float64   `json:"value"`
 }
 
-var nilMetric = Metric{Samples: ps.NewList()}
-
 // MakeMetric makes a new Metric.
+// TODO: Specialized version adding the first sample to avoid generating garbage?
 func MakeMetric() Metric {
-	return nilMetric
+	return Metric{}
 }
 
-// Copy returns a value copy of the Metric. Metric is immutable, so we can skip
+// Copy returns a copy of the Metric.
-// this.
 func (m Metric) Copy() Metric {
-	return m
+	c := m
+	if c.Samples != nil {
+		c.Samples = make([]Sample, len(m.Samples))
+		copy(c.Samples, m.Samples)
+	}
+	return c
 }
 
-// WithFirst returns a fresh copy of m, with First set to t.
+// WithFirst returns a fresh copy of m, with first set to t.
+// TODO: This seems to be unused
 func (m Metric) WithFirst(t time.Time) Metric {
 	return Metric{
 		Samples: m.Samples,
@@ -89,10 +90,7 @@ func (m Metric) WithMax(max float64) Metric {
 
 // Len returns the number of samples in the metric.
 func (m Metric) Len() int {
-	if m.Samples == nil {
+	return len(m.Samples)
-		return 0
-	}
-	return m.Samples.Size()
 }
 
 func first(t1, t2 time.Time) time.Time {
@@ -109,47 +107,35 @@ func last(t1, t2 time.Time) time.Time {
 	return t2
 }
 
-// concat returns a new list formed by adding each element of acc to curr
-// acc or curr can be nil.
-func concat(acc []interface{}, curr ps.List) ps.List {
-	if curr == nil {
-		curr = ps.NewList()
-	}
-	for i := len(acc) - 1; i >= 0; i-- {
-		curr = curr.Cons(acc[i])
-	}
-	return curr
-}
-
 // 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.
-	// This should be O(1) to insert a latest element, and O(n) in general.
+	samplesOut := make([]Sample, 0, len(m.Samples)+1)
-	curr, acc := m.Samples, make([]interface{}, 0, m.Len()+1)
+	var i int
-	for {
+	// TODO: use binary search + copy() to improve performance
-		if curr == nil || curr.IsNil() {
+	for i = 0; i < len(m.Samples); i++ {
+		if m.Samples[i].Timestamp.Equal(t) {
+			i++
 			break
 		}
-
+		if m.Samples[i].Timestamp.After(t) {
-		currSample := curr.Head().(Sample)
-		if currSample.Timestamp.Equal(t) {
-			curr = curr.Tail()
 			break
 		}
-		if currSample.Timestamp.Before(t) {
+		samplesOut = append(samplesOut, m.Samples[i])
-			break
+	}
-		}
+	samplesOut = append(samplesOut, Sample{t, v})
-
+	if i < len(m.Samples) {
-		acc, curr = append(acc, curr.Head()), curr.Tail()
+		samplesOut = append(samplesOut, m.Samples[i:]...)
 	}
-	acc = append(acc, Sample{t, v})
-	curr = concat(acc, curr)
-
 	return Metric{
-		Samples: curr,
+		Samples: samplesOut,
 		Max:     math.Max(m.Max, v),
 		Min:     math.Min(m.Min, v),
 		First:   first(m.First, t),
@@ -159,33 +145,36 @@ func (m Metric) Add(t time.Time, v float64) Metric {
 
 // 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)
+	// Merge two lists of Samples in O(n)
-	curr1, curr2, acc := m.Samples, other.Samples, make([]interface{}, 0, m.Len()+other.Len())
-	var newSamples ps.List
 
+	// TODO: be smarter and check for non-overlapping metrics with first and last?
+	//       (copy() is much faster than checking every single sample)
+	samplesOut := make([]Sample, 0, len(m.Samples)+len(other.Samples))
+	mI, otherI := 0, 0
 	for {
-		if curr1 == nil || curr1.IsNil() {
+		if otherI >= len(other.Samples) {
-			newSamples = concat(acc, curr2)
+			samplesOut = append(samplesOut, m.Samples[mI:]...)
 			break
-		} else if curr2 == nil || curr2.IsNil() {
+		} else if mI >= len(m.Samples) {
-			newSamples = concat(acc, curr1)
+			samplesOut = append(samplesOut, other.Samples[otherI:]...)
 			break
 		}
 
-		s1 := curr1.Head().(Sample)
+		if m.Samples[mI].Timestamp.Equal(other.Samples[otherI].Timestamp) {
-		s2 := curr2.Head().(Sample)
+			samplesOut = append(samplesOut, m.Samples[mI])
-
+			mI++
-		if s1.Timestamp.Equal(s2.Timestamp) {
+			otherI++
-			curr1, curr2, acc = curr1.Tail(), curr2.Tail(), append(acc, s1)
+		} else if m.Samples[mI].Timestamp.Before(other.Samples[otherI].Timestamp) {
-		} else if s1.Timestamp.After(s2.Timestamp) {
+			samplesOut = append(samplesOut, m.Samples[mI])
-			curr1, acc = curr1.Tail(), append(acc, s1)
+			mI++
 		} else {
-			curr2, acc = curr2.Tail(), append(acc, s2)
+			samplesOut = append(samplesOut, other.Samples[otherI])
+			otherI++
 		}
 	}
 
 	return Metric{
-		Samples: newSamples,
+		Samples: samplesOut,
 		Max:     math.Max(m.Max, other.Max),
 		Min:     math.Min(m.Min, other.Min),
 		First:   first(m.First, other.First),
@@ -195,14 +184,14 @@ func (m Metric) Merge(other Metric) Metric {
 
 // Div returns a new copy of the metric, with each value divided by n.
 func (m Metric) Div(n float64) Metric {
-	curr, acc := m.Samples, ps.NewList()
+	samplesOut := make([]Sample, len(m.Samples), len(m.Samples))
-	for curr != nil && !curr.IsNil() {
+
-		s := curr.Head().(Sample)
+	for i := range m.Samples {
-		curr, acc = curr.Tail(), acc.Cons(Sample{s.Timestamp, s.Value / n})
+		samplesOut[i].Value = m.Samples[i].Value / n
+		samplesOut[i].Timestamp = m.Samples[i].Timestamp
 	}
-	acc = acc.Reverse()
 	return Metric{
-		Samples: acc,
+		Samples: samplesOut,
 		Max:     m.Max / n,
 		Min:     m.Min / n,
 		First:   m.First,
@@ -210,109 +199,10 @@ func (m Metric) Div(n float64) Metric {
 	}
 }
 
-// LastSample returns the last sample in the metric, or nil if there are no
+// LastSample obtains the last sample of the metric
-// samples.
+func (m Metric) LastSample() (Sample, bool) {
-func (m Metric) LastSample() *Sample {
+	if m.Samples == nil {
-	if m.Samples == nil || m.Samples.IsNil() {
+		return Sample{}, false
-		return nil
 	}
-	s := m.Samples.Head().(Sample)
+	return m.Samples[len(m.Samples)-1], true
-	return &s
-}
-
-// WireMetrics is the on-the-wire representation of Metrics.
-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.
-	Max     float64  `json:"max"`
-	First   string   `json:"first,omitempty"`
-	Last    string   `json:"last,omitempty"`
-}
-
-func renderTime(t time.Time) string {
-	if t.IsZero() {
-		return ""
-	}
-	return t.Format(time.RFC3339Nano)
-}
-
-func parseTime(s string) time.Time {
-	t, _ := time.Parse(time.RFC3339Nano, s)
-	return t
-}
-
-// ToIntermediate converts the metric to a representation suitable
-// for serialization.
-func (m Metric) ToIntermediate() WireMetrics {
-	samples := []Sample{}
-	if m.Samples != nil {
-		m.Samples.Reverse().ForEach(func(s interface{}) {
-			samples = append(samples, s.(Sample))
-		})
-	}
-	return WireMetrics{
-		Samples: samples,
-		Max:     m.Max,
-		Min:     m.Min,
-		First:   renderTime(m.First),
-		Last:    renderTime(m.Last),
-	}
-}
-
-// FromIntermediate obtains the metric from a representation suitable
-// for serialization.
-func (m WireMetrics) FromIntermediate() Metric {
-	samples := ps.NewList()
-	for _, s := range m.Samples {
-		samples = samples.Cons(s)
-	}
-	return Metric{
-		Samples: samples,
-		Max:     m.Max,
-		Min:     m.Min,
-		First:   parseTime(m.First),
-		Last:    parseTime(m.Last),
-	}
-}
-
-// CodecEncodeSelf implements codec.Selfer
-func (m *Metric) CodecEncodeSelf(encoder *codec.Encoder) {
-	in := m.ToIntermediate()
-	encoder.Encode(in)
-}
-
-// CodecDecodeSelf implements codec.Selfer
-func (m *Metric) CodecDecodeSelf(decoder *codec.Decoder) {
-	in := WireMetrics{}
-	if err := decoder.Decode(&in); err != nil {
-		return
-	}
-	*m = in.FromIntermediate()
-}
-
-// MarshalJSON shouldn't be used, use CodecEncodeSelf instead
-func (Metric) MarshalJSON() ([]byte, error) {
-	panic("MarshalJSON shouldn't be used, use CodecEncodeSelf instead")
-}
-
-// UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead
-func (*Metric) UnmarshalJSON(b []byte) error {
-	panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead")
-}
-
-// GobEncode implements gob.Marshaller
-func (m Metric) GobEncode() ([]byte, error) {
-	buf := bytes.Buffer{}
-	err := gob.NewEncoder(&buf).Encode(m.ToIntermediate())
-	return buf.Bytes(), err
-}
-
-// GobDecode implements gob.Unmarshaller
-func (m *Metric) GobDecode(input []byte) error {
-	in := WireMetrics{}
-	if err := gob.NewDecoder(bytes.NewBuffer(input)).Decode(&in); err != nil {
-		return err
-	}
-	*m = in.FromIntermediate()
-	return nil
 }

report/metrics_test.go

@@ -223,50 +223,37 @@ func TestMetricMarshalling(t *testing.T) {
 		want = want.Add(sample.Timestamp, sample.Value)
 	}
 
-	// gob
+	for _, h := range []codec.Handle{
-	{
+		codec.Handle(&codec.MsgpackHandle{}),
-		gobs, err := want.GobEncode()
+		codec.Handle(&codec.JsonHandle{}),
-		if err != nil {
+	} {
+		buf := &bytes.Buffer{}
+		encoder := codec.NewEncoder(buf, h)
+		if err := encoder.Encode(want); err != nil {
 			t.Fatal(err)
 		}
+		bufCopy := bytes.NewBuffer(buf.Bytes())
+
+		decoder := codec.NewDecoder(buf, h)
 		var have report.Metric
-		have.GobDecode(gobs)
+		if err := decoder.Decode(&have); err != nil {
+			t.Fatal(err)
+		}
+
 		if !reflect.DeepEqual(want, have) {
 			t.Error(test.Diff(want, have))
 		}
-	}
 
-	// others
+		// extra check for samples
-	{
+		decoder = codec.NewDecoder(bufCopy, h)
-
+		var wire struct {
-		for _, h := range []codec.Handle{
+			Samples []report.Sample `json:"samples"`
-			codec.Handle(&codec.MsgpackHandle{}),
+		}
-			codec.Handle(&codec.JsonHandle{}),
+		if err := decoder.Decode(&wire); err != nil {
-		} {
+			t.Error(err)
-			buf := &bytes.Buffer{}
+		}
-			encoder := codec.NewEncoder(buf, h)
+		if !reflect.DeepEqual(wantSamples, wire.Samples) {
-			want.CodecEncodeSelf(encoder)
+			t.Error(test.Diff(wantSamples, wire.Samples))
-			bufCopy := bytes.NewBuffer(buf.Bytes())
-
-			decoder := codec.NewDecoder(buf, h)
-			var have report.Metric
-			have.CodecDecodeSelf(decoder)
-
-			if !reflect.DeepEqual(want, have) {
-				t.Error(test.Diff(want, have))
-			}
-
-			// extra check for samples
-			decoder = codec.NewDecoder(bufCopy, h)
-			var wire struct {
-				Samples []report.Sample `json:"samples"`
-			}
-			if err := decoder.Decode(&wire); err != nil {
-				t.Error(err)
-			}
-			if !reflect.DeepEqual(wantSamples, wire.Samples) {
-				t.Error(test.Diff(wantSamples, wire.Samples))
-			}
 		}
 	}