Merge pull request #599 from feliksik/feliksik/easy-percentiles

Cleaner/easier way for user to specify Cloudwatch metric percentiles

metrics/cloudwatch/cloudwatch.go

@@ -14,6 +14,7 @@ import (
 	"github.com/go-kit/kit/metrics"
 	"github.com/go-kit/kit/metrics/generic"
 	"github.com/go-kit/kit/metrics/internal/lv"
+	"strconv"
 )
 
 const (
@@ -38,7 +39,7 @@ type CloudWatch struct {
 	counters              *lv.Space
 	gauges                *lv.Space
 	histograms            *lv.Space
-	percentiles           Percentiles
+	percentiles           []float64 // percentiles to track
 	logger                log.Logger
 	numConcurrentRequests int
 }
@@ -57,16 +58,19 @@ func WithLogger(logger log.Logger) option {
 	}
 }
 
-func WithPercentiles(p Percentiles) option {
+// WithPercentiles registers the percentiles to track, overriding the
+// existing/default values.
+// Reason is that Cloudwatch makes you pay per metric, so you can save half the money
+// by only using 2 metrics instead of the default 4.
+func WithPercentiles(percentiles ...float64) option {
 	return func(c *CloudWatch) {
-		validated := Percentiles{}
-		for _, entry := range p {
-			if entry.f < 0 || entry.f > 1 {
-				continue // illegal entry
+		c.percentiles = make([]float64, 0, len(percentiles))
+		for _, p := range percentiles {
+			if p < 0 || p > 1 {
+				continue // illegal entry; ignore
 			}
-			validated = append(validated, entry)
+			c.percentiles = append(c.percentiles, p)
 		}
-		c.percentiles = validated
 	}
 }
 
@@ -93,12 +97,7 @@ func New(namespace string, svc cloudwatchiface.CloudWatchAPI, options ...option)
 		histograms:            lv.NewSpace(),
 		numConcurrentRequests: 10,
 		logger:                log.NewLogfmtLogger(os.Stderr),
-		percentiles: Percentiles{
-			{"50", 0.50},
-			{"90", 0.90},
-			{"95", 0.95},
-			{"99", 0.99},
-		},
+		percentiles:           []float64{0.50, 0.90, 0.95, 0.99},
 	}
 
 	for _, optFunc := range options {
@@ -179,17 +178,25 @@ func (cw *CloudWatch) Send() error {
 		return true
 	})
 
+	// format a [0,1]-float value to a percentile value, with minimum nr of decimals
+	// 0.90 -> "90"
+	// 0.95 -> "95"
+	// 0.999 -> "99.9"
+	formatPerc := func(p float64) string {
+		return strconv.FormatFloat(p*100, 'f', -1, 64)
+	}
+
 	cw.histograms.Reset().Walk(func(name string, lvs lv.LabelValues, values []float64) bool {
 		histogram := generic.NewHistogram(name, 50)
 
 		for _, v := range values {
 			histogram.Observe(v)
 		}
 
-		for _, p := range cw.percentiles {
-			value := histogram.Quantile(p.f)
+		for _, perc := range cw.percentiles {
+			value := histogram.Quantile(perc)
 			datums = append(datums, &cloudwatch.MetricDatum{
-				MetricName: aws.String(fmt.Sprintf("%s_%s", name, p.s)),
+				MetricName: aws.String(fmt.Sprintf("%s_%s", name, formatPerc(perc))),
 				Dimensions: makeDimensions(lvs...),
 				Value:      aws.Float64(value),
 				Timestamp:  aws.Time(now),

metrics/cloudwatch/cloudwatch_test.go

@@ -193,4 +193,52 @@ func TestHistogram(t *testing.T) {
 	if err := svc.testDimensions(n99, label, value); err != nil {
 		t.Fatal(err)
 	}
+
+	// now test with only 2 custom percentiles
+	//
+	svc = newMockCloudWatch()
+	cw = New(namespace, svc, WithLogger(log.NewNopLogger()), WithPercentiles(0.50, 0.90))
+	histogram = cw.NewHistogram(name).With(label, value)
+
+	customQuantiles := func() (p50, p90, p95, p99 float64) {
+		err := cw.Send()
+		if err != nil {
+			t.Fatal(err)
+		}
+		svc.mtx.RLock()
+		defer svc.mtx.RUnlock()
+		p50 = svc.valuesReceived[n50]
+		p90 = svc.valuesReceived[n90]
+
+		// our teststat.TestHistogram wants us to give p95 and p99,
+		// but with custom percentiles we don't have those.
+		// So fake them. Maybe we should make teststat.nvq() public and use that?
+		p95 = 541.121341
+		p99 = 558.158697
+
+		// but fail if they are actually set (because that would mean the
+		// WithPercentiles() is not respected)
+		if _, isSet := svc.valuesReceived[n95]; isSet {
+			t.Fatal("p95 should not be set")
+		}
+		if _, isSet := svc.valuesReceived[n99]; isSet {
+			t.Fatal("p99 should not be set")
+		}
+		return
+	}
+	if err := teststat.TestHistogram(histogram, customQuantiles, 0.01); err != nil {
+		t.Fatal(err)
+	}
+	if err := svc.testDimensions(n50, label, value); err != nil {
+		t.Fatal(err)
+	}
+	if err := svc.testDimensions(n90, label, value); err != nil {
+		t.Fatal(err)
+	}
+	if err := svc.testDimensions(n95, label, value); err != nil {
+		t.Fatal(err)
+	}
+	if err := svc.testDimensions(n99, label, value); err != nil {
+		t.Fatal(err)
+	}
 }