[tbs] @matt's feedback

server/status/recorder.go

@@ -25,7 +25,6 @@ import (
 	"github.com/cockroachdb/cockroach/util/hlc"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/metric"
-	"github.com/codahale/hdrhistogram"
 	"github.com/gogo/protobuf/proto"
 )
 
@@ -41,6 +40,22 @@ const (
 	runtimeStatTimeSeriesNameFmt = "cr.node.sys.%s"
 )
 
+type quantile struct {
+	suffix   string
+	quantile float64
+}
+
+var recordHistogramQuantiles = []quantile{
+	{"-max", 100},
+	{"-p99.999", 99.999},
+	{"-p99.99", 99.99},
+	{"-p99.9", 99.9},
+	{"-p99", 99},
+	{"-p90", 90},
+	{"-p75", 75},
+	{"-p50", 50},
+}
+
 // NodeStatusRecorder is used to periodically persist the status of a node as a
 // set of time series data.
 type NodeStatusRecorder struct {
@@ -195,30 +210,13 @@ func (rr registryRecorder) record(dest *[]ts.TimeSeriesData) {
 		case *metric.Gauge:
 			data.Datapoints[0].Value = float64(mtr.Value())
 		case *metric.Histogram:
-			// `Each` calls the given closure under the lock, so we prefer it
-			// over calling Current() which would take a copy of the state.
-			// What we're doing here doesn't take too long.
-			mtr.Each(func(_ string, v interface{}) {
-				h := v.(*hdrhistogram.Histogram)
-				for _, pt := range []struct {
-					suffix   string
-					quantile float64
-				}{
-					{"-max", 100},
-					{"-p99.999", 99.999},
-					{"-p99.99", 99.99},
-					{"-p99.9", 99.9},
-					{"-p99", 99},
-					{"-p90", 90},
-					{"-p75", 75},
-					{"-p50", 50},
-				} {
-					d := *proto.Clone(data).(*ts.TimeSeriesData)
-					d.Name += pt.suffix
-					d.Datapoints[0].Value = h.ValueAtQuantile(pt.quantile)
-					*dest = append(*dest, d)
-				}
-			})
+			h := mtr.Current()
+			for _, pt := range recordHistogramQuantiles {
+				d := *proto.Clone(&data).(*ts.TimeSeriesData)
+				d.Name += pt.suffix
+				d.Datapoints[0].Value = float64(h.ValueAtQuantile(pt.quantile))
+				*dest = append(*dest, d)
+			}
 			return
 		default:
 			log.Warningf("cannot serialize for time series: %T", mtr)

server/status/recorder_test.go

@@ -18,6 +18,7 @@ package status
 
 import (
 	"reflect"
+	"regexp"
 	"sort"
 	"strconv"
 	"testing"
@@ -279,17 +280,32 @@ func TestNodeStatusRecorder(t *testing.T) {
 		generateStoreData(2, "capacity.available", 100, 75),
 
 		// Node stats.
-		generateNodeData(1, "exec.successcount", 100, 2),
-		generateNodeData(1, "exec.errorcount", 100, 1),
-		generateNodeData(1, "exec.success1h", 100, 0),
-		generateNodeData(1, "exec.error1h", 100, 0),
-		generateNodeData(1, "exec.success10m", 100, 0),
-		generateNodeData(1, "exec.error10m", 100, 0),
-		generateNodeData(1, "exec.success1m", 100, 0),
-		generateNodeData(1, "exec.error1m", 100, 0),
+		generateNodeData(1, "exec.success-count", 100, 2),
+		generateNodeData(1, "exec.error-count", 100, 1),
+		generateNodeData(1, "exec.success-1h", 100, 0),
+		generateNodeData(1, "exec.error-1h", 100, 0),
+		generateNodeData(1, "exec.success-10m", 100, 0),
+		generateNodeData(1, "exec.error-10m", 100, 0),
+		generateNodeData(1, "exec.success-1m", 100, 0),
+		generateNodeData(1, "exec.error-1m", 100, 0),
 	}
 
 	actual := recorder.GetTimeSeriesData()
+
+	var actNumLatencyMetrics int
+	expNumLatencyMetrics := len(recordHistogramQuantiles) * len(metric.DefaultTimeScales)
+	for _, item := range actual {
+		if ok, _ := regexp.MatchString(`cr.node.exec.latency.*`, item.Name); ok {
+			actNumLatencyMetrics++
+			expected = append(expected, item)
+		}
+	}
+
+	if expNumLatencyMetrics != actNumLatencyMetrics {
+		t.Fatalf("unexpected number of latency metrics %d, expected %d",
+			actNumLatencyMetrics, expNumLatencyMetrics)
+	}
+
 	sort.Sort(byTimeAndName(actual))
 	sort.Sort(byTimeAndName(expected))
 	if a, e := actual, expected; !reflect.DeepEqual(a, e) {

util/metric/metric.go

@@ -29,19 +29,19 @@ import (
 
 const histWrapNum = 4 // number of histograms to keep in rolling window
 
-type timeScale struct {
+// A TimeScale is a named duration.
+type TimeScale struct {
 	name string
 	d    time.Duration
 }
 
-var scale1M = timeScale{"1m", 1 * time.Minute}
-var scale10M = timeScale{"10m", 10 * time.Minute}
-var scale1H = timeScale{"1h", time.Hour}
+var scale1M = TimeScale{"1m", 1 * time.Minute}
+var scale10M = TimeScale{"10m", 10 * time.Minute}
+var scale1H = TimeScale{"1h", time.Hour}
 
 // Iterable provides a method for synchronized access to interior objects.
 type Iterable interface {
-	// Each calls the given closure with each contained item. The closure must
-	// copy values it plans to use after returning.
+	// Each calls the given closure with each contained item.
 	Each(func(string, interface{}))
 }
 
@@ -132,11 +132,11 @@ func (h *Histogram) Current() *hdrhistogram.Histogram {
 	return hdrhistogram.Import(export)
 }
 
-// Each calls the closure with the empty string and the (locked) receiver.
+// Each calls the closure with the empty string and the receiver.
 func (h *Histogram) Each(f func(string, interface{})) {
 	h.mu.Lock()
-	defer h.mu.Unlock()
 	maybeTick(h)
+	h.mu.Unlock()
 	f("", h)
 }
 
@@ -238,12 +238,13 @@ func (e *Rate) Add(v float64) {
 	e.mu.Unlock()
 }
 
-// Each calls the given closure with the empty string and the Rate.
+// Each calls the given closure with the empty string and the Rate's current value.
 func (e *Rate) Each(f func(string, interface{})) {
 	e.mu.Lock()
-	defer e.mu.Unlock()
 	maybeTick(e)
-	f("", e.wrapped.Value())
+	v := e.wrapped.Value()
+	e.mu.Unlock()
+	f("", v)
 }
 
 // MarshalJSON marshals to JSON.

util/metric/registry.go

@@ -24,6 +24,12 @@ import (
 	"time"
 )
 
+const sep = "-"
+
+// DefaultTimeScales are the durations used for helpers which create windowed
+// metrics in bulk (such as Latency or Rates).
+var DefaultTimeScales = []TimeScale{scale1M, scale10M, scale1H}
+
 // A Registry bundles up various iterables (i.e. typically metrics or other
 // registries) to provide a single point of access to them.
 type Registry struct {
@@ -101,10 +107,10 @@ func (r *Registry) Histogram(name string, duration time.Duration, maxVal int64,
 // with two digits of precision (i.e. errors of <1ms at 100ms, <.6s at 1m).
 // The generated names of the metric will begin with the given prefix.
 func (r *Registry) Latency(prefix string) Histograms {
-	windows := []timeScale{scale1M, scale10M, scale1H}
+	windows := DefaultTimeScales
 	hs := make([]*Histogram, 0, 3)
 	for _, w := range windows {
-		h := r.Histogram(prefix+w.name, w.d, int64(time.Minute), 2)
+		h := r.Histogram(prefix+sep+w.name, w.d, int64(time.Minute), 2)
 		hs = append(hs, h)
 	}
 	return hs
@@ -135,11 +141,11 @@ func (r *Registry) Rate(name string, timescale time.Duration) *Rate {
 // Rates returns a slice of EWMAs prefixed with the given name and
 // various "standard" timescales.
 func (r *Registry) Rates(prefix string) Rates {
-	scales := []timeScale{scale1M, scale10M, scale1H}
+	scales := DefaultTimeScales
 	es := make([]*Rate, 0, len(scales))
 	for _, scale := range scales {
-		es = append(es, r.Rate(prefix+scale.name, scale.d))
+		es = append(es, r.Rate(prefix+sep+scale.name, scale.d))
 	}
-	c := r.Counter(prefix + "count")
+	c := r.Counter(prefix + sep + "count")
 	return Rates{Counter: c, Rates: es}
 }

util/metric/registry_test.go

@@ -27,33 +27,33 @@ func TestRegistry(t *testing.T) {
 
 	_ = r.Gauge("top.gauge")
 	_ = r.Rate("top.rate", time.Minute)
-	_ = r.Rates("top.rates.")
+	_ = r.Rates("top.rates")
 	_ = r.Histogram("top.hist", time.Minute, 1000, 3)
-	_ = r.Latency("top.latency.")
+	_ = r.Latency("top.latency")
 
 	_ = sub.Gauge("gauge")
 	r.MustAdd("bottom.%s#1", sub)
 	if err := r.Add("bottom.%s#1", sub); err == nil {
 		t.Fatalf("expected failure on double-add")
 	}
-	_ = sub.Rates("rates.")
+	_ = sub.Rates("rates")
 
 	expNames := map[string]struct{}{
 		"top.rate":             {},
-		"top.rates.count":      {},
-		"top.rates.1m":         {},
-		"top.rates.10m":        {},
-		"top.rates.1h":         {},
+		"top.rates-count":      {},
+		"top.rates-1m":         {},
+		"top.rates-10m":        {},
+		"top.rates-1h":         {},
 		"top.hist":             {},
-		"top.latency.1m":       {},
-		"top.latency.10m":      {},
-		"top.latency.1h":       {},
+		"top.latency-1m":       {},
+		"top.latency-10m":      {},
+		"top.latency-1h":       {},
 		"top.gauge":            {},
 		"bottom.gauge#1":       {},
-		"bottom.rates.count#1": {},
-		"bottom.rates.1m#1":    {},
-		"bottom.rates.10m#1":   {},
-		"bottom.rates.1h#1":    {},
+		"bottom.rates-count#1": {},
+		"bottom.rates-1m#1":    {},
+		"bottom.rates-10m#1":   {},
+		"bottom.rates-1h#1":    {},
 	}
 
 	r.Each(func(name string, _ interface{}) {