Auto-configure CloudWatch metrics publisher

[maciejwalkowiak]

When cloudwatch-metric-publisher is in the classpath, SDK clients should be configured to publish metrics to CloudWatch.
There should be an easy way to opt-out from it but still keep the dependency in the classpath.

https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/metrics.html

[krimsz]

Hey @maciejwalkowiak I was taking a look at this today, there are a couple of unclear points on how to do this. I've done a small PoC around here and I wanted to see if the design would work or if I should pivot a bit. Tests are not changed for this and docs/comments are lacking still, just wanna see if this would make sense.
https://github.com/krimsz/spring-cloud-aws/tree/cloudwatch-publisher-opt-in

Some points to be considered:

1. I assumed a single Bean for all the clients. Should we also support 1 bean per client? if so, need a bit more clarity on what properties would trigger the creation of a different bean
2. There are 2 ways to disable the MetricsPublisher, by passing a global property spring.cloud.aws.metrics-enabled or under each of the individual clients e.g. spring.cloud.aws.s3.metrics-enabled.
   • The behaviour is activated by default (if the dependency is found) and disabled on-demand.
3. There are some properties we can play with within the MetricsPublisher, (e.g. uploadFrequency, metricLevel, maximumCallsPerUpload and more). Would it be something we would like to configure those? I'm saying this since some might have an impact on the cost if configured wrongly (stated in the docs from the awssdk in the dimensions for example) I would let the Bean with the default values and let the user define their own Bean if needed
4. Take a look as I changed the io.awspring.cloud.core.SpringCloudClientConfiguration from the core package to expos the builder rather than the overrideConfiguration. This is so I can manipulate it before constructing the client. We could look into leaving it as-is if needed and use the toBuilder() method, although it would mean re-creating the object.

P.S. Still figuring out the AbstractAwsConfigDataLocationResolvers

[maciejwalkowiak]

Thanks @krimsz! Looks fantastic so far!

1. Single bean for all clients is good enough. When someone needs another configuration per client they can always overwrite client - we should not prepare auto configurations for all edge cases.
2. Sounds good!
3. Yes! Perhaps there should be spring.cloud.aws.client.metrics.enabled, spring.cloud.aws.client.metrics.upload-frequency etc.
4. No problem with that I don't think this is an expensive object to create.

AbstractAwsConfigDataLocationResolvers are always a bit tricky and I haven't figured out yet whats the best way to do it. I think what you're doing is the way to go, we can always refactor later. Note that Spring has org.springframework.util.ClassUtils.isPresent(..) which perhaps makes it slightly less verbose. We should also minimize number of times we call it so perhaps better to keep it in a static constant.

@MatejNedic @eddumelendez perhaps you would also like to take a look

[maciejwalkowiak]

@krimsz do you think you'll be able to finish it?

[antonpperez]

Hey there, thanks for tagging me, at first I was waiting for someone to reply to you but then I kinda forgot about this.
Next week I might be able to spend some time on this.
@eddumelendez does it mean you already did this or were you commenting about my partial solution?

[eddumelendez]

ignore my comment. I confused it 😬