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.

Sign up for GitHub

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

aid alarms-handler testability by making the main logic a more pure function #2635

Merged
merged 4 commits into from
Jan 23, 2025
Merged

aid alarms-handler testability by making the main logic a more pure function #2635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98c37b6
aid testability by changing the logic to be an effectful crust around…
johnduffell Jan 15, 2025
3a1a36b
more idiomatic constructor syntax
johnduffell Jan 16, 2025
fccef3a
add type to getChatMessages function
johnduffell Jan 20, 2025
c2645ba
minor tweaks PR feedback
johnduffell Jan 23, 2025
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
44 changes: 30 additions & 14 deletions handlers/alarms-handler/src/alarmMappings.ts
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

the changes in this file are simply to make the mappings parameterised rather than being a singleton.

Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,12 @@ const teamToAppMappings: Record<Team, string[]> = {
],
};

const buildAppToTeamMappings = (): Record<string, Team[]> => {
const buildAppToTeamMappings = (
theMappings: Record<Team, string[]>,
): Record<string, Team[]> => {
const mappings: Record<string, Team[]> = {};

for (const [team, apps] of Object.entries(teamToAppMappings)) {
for (const [team, apps] of Object.entries(theMappings)) {
for (const app of apps) {
const teams = mappings[app] ?? [];
teams.push(team as Team);
Expand All @@ -136,19 +138,33 @@ const buildAppToTeamMappings = (): Record<string, Team[]> => {
return mappings;
};

const appToTeamMappings: Record<string, Team[]> = buildAppToTeamMappings();
export type AlarmMappings = {
getTeams: (appName?: string) => Team[];
getTeamWebhookUrl: (team: Team) => string;
};
Comment on lines +141 to +144
Copy link
Member Author

Choose a reason for hiding this comment

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

in the previous PR I did a class as that gives you the type for free, but I wasn't sure how to do similar as function based (assuming this is even what was meant) - perhaps there's a typeof trick I should be using rather than replicating the function signatures here?

Copy link
Member

Choose a reason for hiding this comment

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

I think defining the type to be returned by the function here is good.
Technically this could be an interface though.

Regarding "replicating the function signatures" - I think what you have here is fine. It is possible to reference the type of a field as follows (though I don't think it's necessary here):
const getTeams: AlarmMappings['getTeams'] = (appName) => {...}

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks Tom for the link and I will leave it as it is then, I agree on the referencing thing.


export const getTeams = (appName?: string): Team[] => {
if (appName && appToTeamMappings[appName]) {
return appToTeamMappings[appName];
}
export const AlarmMappings: (
johnduffell marked this conversation as resolved.
Show resolved Hide resolved
mappings: Record<string, string[]>,
) => AlarmMappings = (mappings: Record<string, string[]>) => {
const appToTeamMappings: Record<string, Team[]> =
buildAppToTeamMappings(mappings);

return ['SRE'];
};
const getTeams = (appName?: string): Team[] => {
if (appName && appToTeamMappings[appName]) {
return appToTeamMappings[appName];
}

export const getTeamWebhookUrl = (team: Team): string => {
return getIfDefined<string>(
process.env[`${team}_WEBHOOK`],
`${team}_WEBHOOK environment variable not set`,
);
return ['SRE'];
};

const getTeamWebhookUrl = (team: Team): string => {
return getIfDefined<string>(
process.env[`${team}_WEBHOOK`],
`${team}_WEBHOOK environment variable not set`,
);
};

return { getTeams, getTeamWebhookUrl };
};

export const ProdAlarmMappings = AlarmMappings(teamToAppMappings);
129 changes: 72 additions & 57 deletions handlers/alarms-handler/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { SNSEventRecord, SQSEvent } from 'aws-lambda';
import type { SNSEventRecord, SQSEvent, SQSRecord } from 'aws-lambda';
import { z } from 'zod';
import { getTeams, getTeamWebhookUrl } from './alarmMappings';
import type { AlarmMappings } from './alarmMappings';
import { ProdAlarmMappings } from './alarmMappings';
import { getAppNameTag } from './cloudwatch';

const cloudWatchAlarmMessageSchema = z.object({
Expand All @@ -17,23 +18,21 @@ type CloudWatchAlarmMessage = z.infer<typeof cloudWatchAlarmMessageSchema>;
export const handler = async (event: SQSEvent): Promise<void> => {
try {
for (const record of event.Records) {
console.log(record);

const { Message, MessageAttributes } = JSON.parse(
record.body,
) as SNSEventRecord['Sns'];

const parsedMessage = attemptToParseMessageString({
messageString: Message,
});

if (parsedMessage) {
await handleCloudWatchAlarmMessage({ message: parsedMessage });
} else {
await handleSnsPublishMessage({
message: Message,
messageAttributes: MessageAttributes,
});
const maybeChatMessages = await getChatMessages(
record,
ProdAlarmMappings,
);

if (maybeChatMessages) {
await Promise.all(
maybeChatMessages.webhookUrls.map((webhookUrl) => {
return fetch(webhookUrl, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ text: maybeChatMessages.text }),
});
}),
);
}
}
} catch (error) {
Expand All @@ -42,6 +41,44 @@ export const handler = async (event: SQSEvent): Promise<void> => {
}
};

export async function getChatMessages(
record: SQSRecord,
alarmMappings: AlarmMappings,
) {
console.log('sqsRecord', record);
Copy link
Member

Choose a reason for hiding this comment

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

return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's Promise<{webhookUrls: string[], text: string} | undefined>

Last year someone in the team told me in typescript you are supposed to avoid specifying return types as it can break your code at run time. Someone sent me some videos about it which I watched, but I forgot the exact reasoning, and can't find them any more. Perhaps some of these? https://marcoheine.com/today-i-learned/dont-use-return-types-in-typescript

Copy link
Member

Choose a reason for hiding this comment

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

I've not heard that before...
In general if the return type isn't obvious then it's helpful to other humans to specify it. In this case it wasn't obvious to me

Copy link
Member Author

Choose a reason for hiding this comment

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

great in that case I've added it in!

Copy link
Member Author

Choose a reason for hiding this comment

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

aha here was the original note suggesting to avoid them which I clearly took some time to digest! #2317 (comment)


Copy link
Member

Choose a reason for hiding this comment

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

Should these console.logs be removed before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally left them as I think this is the correct way to log things in cloudwatch in a JS lambda, if that's right?

const { Message, MessageAttributes } = JSON.parse(
record.body,
) as SNSEventRecord['Sns'];

console.log('snsEvent', Message, MessageAttributes);

const parsedMessage = attemptToParseMessageString({
messageString: Message,
});

console.log('parsedMessage', parsedMessage);

let message;
if (parsedMessage) {
message = await handleCloudWatchAlarmMessage({ message: parsedMessage });
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if handle is the right prefix for this method now? To me it kind of implies actually doing something with the message. What about buildCloudwatchAlarmMessage?

Copy link
Member Author

Choose a reason for hiding this comment

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

good spot, same for buildSnsPublishMessage

message = handleSnsPublishMessage({
message: Message,
messageAttributes: MessageAttributes,
});
}

if (message) {
const teams = alarmMappings.getTeams(message.app);
console.log('sending message to teams', teams);
const webhookUrls = teams.map(alarmMappings.getTeamWebhookUrl);
Copy link
Member

Choose a reason for hiding this comment

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

If we keep this log line, what about mapping message to teams since we aren't actually sending it here?

Suggested change
console.log('sending message to teams', teams);
console.log('mapping message to teams', teams);

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, went for
console.log(app ${message.app} is assigned to teams ${teams.join(', ')});
in the end

return { webhookUrls, text: message.text };
} else {
return undefined;
}
}
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

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

Totally subjective, but we don't need this else block I don't think. With no explicit return undefined will be returned implicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

got it, so it's like of like mapping an Option in scala

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that but it complains, not sure if I need to change something else?
image

Copy link
Member

Choose a reason for hiding this comment

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

Oh weird sorry about that. Maybe it's the fact that the return value is wrapped in a promise? Best to leave it as is (which I think you've done!)

Copy link
Member

@tjmw tjmw Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I'm just wrong here - even without the promise wrapping I see the same thing in the TS playground:

Screenshot 2025-01-23 at 11 53 32


const attemptToParseMessageString = ({
messageString,
}: {
Expand Down Expand Up @@ -70,32 +107,21 @@ const handleCloudWatchAlarmMessage = async ({
} = message;

const app = await getAppNameTag(AlarmArn, AWSAccountId);
const teams = getTeams(app);

await Promise.all(
teams.map((team) => {
const webhookUrl = getTeamWebhookUrl(team);

const title =
NewStateValue === 'OK'
? `✅ *ALARM OK:* ${AlarmName} has recovered!`
: `🚨 *ALARM:* ${AlarmName} has triggered!`;
const text = `${title}\n\n*Description:* ${
AlarmDescription ?? ''
}\n\n*Reason:* ${NewStateReason}`;

console.log(`CloudWatch alarm from ${app} owned by ${team}`);

return fetch(webhookUrl, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ text }),
});
}),
);

const title =
NewStateValue === 'OK'
? `✅ *ALARM OK:* ${AlarmName} has recovered!`
: `🚨 *ALARM:* ${AlarmName} has triggered!`;
const text = `${title}\n\n*Description:* ${
AlarmDescription ?? ''
}\n\n*Reason:* ${NewStateReason}`;

console.log(`CloudWatch alarm from ${app}`);

return { app, text };
};

const handleSnsPublishMessage = async ({
const handleSnsPublishMessage = ({
message,
messageAttributes,
}: {
Expand All @@ -109,21 +135,10 @@ const handleSnsPublishMessage = async ({
}

const app = messageAttributes.app?.Value;
const teams = getTeams(app);

await Promise.all(
teams.map((team) => {
const webhookUrl = getTeamWebhookUrl(team);

const text = message;
const text = message;

console.log(`SNS publish message from ${app} owned by ${team}`);
console.log(`SNS publish message from ${app}`);

return fetch(webhookUrl, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ text }),
});
}),
);
return { app, text };
};
Copy link
Member

Choose a reason for hiding this comment

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

Totally subjective, and very minor, but rather than assigning to a new const I'd probably just map from one name to the other in the return:

Suggested change
return { app, text };
return { app, text: message };

8 changes: 4 additions & 4 deletions handlers/alarms-handler/test/alarmMappings.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
import { getTeams } from '../src/alarmMappings';
import { ProdAlarmMappings } from '../src/alarmMappings';

describe('getTeam', () => {
it('returns the correct team for a given app', () => {
const app = 'apps-metering';

const team = getTeams(app);
const team = ProdAlarmMappings.getTeams(app);

expect(team).toEqual(['GROWTH']);
});

it('returns SRE if the app does not have a team owner', () => {
const app = 'foo';

const team = getTeams(app);
const team = ProdAlarmMappings.getTeams(app);

expect(team).toEqual(['SRE']);
});

it('returns SRE if there is no app tag', () => {
const app = undefined;

const team = getTeams(app);
const team = ProdAlarmMappings.getTeams(app);

expect(team).toEqual(['SRE']);
});
Expand Down
85 changes: 83 additions & 2 deletions handlers/alarms-handler/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { type SQSEvent } from 'aws-lambda';
import { handler } from '../src';
import type { SQSEvent, SQSRecord } from 'aws-lambda';
import { getChatMessages, handler } from '../src';
import { AlarmMappings } from '../src/alarmMappings';
import { getAppNameTag } from '../src/cloudwatch';

jest.mock('../src/cloudwatch');
Expand Down Expand Up @@ -62,6 +63,25 @@ describe('Handler', () => {
expect(getAppNameTag).toHaveBeenCalledWith('mock-arn', '111111');
expect(fetch).toHaveBeenCalledWith(mockEnv.SRE_WEBHOOK, expect.any(Object));
});
it('should handle captured CloudWatch alarm message', async () => {
(getAppNameTag as jest.Mock).mockResolvedValueOnce('mock-app');

Copy link
Member Author

Choose a reason for hiding this comment

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

we still have one "mocked" function but it's more minor functionality and doesn't lend itself to parameterising as easily as the alarm mappings.

const result = await getChatMessages(
fullCloudWatchAlarmEvent,
AlarmMappings({ SRE: ['mock-app'] }),
);

expect(getAppNameTag).toHaveBeenCalledWith(
'arn:aws:cloudwatch:eu-west-1:1234:alarm:DISCOUNT-API-CODE Discount-api 5XX response',
'1234',
);
const expectedText =
'🚨 *ALARM:* DISCOUNT-API-CODE Discount-api 5XX response has triggered!\n\n' +
'*Description:* Impact - Discount api returned a 5XX response check the logs for more information: https://eu-west-1.console.aws.amazon.com/cloudwatch/home?region=eu-west-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fdiscount-api-CODE. Follow the process in https://docs.google.com/document/d/sdkjfhskjdfhksjdhf/edit\n\n' +
'*Reason:* Threshold Crossed: 1 datapoint [2.0 (09/10/24 07:18:00)] was greater than or equal to the threshold (1.0).';
expect(result?.webhookUrls).toEqual([mockEnv.SRE_WEBHOOK]);
expect(result?.text).toEqual(expectedText);
});

it('should handle SNS publish message', async () => {
jest
Expand Down Expand Up @@ -116,3 +136,64 @@ describe('Handler', () => {
);
});
});

const fullCloudWatchAlarmEvent = {
messageId: 'askjdhaskjhdjkashdakjsdjkashd',
receiptHandle: 'skdfhksjdfhksjdhfkjsdhfjkhsdfksd==',
body: JSON.stringify({
Type: 'Notification',
MessageId: 'sdkfjhslkdfhjksjdhfkjsdhf',
TopicArn: 'arn:aws:sns:eu-west-1:123456:alarms-handler-topic-CODE',
Subject:
'ALARM: "DISCOUNT-API-CODE Discount-api 5XX response" in EU (Ireland)',
Message: JSON.stringify({
AlarmName: 'DISCOUNT-API-CODE Discount-api 5XX response',
AlarmDescription:
'Impact - Discount api returned a 5XX response check the logs for more information: https://eu-west-1.console.aws.amazon.com/cloudwatch/home?region=eu-west-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fdiscount-api-CODE. Follow the process in https://docs.google.com/document/d/sdkjfhskjdfhksjdhf/edit',
AWSAccountId: '1234',
AlarmConfigurationUpdatedTimestamp: '2024-09-23T09:21:15.363+0000',
NewStateValue: 'ALARM',
NewStateReason:
'Threshold Crossed: 1 datapoint [2.0 (09/10/24 07:18:00)] was greater than or equal to the threshold (1.0).',
StateChangeTime: '2024-10-09T07:23:16.236+0000',
Region: 'EU (Ireland)',
AlarmArn:
'arn:aws:cloudwatch:eu-west-1:1234:alarm:DISCOUNT-API-CODE Discount-api 5XX response',
OldStateValue: 'OK',
OKActions: [],
AlarmActions: ['arn:aws:sns:eu-west-1:1234:alarms-handler-topic-CODE'],
InsufficientDataActions: [],
Trigger: {
MetricName: '5XXError',
Namespace: 'AWS/ApiGateway',
StatisticType: 'Statistic',
Statistic: 'SUM',
Unit: null,
Dimensions: [[Object]],
Period: 300,
EvaluationPeriods: 1,
ComparisonOperator: 'GreaterThanOrEqualToThreshold',
Threshold: 1,
TreatMissingData: '',
EvaluateLowSampleCountPercentile: '',
},
}),
Timestamp: '2024-10-09T07:23:16.318Z',
SignatureVersion: '1',
Signature: 'skjefhksjdhfkjsdhfkjsdhfkjsdf==',
SigningCertURL: 'https://sns.eu-west-1.amazonaws.com/smhdfsmdfhgsdjf.pem',
UnsubscribeURL:
'https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:eu-west-1:1234:alarms-handler-topic-CODE:sdkjfhsdkjfhskdjf',
}),
attributes: {
ApproximateReceiveCount: '1',
SentTimestamp: '1728458596353',
SenderId: 'askjdhaskjdhaksdj',
ApproximateFirstReceiveTimestamp: '1728458596364',
},
messageAttributes: {},
md5OfBody: 'askdjalksdjlasdjlaksjd',
eventSource: 'aws:sqs',
eventSourceARN: 'arn:aws:sqs:eu-west-1:1234:alarms-handler-queue-CODE',
awsRegion: 'eu-west-1',
} as SQSRecord;
Loading