New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudWatchReporter is tightly coupled with ScheduledReporter #20

Open
jlhood opened this Issue Dec 3, 2016 · 1 comment

Comments

Projects
None yet
1 participant
@jlhood

jlhood commented Dec 3, 2016

CloudWatchReporter has a lot of great functionality, like the dimension support, but it currently extends ScheduledReporter, so there's limited control over when metrics are published to CloudWatch. I'd like to break out the functionality to publish metrics to CloudWatch into a separate class that can be composed with a ScheduledReporter subclass, but also used standalone.

That way I can use the standalone class to report metrics on a per-request basis. I need this for a serverless application I'm working on since I can't rely on the container continuing to exist and would instead like to create a new MetricRegistry for each request and then publish any metrics collected at the completion of that request.

I've forked the repo and intend to make this change myself and send a PR, but I'm opening this issue to discuss the best way to do this.

@jlhood

This comment has been minimized.

Show comment
Hide comment
@jlhood

jlhood Dec 3, 2016

Ideally I'd just change CloudWatchReporter to no longer extend ScheduledReporter and just implement Reporter and work in a standalone way. Then I'd create a ScheduledCloudWatchReporter object that extends ScheduledReporter and have it take a CloudWatchReporter in its constructor and delegate to it in the report() override method.

However that change would not be backwards-compatible so that wouldn't be something I could send a PR for. A backwards-compatible approach would be to create a class like NonScheduledCloudWatchReporter or AdhocCloudWatchReporter (better name suggestions would be appreciated) and move most of the CloudWatchReporter logic there. Then update CloudWatchReporter to take that class in its constructor so it continues to work.

I'm going to code up that approach and send a PR when I have it working. Just wanted to post my thoughts on it since I'm new to the codebase. If you have comments or suggestions, or there's something I'm missing, let me know. Thanks!

jlhood commented Dec 3, 2016

Ideally I'd just change CloudWatchReporter to no longer extend ScheduledReporter and just implement Reporter and work in a standalone way. Then I'd create a ScheduledCloudWatchReporter object that extends ScheduledReporter and have it take a CloudWatchReporter in its constructor and delegate to it in the report() override method.

However that change would not be backwards-compatible so that wouldn't be something I could send a PR for. A backwards-compatible approach would be to create a class like NonScheduledCloudWatchReporter or AdhocCloudWatchReporter (better name suggestions would be appreciated) and move most of the CloudWatchReporter logic there. Then update CloudWatchReporter to take that class in its constructor so it continues to work.

I'm going to code up that approach and send a PR when I have it working. Just wanted to post my thoughts on it since I'm new to the codebase. If you have comments or suggestions, or there's something I'm missing, let me know. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment