Metrics - Cloudwatch v2 #668
Conversation
metrics/cloudwatch2/cloudwatch2.go
Outdated
|
|
||
| type option func(*CloudWatch) | ||
|
|
||
| func (cw *CloudWatch) apply(opt option) { |
There was a problem hiding this comment.
Ahh, yup. That's what I get for doin' a copypasta.
metrics/cloudwatch2/cloudwatch2.go
Outdated
| svc: svc, | ||
| counters: lv.NewSpace(), | ||
| numConcurrentRequests: 10, | ||
| logger: log.NewLogfmtLogger(os.Stderr), |
There was a problem hiding this comment.
default should be NewNopLogger
metrics/cloudwatch2/cloudwatch2.go
Outdated
| // manually or with one of the helper methods. | ||
| func New(namespace string, svc cloudwatchiface.CloudWatchAPI, options ...option) *CloudWatch { | ||
| cw := &CloudWatch{ | ||
| sem: nil, // set below |
There was a problem hiding this comment.
Should this field be omitted since it's set below?
metrics/cloudwatch2/cloudwatch2.go
Outdated
| } | ||
|
|
||
| var zero = float64(0.0) | ||
| var zeros = cloudwatch.StatisticSet{ |
There was a problem hiding this comment.
this is only used once, any reason it should be a package var?
There was a problem hiding this comment.
My intent was to avoid paying the "construction costs" every time a zero set is needed.
There was a problem hiding this comment.
... Should I do something different to make that apparent?
There was a problem hiding this comment.
Maybe just a light comment could work.
metrics/cloudwatch2/cloudwatch2.go
Outdated
|
|
||
| var errors = make(chan error, len(batches)) | ||
| for _, batch := range batches { | ||
| go func(batch []cloudwatch.MetricDatum) { |
There was a problem hiding this comment.
the goroutines could run as an errorgroup
There was a problem hiding this comment.
Oooh, cool! I hadn't yet added errorgroup to my toolbelt. Added!
|
@peterbourgon How are you feeling about this PR? |
metrics/cloudwatch2/cloudwatch2.go
Outdated
| numConcurrentRequests int | ||
| } | ||
|
|
||
| type option func(*CloudWatch) |
There was a problem hiding this comment.
Since this is returned by exported functions, it's nicer to also make it exported. (And commented, etc.)
metrics/cloudwatch2/cloudwatch2.go
Outdated
|
|
||
| // WithLogger sets the Logger that will recieve error messages generated | ||
| // during the WriteLoop | ||
| func WithLogger(logger log.Logger) option { |
There was a problem hiding this comment.
I find it's nice if functional options describe the default behavior if the option isn't invoked. In this case, it might be nice to add: "By default, no logger is used."
metrics/cloudwatch2/cloudwatch2.go
Outdated
| // WithConcurrentRequests sets the upper limit on how many | ||
| // cloudwatch.PutMetricDataRequest may be under way at any | ||
| // given time. If n is greater than 20, 20 is used. | ||
| func WithConcurrentRequests(n int) option { |
There was a problem hiding this comment.
Similarly: "By default, 10 concurrent requests are used."
|
|
||
| "github.com/aws/aws-sdk-go-v2/aws" | ||
| "github.com/aws/aws-sdk-go-v2/service/cloudwatch" | ||
| "github.com/aws/aws-sdk-go-v2/service/cloudwatch/cloudwatchiface" |
There was a problem hiding this comment.
package cloudwatchiface 🤦♂️
There was a problem hiding this comment.
Yah. It's supposed to be in support of mocking out their service... But it feels so icky.
|
|
||
| func makeDimensions(labelValues ...string) []cloudwatch.Dimension { | ||
| dimensions := make([]cloudwatch.Dimension, len(labelValues)/2) | ||
| for i, j := 0, 0; i < len(labelValues); i, j = i+2, j+1 { |
There was a problem hiding this comment.
I can't take credit for this... I lifted this directly from the existing cloudwatch package.
metrics/convert/convert.go
Outdated
| @@ -0,0 +1,135 @@ | |||
| // Package convert provides a way to use Counters, Histograms, or Gauges | |||
| // as one of the other types | |||
| package convert | |||
There was a problem hiding this comment.
It feels like this is tailor-made to cloudwatch2, and I'm a little bit scared that folks will see it and think it's something they should be doing in their own code. Would you be OK with moving this underneath the internal/ dir?
| } | ||
| } | ||
|
|
||
| type mockCloudWatch struct { |
metrics/convert/convert_test.go
Outdated
| h := NewCounterAsHistogram(c) | ||
| top := NewHistogramAsCounter(h).With("label", "counter").(histogramCounter) | ||
| mid := top.h.(counterHistogram) | ||
| low := mid.c.(*generic.Counter) |
|
@peterbourgon Back atcha! 😄 |
|
Nice! Thanks! |
Per the conversation in the Slack channel: https://gophers.slack.com/archives/C04S3T99A/p1519410405000323
I have created the
metrics/cloudwatch2package.This package targets using the V2 SDK (https://github.com/aws/aws-sdk-go-v2). It also opts for using the CloudWatch native client-side aggregation into "StatisticsSet" -
https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_StatisticSet.html. (Whereas the existing
metrics/cloudwatchpackage does addition lossy aggregations and just emits a single "Value".) Also, as per CloudWatch idiom, Counters / Histograms / Gauges are all sent to CloudWatch in exactly the same way.(I mostly cribbed from the existing
cloudwatchpackage, simplified the work it does, and touched up around the edges to adjust to the delta in API between v1 and v2 versions of the AWS SDK.)Since Histograms/Gauges all reduce down to the exact same behavior as Counters, I created the
convertpackage to easily swap in Histogram/Gauge API semantics. I made it its own package in case any other metrics systems have a need for similar generification... Though if this is an overshoot, I could dumb that down and just isolate it into thecloudwatch2package.