Conversation
|
I imagine the best way to test it would be to make a mock cloudwatchiface.CloudWatchAPI in the test, have it accept the writes, and compare output. edit: (ಠ_ಠ) |
|
An approach could be to have a slimmed down interface locally which also makes it explicit what part of the Cloudwatch API the code depends on. Currently only |
|
@peterbourgon I did in fact use the interface earlier today. It's all there. |
|
I'm happy with where this PR is at. Would love a review. |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
The basic structure is good! A few small things to fix up and then LGTM.
| "github.com/aws/aws-sdk-go/service/cloudwatch/cloudwatchiface" | ||
| "github.com/go-kit/kit/log" | ||
| "github.com/go-kit/kit/metrics" | ||
| "github.com/go-kit/kit/metrics/generic" |
There was a problem hiding this comment.
Import grouping here.
import (
// stdlib
// third-party
// go-kit/kit/...
)
metrics/cloudwatch/cloudwatch.go
Outdated
| // New returns a CloudWatch object that may be used to create metrics. Namespace is | ||
| // applied to all created metrics and maps to the CloudWatch namespace. | ||
| // Callers must ensure that regular calls to Send are performed, either manually or with one of the helper methods. | ||
| func New(namespace string, logger log.Logger, svc cloudwatchiface.CloudWatchAPI) *CloudWatch { |
There was a problem hiding this comment.
The logger is typically the last parameter in the constructor.
metrics/cloudwatch/cloudwatch.go
Outdated
| } | ||
|
|
||
| // NewCounter returns a new usable counter metric. | ||
| func NewCounter(name string) *Counter { |
There was a problem hiding this comment.
It seems the only "correct" way to construct a new {Counter, Gauge, Histogram} is via the corresponding method on the CloudWatch struct. Am I reading this correctly? If so, should these free functions be eliminated?
There was a problem hiding this comment.
Sure, I followed the pattern from one of the other metric packages. I'll clean this up.
| "github.com/aws/aws-sdk-go/service/cloudwatch/cloudwatchiface" | ||
| "github.com/go-kit/kit/log" | ||
| "github.com/go-kit/kit/metrics/teststat" | ||
| ) |
There was a problem hiding this comment.
Import grouping, as above.
metrics/cloudwatch/cloudwatch.go
Outdated
| } | ||
| } | ||
|
|
||
| // Send will fire an api request to CloudWatch with the latest stats for |
metrics/cloudwatch/cloudwatch.go
Outdated
| } | ||
|
|
||
| // Send will fire an api request to CloudWatch with the latest stats for | ||
| // all metrics. |
There was a problem hiding this comment.
Perhaps a note that most users will want to call WriteLoop, instead? Or — better yet — maybe you can unexport this method?
There was a problem hiding this comment.
I thought it might be worth keeping this around as someone might have their own way they want to fire the api call.
There was a problem hiding this comment.
Yep, totally fair, just some guidance in the comment would be good.
metrics/cloudwatch/cloudwatch.go
Outdated
| // WriteLoop is a helper method that invokes Send every | ||
| // time the passed channel fires. This method blocks until the channel is | ||
| // closed, so clients probably want to run it in its own goroutine. For typical | ||
| // usage, create a time.Ticker and pass its C channel to this method. |
| cw.counters[name] = c | ||
| cw.mtx.Unlock() | ||
| return c | ||
| } |
There was a problem hiding this comment.
cw.mtx.Lock()
defer cw.mtx.Unlock()
c := &counter{...} // assuming you remove the free constructor
cw.counters[name] = c
return c
metrics/cloudwatch/cloudwatch.go
Outdated
| defer cw.mtx.RUnlock() | ||
| now := time.Now() | ||
|
|
||
| datums := []*cloudwatch.MetricDatum{} |
There was a problem hiding this comment.
I slightly prefer
var datums []*cloudwatch.MetricDatumbut this is a tiny nit.
|
Oh! And can you also please add CloudWatch to the appropriate table/s in the documentation in the parent directory? |
|
The only thing I haven't done is add it to the table in documentation. I don't quite understand the table and haven't yet had enough time to read into it. |
|
Mentioned this in Slack but just for the record:
|
|
Sorted. Thanks for clearing that up. |
|
Amazing, thank you so much! |
Cloudwatch metrics
3 participants
Ref #489.
I'm reasonably happy with the current implementation, but I am having some trouble working out the best way to test the writing of metrics. Going to spend some time investigating ways to run integration tests for this.
CloudWatch charges per API request so we batch the requests for all metrics at once.