Skip to content

Add cloudwatch metrics auto-configuration with aws sdk v2 #237

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

Merged
merged 7 commits into from
Jun 27, 2022

Conversation

eddumelendez
Copy link
Contributor

As part of the effort to move to aws-sdk-v2. Metrics has been added using
micrometer-registry-cloudwatch2 as a auto-configuration.

@maciejwalkowiak maciejwalkowiak added this to the 3.0.0 M1 milestone Apr 2, 2022
@maciejwalkowiak maciejwalkowiak added component: cloudwatch CloudWatch integration related issue type: feature Integration with a new AWS service or bigger change in existing integration labels Apr 2, 2022
As part of the effort to move to aws-sdk-v2. Metrics has been added using
`micrometer-registry-cloudwatch2` as a auto-configuration.
@eddumelendez eddumelendez changed the title Add metrics to v3 Add cloudwatch metrics auto-configuration with aws sdk v2 Apr 19, 2022
* @since 2.0.0
*/
@ConfigurationProperties(prefix = "management.metrics.export.cloudwatch")
public class CloudWatchProperties extends StepRegistryProperties {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency, shouldn't it bind to spring.cloud.aws.cloudwatch and extends AwsClientProperties? Similar to how Stackdriver is done in GCP, where StackdriverPropertiesConfigAdapter adapts GCP properties to Micrometer.

@maciejwalkowiak
Copy link
Contributor

I am not sure if this is how its done in Stackdriver integration in GCP. As far as I can see, in Spring Cloud GCP client can be configured with spring.cloud.gcp.metrics (https://docs.spring.io/spring-cloud-gcp/docs/current/reference/html/index.html#spring-data-cloud-spanner) but Micrometer related properties go to management.metrics.export.stackdriver.

Shouldn't we have the same for Spring Cloud AWS? The benefit of configuring client through spring.cloud.aws.cloudwatch would be ability for CloudWatchProperties to extends AwsClientProperties which could be then configured with AwsClientBuilderConfigurer.

@eddumelendez
Copy link
Contributor Author

Spring boot provides auto-configuration for stackdriver and other but not for cloudwatch v1 nor v2 and management.metrics.export.stackdriver works for those actuator auto-configurations. In GCP case, it the auto-configuration provided by gcp executes before the one for spring-boot, providing the information needed.

This PR follows the the current implementation in previous version but I was also thinking about give the shape similar to what GCP does but then I realized that we will be providing two auto-configurations in the same place, one similar to the one that provides spring boot for stackdriver and other similar to the one that spring-cloud-gcp does. I would prefer to keep it as previous version on spring-cloud aws because everything is already powered by spring boot. Let me know your thought on this.

@maciejwalkowiak
Copy link
Contributor

Do you know why Spring Cloud GCP provides its own Stackdriver autoconfiguration if its already covered in Spring Boot?

I think it would be nice that CloudWatchProperties extend AwsClientProperties so that we can have same ways to configure all AWS clients properties.

@eddumelendez
Copy link
Contributor Author

spring boot stackdriver auto-configuration needs a project id and credentials to be setup manually meanwhile spring cloud gcp does that automatically taking into account global properties for the attributes mentioned at the beginning.

CloudWatchPropeties doesn't extend AwsProperties because it already extends StepRegistryProperties but region and endpoint are already provided in order to keep consistency with other autoconfigurations.

- add nullability annotations
- separate client properties from metrics properties
- support global endpoint configuration
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 19 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@maciejwalkowiak
Copy link
Contributor

Look at the commit I've added as i think its easier to discuss the actual code - when client properties are separated from metrics properties, we can use AwsClientBuilderConfigurer and keep all clients configuration consistent - not saying that it must be like that, but I don't see any drawbacks.

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.0.0 M1, 3.0.0 M2 Jun 8, 2022
@maciejwalkowiak maciejwalkowiak mentioned this pull request Jun 8, 2022
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
3.2% 3.2% Duplication

@maciejwalkowiak maciejwalkowiak merged commit 225ec81 into awspring:main Jun 27, 2022
@maciejwalkowiak
Copy link
Contributor

Thanks @eddumelendez. Docs are pending but to avoid efforts for keeping this branch up to date we can add docs in separate PR, just has to be done before we release M2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: cloudwatch CloudWatch integration related issue type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants