Add pattern for scheduled lambdas #186

jacobwinch · 2021-01-21T09:43:34Z

What does this change?

This PR is a first attempt at a scheduled lambda pattern. It also introduces a construct for CloudWatch alarms, and a pattern for a lambda-specific CloudWatch alarms, based on error %.

I've introduced the CloudWatch-y stuff at the same time as the scheduled lambda pattern because a number of examples of this pattern 'in the wild' also include an alert. I think we should be encouraging developers to think about monitoring their software whenever they add new compute resources, so hopefully this will help to do that.

Does this change require changes to existing projects or CDK CLI?

No changes are required, however stacks that use schedule-based lambdas can simplify their infrastructure definition slightly.

How to test

Upgrade an existing stack to use this new pattern. The snapshot tests should show 0 (or trivial) changes.
Create a new stack based on this pattern. You should end up with a functional scheduled lambda.
Configure monitoring and force the lambda to fail X% of the time to ensure that the new alert pattern works as expected.

I've tested 2 & 3 in the developerPlayground account.

How can we measure success?

Projects already using this library (e.g. tag-janitor and pagerduty-summary) should be able to simplify their infrastructure definition slightly
New projects should get sensible monitoring for a tiny amount of extra code/effort

Have we considered potential risks?

This is all new functionality; merging this PR should not have an impact on any projects currently using this library.

jacobwinch · 2021-01-21T09:45:27Z

src/patterns/lambda-alarms.ts

