From f583a2039ca3042007600903282da50c57d3673b Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Tue, 8 Aug 2017 17:22:17 +0200 Subject: [PATCH 1/4] Cleaner/easier way for user to specify Cloudwatch metric percentiles. --- metrics/cloudwatch/cloudwatch.go | 39 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/metrics/cloudwatch/cloudwatch.go b/metrics/cloudwatch/cloudwatch.go index 61cc502f1..3a5f4f86d 100644 --- a/metrics/cloudwatch/cloudwatch.go +++ b/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,17 @@ func WithLogger(logger log.Logger) option { } } -func WithPercentiles(p Percentiles) option { +// WithPercentiles registers the percentiles to track, overriding the +// existing/default values. +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) + 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 +95,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,6 +176,14 @@ 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) @@ -186,10 +191,10 @@ func (cw *CloudWatch) Send() error { 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_p%s", name, formatPerc(perc))), Dimensions: makeDimensions(lvs...), Value: aws.Float64(value), Timestamp: aws.Time(now), From c9614280c02e154c0b27aaf1b2ec0bf5c37aed19 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Wed, 9 Aug 2017 09:23:46 +0200 Subject: [PATCH 2/4] fix test to read quantile metrics with p prefix --- metrics/cloudwatch/cloudwatch_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/metrics/cloudwatch/cloudwatch_test.go b/metrics/cloudwatch/cloudwatch_test.go index 8ed93ace3..f934cc8d3 100644 --- a/metrics/cloudwatch/cloudwatch_test.go +++ b/metrics/cloudwatch/cloudwatch_test.go @@ -161,10 +161,10 @@ func TestHistogram(t *testing.T) { svc := newMockCloudWatch() cw := New(namespace, svc, WithLogger(log.NewNopLogger())) histogram := cw.NewHistogram(name).With(label, value) - n50 := fmt.Sprintf("%s_50", name) - n90 := fmt.Sprintf("%s_90", name) - n95 := fmt.Sprintf("%s_95", name) - n99 := fmt.Sprintf("%s_99", name) + n50 := fmt.Sprintf("%s_p50", name) + n90 := fmt.Sprintf("%s_p90", name) + n95 := fmt.Sprintf("%s_p95", name) + n99 := fmt.Sprintf("%s_p99", name) quantiles := func() (p50, p90, p95, p99 float64) { err := cw.Send() if err != nil { From 0be9fba9d47d851aa82b80a12939b9769381e6d5 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Thu, 10 Aug 2017 10:13:39 +0200 Subject: [PATCH 3/4] test cloudwatch.WithPercentiles() --- metrics/cloudwatch/cloudwatch.go | 4 ++- metrics/cloudwatch/cloudwatch_test.go | 48 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/metrics/cloudwatch/cloudwatch.go b/metrics/cloudwatch/cloudwatch.go index 3a5f4f86d..3d664d70c 100644 --- a/metrics/cloudwatch/cloudwatch.go +++ b/metrics/cloudwatch/cloudwatch.go @@ -60,9 +60,11 @@ func WithLogger(logger log.Logger) 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) { - c.percentiles = make([]float64, 0) + c.percentiles = make([]float64, 0, len(percentiles)) for _, p := range percentiles { if p < 0 || p > 1 { continue // illegal entry; ignore diff --git a/metrics/cloudwatch/cloudwatch_test.go b/metrics/cloudwatch/cloudwatch_test.go index f934cc8d3..c56201d9b 100644 --- a/metrics/cloudwatch/cloudwatch_test.go +++ b/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) + } } From 4d9f5253578945115b0504c387870beefdc0e806 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Mon, 14 Aug 2017 10:20:10 +0200 Subject: [PATCH 4/4] do not prefix metrics with 'p', just like it was previously. --- metrics/cloudwatch/cloudwatch.go | 2 +- metrics/cloudwatch/cloudwatch_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/metrics/cloudwatch/cloudwatch.go b/metrics/cloudwatch/cloudwatch.go index 3d664d70c..4322d4cf2 100644 --- a/metrics/cloudwatch/cloudwatch.go +++ b/metrics/cloudwatch/cloudwatch.go @@ -196,7 +196,7 @@ func (cw *CloudWatch) Send() error { for _, perc := range cw.percentiles { value := histogram.Quantile(perc) datums = append(datums, &cloudwatch.MetricDatum{ - MetricName: aws.String(fmt.Sprintf("%s_p%s", name, formatPerc(perc))), + MetricName: aws.String(fmt.Sprintf("%s_%s", name, formatPerc(perc))), Dimensions: makeDimensions(lvs...), Value: aws.Float64(value), Timestamp: aws.Time(now), diff --git a/metrics/cloudwatch/cloudwatch_test.go b/metrics/cloudwatch/cloudwatch_test.go index c56201d9b..e5442cbc0 100644 --- a/metrics/cloudwatch/cloudwatch_test.go +++ b/metrics/cloudwatch/cloudwatch_test.go @@ -161,10 +161,10 @@ func TestHistogram(t *testing.T) { svc := newMockCloudWatch() cw := New(namespace, svc, WithLogger(log.NewNopLogger())) histogram := cw.NewHistogram(name).With(label, value) - n50 := fmt.Sprintf("%s_p50", name) - n90 := fmt.Sprintf("%s_p90", name) - n95 := fmt.Sprintf("%s_p95", name) - n99 := fmt.Sprintf("%s_p99", name) + n50 := fmt.Sprintf("%s_50", name) + n90 := fmt.Sprintf("%s_90", name) + n95 := fmt.Sprintf("%s_95", name) + n99 := fmt.Sprintf("%s_99", name) quantiles := func() (p50, p90, p95, p99 float64) { err := cw.Send() if err != nil {