Make sure Cloudwatch-metrics only send the *new* counter values. #591
Conversation
feliksik
force-pushed
the
bug/cloudwatch-reset-counter
branch
3 times, most recently
from
158ebc0 to
cd077d8
Compare
feliksik
force-pushed
the
bug/cloudwatch-reset-counter
branch
3 times, most recently
from
847ba2b to
9aa8e8e
Compare
… does. Note there is a breaking API change, as the cloudwatch object now has optional parameters.
…r() did not add any samples.
feliksik
force-pushed
the
bug/cloudwatch-reset-counter
branch
from
9aa8e8e to
1f238bf
Compare
metrics/cloudwatch/cloudwatch.go
Outdated
| logger log.Logger | ||
| numConcurrentRequests int | ||
| } |
There was a problem hiding this comment.
Why is this a separate struct embedded into the type? I'd prefer to see these things flattened into the parent.
There was a problem hiding this comment.
I see that; I will change it accordingly.
metrics/cloudwatch/cloudwatch.go
Outdated
| } | ||
| o.percentiles = validated | ||
| } | ||
| } |
There was a problem hiding this comment.
No other metrics package has user-modifiable percentiles. Why is it important here?
There was a problem hiding this comment.
The rationale is that in the way it's used here, you have an extra metric for each percentile. That costs $50 cents/month. If you want only the 50th and 95th percentile, you'd pay twice as much when the 90th and 99th are logged, too.
For many metrics, this makes a difference. I hope that fact that it's configurable, makes this unobtrusive?
|
@cam-stitt do you have any comments on this PR? |
|
I'm not sure what other metric packages in go-kit use the "p" approach, but if @peterbourgon is okay with it, it doesn't bother me. It would probably cause anyone that has been using this package to adjust their dashboards/alerts accordingly though. |
|
Graphite seems to use a p prefix; influx to, but created a separate tag, without the metric name. The others I don't understand the semantics of the prepared messages :-) |
Testcase and fix for issue #590 .
The commits are separated and documented.
I only implemented the failing test in cloudwatch_test.go, instead of in teststat.TestCounter(), as it's not entirely clear to me if we can generalize the expected behavior for all methods. In particular, as the Send() method for e.g. Cloudwatch/Influx/Statsdogd is not implementing a generic interface, the semantics for each such method, and whether it should reset counters, may be defined differently (see #578 for an example of funkyness).
todos:
[ ] determine if this is actually a bug (I believe so)
[ ] determine whether the failing test case should be defined in the cloudwatch_test.go or in the teststat.TestCounter() method.
[ ] fix the functionality in cloudwatch.go and other affected pieces.