+export interface ErrorPercentageMonitoring
+  extends Omit<GuAlarmProps, "metric" | "threshold" | "comparisonOperator" | "evaluationPeriods"> {
+  toleratedErrorPercentage: number;
+  numberOfFiveMinutePeriodsToEvaluate?: number;


Better suggestions for this name are welcome!

jacobwinch · 2021-01-21T09:56:40Z

src/patterns/scheduled-lambda.ts

+
+interface GuScheduledLambdaProps extends Omit<GuFunctionProps, "rules" | "apis"> {
+  schedule: Schedule;
+  monitoringConfiguration?: ErrorPercentageMonitoring;


I wondered about making the opt-out for this more explicit. In Scala I would do something like:

sealed trait LambdaMonitoring case object NoMonitoring extends LambdaMonitoring case class ErrorPercentageMonitoring(toleratedErrorPercentage: Int, snsTopicArn: String, numberOfFiveMinutePeriodsToEvaluate: Option[Int]=Some(5)) extends LambdaMonitoring

The interface would then be:

monitoringConfiguration: LambdaMonitoring

So to opt-out, you'd have to state:

monitoringConfiguration: NoMonitoring

This would make it clearer (and highlight to reviewers) that you were intentionally omitting this (hopefully for a good reason e.g. you prefer some different alerting system to CloudWatch), rather than just being (another) optional property that can be easily forgotten.

Any opinions on whether we'd want to do this? If so, any suggestions on how we could go about this in TypeScript? I tried a couple of options using Unions but they were all pretty ugly!

Yeah, that would be nice. I don't know of a nice way of doing that in typescript although I often feel like there must be one that I'm missing.

Does this work? (I'm not too certain on the as though!)

Thanks for the suggestion @akash1810 - I've used this as the basis for dc3972c. To be honest, I'm not sure whether I like this more than my original implementation, but I've spent quite a bit of time experimenting and I can't find a cleaner way. Happy to revert dc3972c if people are not keen on this approach!

jacobwinch · 2021-01-21T10:00:15Z

src/patterns/lambda-alarms.ts

@@ -0,0 +1,32 @@
+import { ComparisonOperator, MathExpression } from "@aws-cdk/aws-cloudwatch";


I wasn't 100% sure whether this should be a pattern or a construct - any feedback on this would be helpful.

My first impression is that this should be in constructs as patterns should essentially encompass an entire stack but interested to hear other people's opinions too

Having read this comment and listened to @akash1810's excellent explanation of patterns during the presentations, I'm inclined to agree - I'll move this.

Agree with @jamie-lynch and reminded that InstanceRole needs moving out of the patterns directory! https://docs.aws.amazon.com/cdk/latest/guide/constructs.html is a good read.

jacobwinch · 2021-01-21T10:02:27Z

src/constructs/cloudwatch/alarm.ts

+import type { GuStack } from "../core";
+
+export interface GuAlarmProps extends AlarmProps {
+  snsTopicArn: string;


Most teams I've worked on have 1 or 2 topics configured for all alerts and then pass the arn into various templates as parameters.

We could also consider taking the topic name (which is not private) instead of the full arn (this contains an AWS account number, which should be kept private), to simplify things slightly?

Yeah, it looks like that's what the GuLogShippingPolicy does. Then it gets the region and account from scope

Is it worth having a single topic, set in param store, for use by multiple stacks? Or is the separation of concerns important?

Good question. My initial thought is that configuring a single topic in parameter store would not work so well in this case because multiple teams sometimes share the same AWS account, but not the same topic for alerts (or the same mailing list). Similarly teams might want to use different topics for alerts of a different priority etc.

I think it'd be worth thinking about how we might standardise this further in the future, but I'm inclined to leave it as is for now.

akash1810 · 2021-01-21T16:12:17Z

src/constructs/cloudwatch/alarm.ts

+import type { GuStack } from "../core";
+
+export interface GuAlarmProps extends AlarmProps {
+  snsTopicArn: string;


Is it worth having a single topic, set in param store, for use by multiple stacks? Or is the separation of concerns important?

akash1810 · 2021-01-21T16:15:00Z

src/constructs/lambda/lambda.ts

@@ -12,7 +12,7 @@ interface ApiProps extends Omit<LambdaRestApiProps, "handler"> {
  id: string;
 }

-interface GuFunctionProps extends Omit<FunctionProps, "code"> {
+export interface GuFunctionProps extends Omit<FunctionProps, "code"> {
  code: { bucket: string; key: string };


Possibly not for this PR, but would be good for the bucket to come for free, GuGetDistributablePolicy is a vague demonstration of this.

akash1810 · 2021-01-21T16:18:07Z

src/patterns/lambda-alarms.ts

@@ -0,0 +1,32 @@
+import { ComparisonOperator, MathExpression } from "@aws-cdk/aws-cloudwatch";


Agree with @jamie-lynch and reminded that InstanceRole needs moving out of the patterns directory! https://docs.aws.amazon.com/cdk/latest/guide/constructs.html is a good read.

akash1810 · 2021-01-21T16:27:35Z

src/patterns/scheduled-lambda.ts

+
+interface GuScheduledLambdaProps extends Omit<GuFunctionProps, "rules" | "apis"> {
+  schedule: Schedule;
+  monitoringConfiguration?: ErrorPercentageMonitoring;


Does this work? (I'm not too certain on the as though!)

akash1810 · 2021-01-22T20:08:52Z

package.json

@@ -40,6 +40,7 @@
  "dependencies": {
    "@aws-cdk/assert": "~1.74.0",
    "@aws-cdk/aws-autoscaling": "~1.74.0",
+    "@aws-cdk/aws-cloudwatch-actions": "~1.74.0",


This'll need rebasing from main since #191.

Suggested change

"@aws-cdk/aws-cloudwatch-actions": "~1.74.0",

"@aws-cdk/aws-cloudwatch-actions": "1.86.0",

jamie-lynch

This looks great! I've left a couple of comments but either stuff for us to think about in the future or personal preference on how we test so happy for this to be merged.

jamie-lynch · 2021-01-25T13:38:50Z

src/constructs/cloudwatch/alarm.test.ts

+      runtime: Runtime.NODEJS_12_X,
+    });
+    new GuAlarm(stack, "alarm", {
+      alarmName: `Alarm in ${stack.stage}`,


One for a future PR but I wonder if it would be useful to append stack, stage and app to the alarm name (or including the information in some other way) as part of the GuAlarm

Good idea. Ideally we would add these as tags, but unfortunately that's not currently possible, so perhaps we should put these into the name by default for now.

jamie-lynch · 2021-01-25T13:52:42Z

src/constructs/cloudwatch/lambda-alarms.ts

+export type LambdaMonitoring = NoMonitoring | ErrorPercentageMonitoring;
+
+export interface NoMonitoring {
+  noMonitoring: true;
+}
+
+export interface ErrorPercentageMonitoring
+  extends Omit<GuAlarmProps, "metric" | "threshold" | "comparisonOperator" | "evaluationPeriods"> {
+  toleratedErrorPercentage: number;
+  numberOfFiveMinutePeriodsToEvaluate?: number;
+  noMonitoring?: false;
+}
+
+interface GuLambdaAlarmProps extends ErrorPercentageMonitoring {
+  lambda: GuLambdaFunction;
+}


Maybe one for a future PR but I wonder if the noMonitoring stuff is something that should live at a higher level as it's unlikely to be specific to Lambda monitoring and also it feels a bit odd to define a prop here which then isn't used.

Maybe NoMonitoring should be exported from somewhere else and the monitoringConfiguration prop becomes monitoringConfiguration: LambdaMonitoring | NoMonitoring? Then I guess ErrorPercentageMonitoring could extend GuMonitoringProps to add the noMonitoring?: false.

Agreed - this part of the code needs a little more thought as we cover more use-cases. I think I will go ahead and merge this as is for now, but perhaps we can refactor this slightly as part of adding additional lambda patterns.

jamie-lynch · 2021-01-25T13:57:06Z

src/constructs/cloudwatch/lambda-alarms.test.ts

+    expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot();
+    expect(stack).toHaveResource("AWS::CloudWatch::Alarm", {
+      EvaluationPeriods: 12,
+    });


I think given this has been moved to constructs we should prefer direct assertions over snapshots wherever possible. For the first test, the snapshot is maybe still the better choice as it will pick up any unexpected side effects of changes we make but for these tests that also have a direct assertion, I'd vote that the snapshot assertions can be removed.

Sounds sensible, I've updated the tests and removed obsolete snapshots in 139d524.

jacobwinch added 3 commits January 19, 2021 16:57

Add simple pattern for scheduled lambda

2c89a6e

Add construct for CloudWatch Alarms

489bbff

Add optional monitoring for scheduled lambdas

3d6c3be

jacobwinch commented Jan 21, 2021

View reviewed changes

jacobwinch mentioned this pull request Jan 21, 2021

Provide sensible defaults for GuLambda memorySize and timeout #188

Merged

Move lambda-alarms from patterns into constructs

69769c8

akash1810 reviewed Jan 21, 2021

View reviewed changes

jacobwinch added 8 commits January 22, 2021 10:13

Change snsTopicArn to snsTopicName

c11f607

Merge branch 'main' into jw-scheduled-lambda

837f3bb

Update test snapshots

c2f4d58

Make configuring alarms opt-out rather than opt-in

dc3972c

Add label for % based alarm

b1d1681

Add default alarm names and descriptions

ddfafe6

Fix bad import

a8994ec

Tidy up

628e72c

akash1810 reviewed Jan 22, 2021

View reviewed changes

jacobwinch added 2 commits January 25, 2021 10:01

Merge branch 'main' into jw-scheduled-lambda

3906f99

Add up to date cloudwatch-actions dependency

f2b937b

jacobwinch marked this pull request as ready for review January 25, 2021 12:05

jacobwinch requested a review from a team January 25, 2021 12:05

jamie-lynch approved these changes Jan 25, 2021

View reviewed changes

Remove snapshot assertions if we have a direct assertion

139d524

jacobwinch merged commit 73f2ddd into main Jan 25, 2021

jacobwinch deleted the jw-scheduled-lambda branch January 25, 2021 14:18

This was referenced Jan 26, 2021

Add alert based on error % of lambda guardian/tag-janitor#38

Merged

Add pattern for SNS lambdas #215

Merged

jacobwinch mentioned this pull request Feb 10, 2021

docs: Add basic docs for scheduled lambda pattern #241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pattern for scheduled lambdas #186

Add pattern for scheduled lambdas #186

jacobwinch commented Jan 21, 2021

jacobwinch Jan 21, 2021

jacobwinch Jan 21, 2021

jamie-lynch Jan 21, 2021

akash1810 Jan 21, 2021

jacobwinch Jan 22, 2021 •

edited

Loading

jacobwinch Jan 21, 2021

jamie-lynch Jan 21, 2021

jacobwinch Jan 21, 2021 •

edited

Loading

akash1810 Jan 21, 2021

jacobwinch Jan 21, 2021

jamie-lynch Jan 21, 2021

akash1810 Jan 21, 2021

jacobwinch Jan 22, 2021

akash1810 Jan 21, 2021

akash1810 Jan 21, 2021

akash1810 Jan 21, 2021

akash1810 Jan 21, 2021

akash1810 Jan 22, 2021

jamie-lynch left a comment

jamie-lynch Jan 25, 2021

jacobwinch Jan 25, 2021

jamie-lynch Jan 25, 2021

jacobwinch Jan 25, 2021

jamie-lynch Jan 25, 2021

jacobwinch Jan 25, 2021

		@@ -0,0 +1,32 @@
		import { ComparisonOperator, MathExpression } from "@aws-cdk/aws-cloudwatch";

	"@aws-cdk/aws-cloudwatch-actions": "~1.74.0",
	"@aws-cdk/aws-cloudwatch-actions": "1.86.0",

Add pattern for scheduled lambdas #186

Add pattern for scheduled lambdas #186

Conversation

jacobwinch commented Jan 21, 2021

What does this change?

Does this change require changes to existing projects or CDK CLI?

How to test

How can we measure success?

Have we considered potential risks?

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

jacobwinch Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobwinch Jan 21, 2021 • edited Loading

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

jamie-lynch 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

jacobwinch Jan 22, 2021 •

edited

Loading

jacobwinch Jan 21, 2021 •

edited

Loading