cloudwatch: Consolidate client logic

[patstrom]

What this PR does / why we need it:
Consolidate logic for getting clients in CloudWatch data source. Also clean up the CloudWatch test suite.

Special notes for your reviewer:

This is part 1 of PRs to replace #23439

It is probably appropriate for @aknuds1 to review this

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[patstrom]

@aknuds1

[patstrom]

I had to remove the "caching" of EC2 and RGTA clients in order to unify the interface for fetching in the first place. The other alternative would've been to add a cwSvc cloudwatchiface.CloudWatchAPI field to the cloudwatchExecutor and a corresponding ensureCloudwatchClient function that does a nil-check and I felt that was the wrong way forward.

The Cloudwatch logs client cache I removed because it did a cloudwatchlogs-specific caching on a region basis. While that is what we want (eventually) I thought the proper way was to remove it for now in order to be able to unify the interface and then later on start caching the sessions on a region-basis instead. That way all the different clients can use the same cache-implementation.

So the idea here moving forward is that once there is a unified interface for fetching AWS clients we can move on to fix the now buggy cache by using the data source id as key and start caching the actual Session (on a region-basis) instead of Credentials, which will be a performance improvement.

[aknuds1]

@patstrom So if we ignore the reasons why you had to remove the caching in the first place, what would break if we re-introduce client caching (according to the original naive scheme) in this PR? I'm leaning towards re-introducing client caching in this PR, so we don't break anything and instead propose an improved(?) caching system in another PR.

I think technically, reintroducing client caching will be easy.

[patstrom]

The reasons for actually removing the caching in this PR is purely in order to de-clutter the code base. Re-introducing the very same ec2Svc, rgtaSvc and logsClientsByRegion and instead manipulating them in the corresponding "get" function shouldn't break anything when compared to the previous behavior (as far as I can tell).

Do note that all caching in this package is buggy though. Nothing is properly invalidated. E.g if you provide a working configuration, do some queries (and stuff is cached) and then update the configuration to something that shouldn't work, it still will (in some cases).

I guess it is a matter of taste if you want to remove it now or later (either when replacing them with something proper or just removing them to fix the bugs). Personally I felt that given that they are buggy and that there are two different client cache implementations I deemed it better to just remove them since they made the code more difficult to reason about.

[aknuds1]

Thanks for clarifying that @patstrom. It sounds as if I might just code up a reintroduction of caching for this PR, to keep the current scheme (for the time being?), unless my colleagues think differently.

[mtanda]

@mtanda Do you know why CloudWatch logs, EC2 and RGTA clients are cached (while the CloudWatch client is not)? FYI, this PR proposes to drop the caching of clients.

I didn't intend to cache client, I need to use mock client for test code.
5255c9c
(Sorry, I didn't read discussion)

[aknuds1]

@mtanda Aha, thanks for clarifying that.

[aknuds1]

@patstrom I've re-introduced client caching in this branch. I also removed initialization of cloudWatchExecutor.DataSource when it gets instantiated, since I realized it always gets initialized in the Query method anyway. Also fixed a merge conflict. Please look my changes over and merge them into your branch, or let me know if you see any problems.

[aknuds1]

LGTM now, but there's a remaining minor merge conflict. I've fixed it in this branch.

[aknuds1]

Thank you for contributing to Grafana!

[patstrom]

Cheers! 🎉

Do you want me to update the other MRs to be based on this instead?

[aknuds1]

@patstrom Just base them (rebase, merge, whatever) on master, since this PR has been merged. We'll evaluate them as we find the time.

[patstrom]

Ah yeah that's what I meant 😅