Skip to content
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

Migration of registry-cloudwatch to AWS SDK 2.x #1165

Merged
merged 2 commits into from
Jun 26, 2019

Conversation

PyvesB
Copy link
Contributor

@PyvesB PyvesB commented Jan 17, 2019

The AWS SDK for Java 2.0 reached general availability back in November (more information on their official blog post) and has de facto become the preferred way of integrating with AWS using Java.

Amongst other benefits, using it rather than the old version improves performance and allows to write cleaner code.

This pull request migrates the registry-cloudwatch implementation to the new SDK.

@shakuzen
Copy link
Member

This would break all existing users of the CloudWatchMeterRegistry due to the constructor change. I don't think we want to do that in a minor release. Maybe it is better to make a separate module using the new AWS SDK v2 and mark the current one as deprecated for removal in our next major version.

@PyvesB
Copy link
Contributor Author

PyvesB commented Jan 31, 2019

Fair point. However, this would likely lead to some duplicated code. Is this temporary stateacceptable in your opinion, given that the old module will be deleted sometime in the future anyway? Or should we invest time in creating some sort of micrometer-registry-cloudwatch-common sub-project to extract as much common code as possible, used both by the micrometer-registry-cloudwatch and micrometer-registry-cloudwatch-v2 modules? Ideally, we would have to refactor again when the old module is deleted though.

@shakuzen
Copy link
Member

I think duplicating the code will probably be simplest. We will just need to be careful that any bug fixes in one get ported to the other while we are supporting both.

@PyvesB
Copy link
Contributor Author

PyvesB commented Feb 1, 2019

Done. I called the new module micrometer-registry-cloudwatch2 to follow the same naming convention as with the Jersey implementation.

@shakuzen shakuzen added this to the 1.2.0 milestone Feb 4, 2019
@shakuzen shakuzen added the enhancement A general enhancement label Feb 4, 2019
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new module should use a different package (most simply io.micrometer.cloudwatch2), but I can change this post merge. Thank you for contributing this.

@shakuzen shakuzen merged commit 52e52c7 into micrometer-metrics:master Jun 26, 2019
@shakuzen shakuzen added the registry: cloudwatch A CloudWatch Registry related issue label Aug 8, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement registry: cloudwatch A CloudWatch Registry related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants