Raise an alarm when any stages are in a Failed state. #6

sam-goodwin · 2018-12-13T04:32:59Z

A function runs every minute and calls GetPipelineState for the provided pipeline's name, counts the number of failed stages and emits a JSON log { failedCount: }. A metric filter is then configured to track this value as a CloudWatch metric, and a corresponding alarm is set to fire when the maximim value of a single 5-minute interval is >= 1.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…or the number of failed stages, enabling consistent alarming on a 'failed pipeline'.

eladb

Can you please add a unit test?

lib/pipeline-watcher.ts

lib/pipeline.ts

eladb · 2018-12-13T04:44:28Z

lib/pipeline.ts

+      .addResource(props.pipeline.pipelineArn)
+      .addAction('codepipeline:GetPipelineState'));
+
+    // Explicitly and pre-emptively create the function's log group because we need it for the metric filter.


There's a race condition here between Lambda and CloudFormation. I remember @rix0rrr had to create a custom resource to implement something like that and I wonder if that's the case, if we should actually include this in the Lambda construct library.

I can address this by having the EventRule trigger depend on the LogGroup Resource. This ensures the log group is created before the first invocation of the function. Lambda lazily creates the log group on first invocation, so no more race condition. The only down-side I can see is that I have to drop down to L1 and rely on the findChild(‘Resource’) convention.

const logGroupResource = logGroup.findChild('Resource') as cdk.Resource; const triggerResource = trigger.findChild('Resource') as cdk.Resource; triggerResource.addDependency(logGroupResource);

I guess...

We should try to find a better way to allow defining dependencies between resources (aws/aws-cdk#95)

We have an issue to try and make log groups accessible in the CDK (aws/aws-cdk#667).

Another alternative is that I have the handler call putMetricData directly instead of using a MetricFilter. Let me know what you think so I can wrap this up.

I think the metric filter route is the right one, just want to make sure we have a better path in the CDK for future generations to use it. Tactically, your solution for delivlib is perfectly fine.

lib/pipeline.ts

… Fix style and FnConcat comments.

eladb · 2018-12-13T06:49:06Z

lib/pipeline-watcher/watcher-function.ts

@@ -0,0 +1,29 @@
+import AWS = require('aws-sdk');


Sorry for the nit picking, but I'd call this "watcher-handler"

eladb · 2018-12-13T06:49:44Z

lib/pipeline-watcher/watcher-function.ts

+      .length;
+  }
+  process.stdout.write(JSON.stringify({failedCount}));
+}


Can we write a simple unit test for this handler? (I know we don't have any UT for handlers in this library, yet, but let's raise the bar)

Working on it .. it's a pain since we use nodeunit (deprecated) and most of what the handler does is create a client, check env variables, make a request and log to stdout - all of which are annoying to test, ha.

See #5 - I don't think we need to use nodeunit here. Moving to jest?

eladb · 2018-12-13T06:50:43Z

lib/pipeline.ts

@@ -6,6 +6,7 @@ import iam = require('@aws-cdk/aws-iam');
 import sns = require('@aws-cdk/aws-sns');
 import cdk = require('@aws-cdk/cdk');
 import path = require('path');
+import { PipelineWatcher } from './pipeline-watcher/watcher';


Create an pipeline-watcher/index.ts file and export * from './watcher', so here it will be:

import { PipelineWatcher } from './pipeline-watcher'

eladb · 2018-12-13T06:51:08Z

lib/pipeline-watcher/watcher.ts

+    });
+
+    this.alarm = new cloudwatch.Alarm(this, 'Alarm', {
+      alarmDescription: `Pipeline ${props.title || props.pipeline.pipelineName} has failed failed stages`,


"failed failed"

eladb · 2018-12-13T06:52:57Z

lib/pipeline-watcher/watcher.ts

+    // TODO: This creates a long namespace, better alternatives?
+    const metricNamespace =  `CodePipeline/${props.pipeline.pipelineName}`;
+
+    new logs.MetricFilter(this, 'MetricFilter', {


You can probably use logGroup.extractMetric here.

Tried, it doesn't support namespaces with a / or token values. Cut an issue: aws/aws-cdk#1351

I don't think this namespace is appropriate. Generally this would contain the service name, like AWS/SQS or AWS/DynamoDB. Variables go into Dimensions. So in this case, I would expect it to be CDK/Delivlib or similar.

Can't use dimensions in metric filters unfortunately.

I could instead make the metric name ugly instead of the namespace:

Namespace: CDK/Delivlib

MetricName: <pipeline-name>_FailedStages

lib/pipeline-watcher/watcher.ts

lib/pipeline-watcher/index.ts

sam-goodwin · 2018-12-13T09:33:13Z

lib/pipeline-watcher/watcher.ts

+
+    const metricName = 'FailedStages';
+    // TODO: This creates a long namespace, better alternatives?
+    const metricNamespace =  `CodePipeline/${props.pipeline.pipelineName}`;


@rix0rrr I've called it out in the code here. Any other suggestions for distinguishing individual pipeline's metrics from each other? I can't use dimensions in metric filters :(

* **pipeline**: concurrency limit ([#9](#9)) * **gh-pages-publisher**: force-push without history ([#7](#7)) * **pipeline**: send email notifications on any action failure ([#10](#10)) * **github-releases**: if changelog doesn't exist, don't include release notes ([#8](#8)) * **pipeline**: raise an alarm when any stages are in a Failed state ([#6](#6))

Sam Goodwin added 2 commits December 12, 2018 17:11

Configure a lambda function to watch the pipeline and emit a metric f…

f7b23b3

…or the number of failed stages, enabling consistent alarming on a 'failed pipeline'.

Remove samgood test names of stacks

7fe45e6

eladb suggested changes Dec 13, 2018

View reviewed changes

Sam Goodwin added 2 commits December 12, 2018 21:58

Move pipeline watcher construct and functio to lib/pipeline-watcher/.…

5ae888f

… Fix style and FnConcat comments.

Update expected.json

8429776

eladb reviewed Dec 13, 2018

View reviewed changes

Add unit tests, refactor names as requested.

9bd96f5

eladb approved these changes Dec 13, 2018

View reviewed changes

lib/pipeline-watcher/index.ts Outdated Show resolved Hide resolved

sam-goodwin commented Dec 13, 2018

View reviewed changes

Sam Goodwin added 2 commits December 13, 2018 01:37

Don't export the watcher handler and remove 'failed failed'

88c5cc4

Use pipeline name in metric name instead of namespace

9d5d04e

sam-goodwin merged commit ce744c0 into master Dec 13, 2018

eladb mentioned this pull request Dec 13, 2018

v0.2.0 #11

Merged

sam-goodwin deleted the samgood/alarming branch December 17, 2018 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise an alarm when any stages are in a Failed state. #6

Raise an alarm when any stages are in a Failed state. #6

sam-goodwin commented Dec 13, 2018

eladb left a comment

eladb Dec 13, 2018

sam-goodwin Dec 13, 2018

eladb Dec 13, 2018

sam-goodwin Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

sam-goodwin Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

sam-goodwin Dec 13, 2018

rix0rrr Dec 13, 2018

sam-goodwin Dec 13, 2018

sam-goodwin Dec 13, 2018

sam-goodwin Dec 13, 2018

Raise an alarm when any stages are in a Failed state. #6

Raise an alarm when any stages are in a Failed state. #6

Conversation

sam-goodwin commented Dec 13, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment