Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lambda invocation failure alarm #244

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/alarms/__tests__/__snapshots__/lambda-alarms.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions src/alarms/__tests__/lambda-alarms.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import "@aws-cdk/assert/jest"
import { App, Stack } from "aws-cdk-lib"
import * as cloudwatchActions from "aws-cdk-lib/aws-cloudwatch-actions"
import * as sns from "aws-cdk-lib/aws-sns"
import "jest-cdk-snapshot"
import { LambdaAlarms } from "../lambda-alarms"

test("create alarms", () => {
const app = new App()
const supportStack = new Stack(app, "SupportStack")
const stack = new Stack(app, "Stack")

const topic = new sns.Topic(supportStack, "Topic")
const action = new cloudwatchActions.SnsAction(topic)

const alarms = new LambdaAlarms(stack, "LambdaAlarms", {
actions: [action],
lambdaFunctionName: "lambda-function-name",
})

alarms.addInvocationErrorAlarm({
appendToAlarmDescription: "Runbook at https://liflig.no",
})

expect(stack).toMatchCdkSnapshot()
})
1 change: 1 addition & 0 deletions src/alarms/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export { DatabaseAlarms, DatabaseAlarmsProps } from "./database-alarms"
export { LambdaAlarms, LambdaAlarmsProps } from "./lambda-alarms"
export { ServiceAlarms, ServiceAlarmsProps } from "./service-alarms"
export { SlackAlarm, SlackAlarmProps } from "./slack-alarm"
62 changes: 62 additions & 0 deletions src/alarms/lambda-alarms.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import * as constructs from "constructs"
import * as cdk from "aws-cdk-lib"
import * as cloudwatch from "aws-cdk-lib/aws-cloudwatch"

export interface LambdaAlarmsProps {
actions: cloudwatch.IAlarmAction[]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Den er en array, for i DTP har vi sendt samme alarm til fler actions, både Slack og OpsGenie.
Kan alternativt vært en enkelt IAlarmAction og så instansiert LambdaAlarms to ganger og lagd 2 Alarms i DTP, spesielt nå som jeg fjernet eksplisitt alarmName.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tror det er enklest å holde det til en IAlarmAction for enkelhetens skyld, spesielt nå som det å opprette flere ikke er et problem etter fjerning av alarmName

lambdaFunctionName: string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Endret til å ta inn bare en lambda name som string i stedet for lambda.Function. Unngår imports her og lettere å skrive test-kode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Har lurt litt på denne, og tror vi bør holde det til å sende inn selve funksjonen.

  • Dersom vi senere introduserer andre alarmer/metrikker som ser på andre attributter i en lambda vil vi være nødt til å dytte ut en breaking change til alle konsumenter hvor vi endrer signaturen fra string (funksjonsnavn) til objekt (funksjon)
  • Vi mister den eksplisitte CloudFormation-avhengigheten mellom metrikken og lambdaen. Det er ikke et problem i akkurat dette tilfellet, siden metrikken kan opprettes uavhengig, men det hadde vært kjekt å ha avhengigheten definert uansett

Så er ikke krise, men tror vi bør gjøre det for å ruste oss mot endringer

}

export class LambdaAlarms extends constructs.Construct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kan ha en docstring som oppsummerer klassens ansvar, gjerne med notis tilsvarende service-alarms om at ingen alarmer opprettes før man kaller de forskjellige metodene

private readonly actions: cloudwatch.IAlarmAction[]
private readonly lambdaFunctionName: string

constructor(
scope: constructs.Construct,
id: string,
props: LambdaAlarmsProps,
) {
super(scope, id)

this.actions = props.actions
this.lambdaFunctionName = props.lambdaFunctionName
}

/**
* Sets up a CloudWatch Alarm that triggers if the Lambda fails invocations.
* This usually happens from uncaught exceptions in the lambda.
*/
addInvocationErrorAlarm(
/**
* Configuration for an alarm.
*/
props?: {
/**
* Add extra information to the alarm description, like Runbook URL or steps to triage.
*/
appendToAlarmDescription?: string
},
): void {
const alarm = new cloudwatch.Metric({
metricName: "Errors",
namespace: "AWS/Lambda",
statistic: "Sum",
period: cdk.Duration.seconds(60), // Standard resolution metric has a minimum of 60s period
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kan fjerne kommentaren

dimensionsMap: {
FunctionName: this.lambdaFunctionName,
},
}).createAlarm(this, "FailedInvocationAlarm", {
alarmDescription: `Invocation for '${this.lambdaFunctionName}' failed. ${props?.appendToAlarmDescription ?? ""}`,
Comment on lines +48 to +49
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fjernet eksplisitt alarmName her. For å unngå collisions hvis man lager flere av samme construct or alarm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bra!

comparisonOperator:
cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
evaluationPeriods: 1,
threshold: 1,
treatMissingData: cloudwatch.TreatMissingData.IGNORE,
})

this.actions.forEach((action) => {
alarm.addAlarmAction(action)
alarm.addOkAction(action)
})
}
}