From 82e3a9df2ab3e7228871f0b35c80ceb1d2240b26 Mon Sep 17 00:00:00 2001 From: Tom Haigh Date: Wed, 8 May 2019 09:42:29 +0100 Subject: [PATCH] Reduce AWS sdk calls (#289) * Cache cloudformation calls * Don't check topics exist before creating * Newer queue data source wraps first To avoid duplicating fetches * ioc lifecycle changes for caching * remove duplicate ioc registration * remove unused method * formatting * Revert "remove duplicate ioc registration" This reverts commit ac9a4cd7d622228ea007d6749f6cc0424d2a48b7. --- .../Services/Sqs/SqsServiceSourceTests.cs | 2 +- .../Services/Sqs/QueueDataV2Source.cs | 50 ++----------------- .../Sns/SnsTopicCreatorTests.cs | 35 ++----------- .../Generic/CloudformationStackDeployer.cs | 11 +++- Watchman.Engine/Sns/SnsCreator.cs | 11 ---- Watchman.Engine/Sns/SnsTopicCreator.cs | 10 ++-- .../CloudFormationDeploymentTests.cs | 43 ++++++++++++++++ Watchman.Tests/Fakes/FakeCloudFormation.cs | 2 + Watchman/IoC/ApplicationRegistry.cs | 10 +++- 9 files changed, 75 insertions(+), 99 deletions(-) diff --git a/Watchman.AwsResources.Tests/Services/Sqs/SqsServiceSourceTests.cs b/Watchman.AwsResources.Tests/Services/Sqs/SqsServiceSourceTests.cs index 868ecda1..eb83734e 100644 --- a/Watchman.AwsResources.Tests/Services/Sqs/SqsServiceSourceTests.cs +++ b/Watchman.AwsResources.Tests/Services/Sqs/SqsServiceSourceTests.cs @@ -115,7 +115,7 @@ public void Setup() .ReturnsAsync(_thirdPage); - _queueSource = new QueueDataV2Source(cloudWatchMock.Object); + _queueSource = new QueueDataV2Source(new QueueSource(cloudWatchMock.Object)); } [Test] diff --git a/Watchman.AwsResources/Services/Sqs/QueueDataV2Source.cs b/Watchman.AwsResources/Services/Sqs/QueueDataV2Source.cs index cc268472..601de223 100644 --- a/Watchman.AwsResources/Services/Sqs/QueueDataV2Source.cs +++ b/Watchman.AwsResources/Services/Sqs/QueueDataV2Source.cs @@ -8,56 +8,16 @@ namespace Watchman.AwsResources.Services.Sqs { public class QueueDataV2Source : ResourceSourceBase { - private readonly IAmazonCloudWatch _amazonCloudWatch; + private readonly QueueSource _queueSource; - public QueueDataV2Source(IAmazonCloudWatch amazonCloudWatch) + public QueueDataV2Source(QueueSource queueSource) { - _amazonCloudWatch = amazonCloudWatch; + _queueSource = queueSource; } - private async Task> ReadActiveQueueNames() + private Task> ReadActiveQueueNames() { - var metrics = await ReadQueueMetrics(); - - return metrics - .SelectMany(ExtractQueueNames) - .OrderBy(qn => qn) - .Distinct() - .ToList(); - } - - private async Task> ReadQueueMetrics() - { - var metrics = new List(); - string token = null; - do - { - var request = new ListMetricsRequest - { - MetricName = "ApproximateAgeOfOldestMessage", - NextToken = token - }; - var response = await _amazonCloudWatch.ListMetricsAsync(request); - - if (response != null) - { - token = response.NextToken; - metrics.AddRange(response.Metrics); - } - else - { - token = null; - } - } while (token != null); - - return metrics; - } - - private static IEnumerable ExtractQueueNames(Metric metric) - { - return metric.Dimensions - .Where(d => d.Name == "QueueName") - .Select(d => d.Value); + return _queueSource.GetResourceNamesAsync(); } protected override string GetResourceName(QueueDataV2 resource) diff --git a/Watchman.Engine.Tests/Sns/SnsTopicCreatorTests.cs b/Watchman.Engine.Tests/Sns/SnsTopicCreatorTests.cs index a4fb009b..49430cd1 100644 --- a/Watchman.Engine.Tests/Sns/SnsTopicCreatorTests.cs +++ b/Watchman.Engine.Tests/Sns/SnsTopicCreatorTests.cs @@ -1,4 +1,4 @@ -using System.Threading; +using System.Threading; using Amazon.SimpleNotificationService; using Amazon.SimpleNotificationService.Model; using Moq; @@ -28,28 +28,7 @@ public async Task HappyPathShouldCreateTopic() client.Verify(c => c.CreateTopicAsync("test1-Alerts", It.IsAny()), Times.Once); } - - [Test] - public async Task WhenTopicExistsShouldNotCreateTopic() - { - var client = new Mock(); - client.Setup(c => c.FindTopicAsync(It.IsAny())) - .ReturnsAsync(TestFindTopicResponse()); - - MockCreateTopic(client,TestCreateTopicResponse()); - - var logger = new Mock(); - var snsTopicCreator = new SnsTopicCreator(client.Object, logger.Object); - - var topicArn = await snsTopicCreator.EnsureSnsTopic("test1", false); - - Assert.That(topicArn, Is.Not.Null); - Assert.That(topicArn, Is.EqualTo("existingTopicArn-abc-1234")); - - client.Verify(c => c.CreateTopicAsync(It.IsAny(), It.IsAny()), - Times.Never); - } - + [Test] public async Task DryRunShouldNotCreateTopic() { @@ -74,15 +53,7 @@ private static CreateTopicResponse TestCreateTopicResponse() TopicArn = "testResponse-abc123" }; } - - private Topic TestFindTopicResponse() - { - return new Topic - { - TopicArn = "existingTopicArn-abc-1234" - }; - } - + private void MockCreateTopic(Mock client, CreateTopicResponse response) { diff --git a/Watchman.Engine/Generation/Generic/CloudformationStackDeployer.cs b/Watchman.Engine/Generation/Generic/CloudformationStackDeployer.cs index 69870e31..434d1e0a 100644 --- a/Watchman.Engine/Generation/Generic/CloudformationStackDeployer.cs +++ b/Watchman.Engine/Generation/Generic/CloudformationStackDeployer.cs @@ -78,8 +78,15 @@ public CloudFormationStackDeployer( _s3Location = s3Location; } - private async Task> AllStacks() + private IReadOnlyList _stacks; + + private async Task> AllStacks() { + if (_stacks != null) + { + return _stacks; + } + string nextToken = null; var results = new List(); @@ -96,6 +103,8 @@ private async Task> AllStacks() results.AddRange(allStacks.StackSummaries.Select(x => x.StackName)); } while (nextToken != null); + _stacks = results; + return results; } diff --git a/Watchman.Engine/Sns/SnsCreator.cs b/Watchman.Engine/Sns/SnsCreator.cs index 572a3677..a1dda093 100644 --- a/Watchman.Engine/Sns/SnsCreator.cs +++ b/Watchman.Engine/Sns/SnsCreator.cs @@ -24,16 +24,5 @@ public async Task EnsureSnsTopic(AlertingGroup alertingGroup, bool dryRu } return snsTopicArn; } - - public async Task EnsureSnsTopic(IServiceAlertingGroup alertingGroup, bool dryRun) - { - var snsTopicArn = await _snsTopicCreator.EnsureSnsTopic(alertingGroup.GroupParameters.Name, dryRun); - - if (!dryRun) - { - await _snsSubscriptionCreator.EnsureSnsSubscriptions(alertingGroup.GroupParameters.Targets, snsTopicArn); - } - return snsTopicArn; - } } } diff --git a/Watchman.Engine/Sns/SnsTopicCreator.cs b/Watchman.Engine/Sns/SnsTopicCreator.cs index 1cc94008..c59569f5 100644 --- a/Watchman.Engine/Sns/SnsTopicCreator.cs +++ b/Watchman.Engine/Sns/SnsTopicCreator.cs @@ -19,13 +19,6 @@ public SnsTopicCreator(IAmazonSimpleNotificationService snsClient, public async Task EnsureSnsTopic(string alertingGroupName, bool dryRun) { var topicName = alertingGroupName + "-Alerts"; - var topic = await _snsClient.FindTopicAsync(topicName); - - if (topic != null) - { - _logger.Detail($"Found SNS topic {topicName} with ARN {topic.TopicArn}"); - return topic.TopicArn; - } if (dryRun) { @@ -33,6 +26,9 @@ public async Task EnsureSnsTopic(string alertingGroupName, bool dryRun) return topicName; } + // https://docs.aws.amazon.com/sns/latest/api/API_CreateTopic.html + // "This action is idempotent, so if the requester already owns a topic with the specified name, + // that topic's ARN is returned without creating a new topic." var createResponse = await _snsClient.CreateTopicAsync(topicName); _logger.Info($"Created SNS topic {topicName} with ARN {createResponse.TopicArn}"); return createResponse.TopicArn; diff --git a/Watchman.Tests/CloudFormationDeploymentTests.cs b/Watchman.Tests/CloudFormationDeploymentTests.cs index 84dc3dfa..35277eaa 100644 --- a/Watchman.Tests/CloudFormationDeploymentTests.cs +++ b/Watchman.Tests/CloudFormationDeploymentTests.cs @@ -275,5 +275,48 @@ await context.Get() Assert.That(count, Is.Not.LessThan(approxExpectedPerStack * 0.8)); } } + + [Test] + public async Task StacksAreNotListedMultipleTimes() + { + var config = ConfigHelper.CreateBasicConfiguration("test", "group-suffix", + new AlertingGroupServices() + { + AutoScaling = new AwsServiceAlarms() + { + Resources = new List>() + { + new ResourceThresholds() + { + Pattern = ".*" + } + } + } + }, + numberOfCloudFormationStacks: 5); + + var cloudFormation = new FakeCloudFormation(); + + var context = new TestingIocBootstrapper() + .WithCloudFormation(cloudFormation.Instance) + .WithConfig(config); + + context.GetMock().HasAutoScalingGroups(new[] + { + new AutoScalingGroup() + { + AutoScalingGroupName = $"group-asg", + DesiredCapacity = 40 + } + }); + + await context.Get() + .LoadAndGenerateAlarms(RunMode.GenerateAlarms); + + var stacks = cloudFormation.Stacks(); + + Assert.That(stacks.Count, Is.GreaterThan(1)); + Assert.That(cloudFormation.CallsToListStacks, Is.EqualTo(1)); + } } } diff --git a/Watchman.Tests/Fakes/FakeCloudFormation.cs b/Watchman.Tests/Fakes/FakeCloudFormation.cs index ece8dbfb..48b83802 100644 --- a/Watchman.Tests/Fakes/FakeCloudFormation.cs +++ b/Watchman.Tests/Fakes/FakeCloudFormation.cs @@ -14,6 +14,7 @@ class FakeCloudFormation private Mock fake = new Mock(); + public int CallsToListStacks { get; private set; } = 0; public IAmazonCloudFormation Instance => fake.Object; public FakeCloudFormation() @@ -45,6 +46,7 @@ public FakeCloudFormation() .ReturnsAsync(new UpdateStackResponse()); fake.Setup(x => x.ListStacksAsync(It.IsAny(), It.IsAny())) + .Callback(() => { CallsToListStacks++;}) .ReturnsAsync(() => new ListStacksResponse() { StackSummaries = _submitted diff --git a/Watchman/IoC/ApplicationRegistry.cs b/Watchman/IoC/ApplicationRegistry.cs index 2d61fe96..1cc587b5 100644 --- a/Watchman/IoC/ApplicationRegistry.cs +++ b/Watchman/IoC/ApplicationRegistry.cs @@ -29,8 +29,14 @@ public ApplicationRegistry(StartupParameters parameters) private void ConfigureResourceSources() { - For>().Use(); - For>().Use(); + // These cache table names etc + For>() + .Use() + .Singleton(); + + For>() + .Use() + .Singleton(); } private void ConfigureInternalDependencies(StartupParameters parameters)