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

johnduffell · 2025-01-15T16:24:17Z

A while ago I proposed a new feature in alarms handler to send relevant log links to chat as well.

I made a PR but it got very cluttered with pre-refactoring so I will pull out individual changes to make it easier to review.

This is the first PR
Second PR is here: #2636
Third PR is here: #2641

Regarding this PR

the new tests I needed required parameterising the alarm mappings, so I pulled out the main lambda logic into a function and added that parameter
To make it easier to check what messages should be sent, I made the above return the message list rather than having to trap the HTTP fetch callouts. i.e. pure-ish function, and then the effects can sit at the edge.
As part of that, some duplication naturally fell out between the code paths.

Tested in CODE and still sends alarms

johnduffell · 2025-01-15T16:27:43Z

handlers/alarms-handler/src/alarmMappings.ts

+export type AlarmMappings = {
+	getTeams: (appName?: string) => Team[];
+	getTeamWebhookUrl: (team: Team) => string;
+};


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?

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) => {...}

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

johnduffell · 2025-01-15T16:28:58Z

handlers/alarms-handler/src/alarmMappings.ts

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

johnduffell · 2025-01-15T16:38:38Z

handlers/alarms-handler/test/index.test.ts

@@ -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');


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.

… a more pure function

handlers/alarms-handler/src/alarmMappings.ts

tomrf1 · 2025-01-16T09:00:49Z

handlers/alarms-handler/src/index.ts

+export async function getChatMessages(
+	record: SQSRecord,
+	alarmMappings: AlarmMappings,
+) {


return type?

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

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

great in that case I've added it in!

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

tjmw

This looks good! I left a bunch of pretty minor comments/suggestions to take or leave.

tjmw · 2025-01-22T17:27:32Z

handlers/alarms-handler/src/alarmMappings.ts

 };
+
+export const ProdAlarmMappings = buildAlarmMappings(teamToAppMappings);


Maybe just me, but I feel like it's slightly unconventional to have this be camel case with the first letter in caps (which makes me think it's a class name or even a type). I'd probably go for prodAlarmMappings. Obviously a very minor thing so no big deal either way!

tjmw · 2025-01-22T17:29:30Z

handlers/alarms-handler/src/index.ts

+	} else {
+		return undefined;
+	}


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

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

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

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!)

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

tjmw · 2025-01-22T17:30:15Z

handlers/alarms-handler/src/index.ts

+	record: SQSRecord,
+	alarmMappings: AlarmMappings,
+): Promise<{ webhookUrls: string[]; text: string } | undefined> {
+	console.log('sqsRecord', record);


Should these console.logs be removed before merging?

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?

tjmw · 2025-01-22T17:34:33Z

handlers/alarms-handler/src/index.ts

+
+	let message;
+	if (parsedMessage) {
+		message = await handleCloudWatchAlarmMessage({ message: parsedMessage });


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?

good spot, same for buildSnsPublishMessage

tjmw · 2025-01-22T17:42:03Z

handlers/alarms-handler/src/index.ts

+
+	if (message) {
+		const teams = alarmMappings.getTeams(message.app);
+		console.log('sending message to teams', teams);


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);

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

tjmw · 2025-01-22T17:43:23Z

handlers/alarms-handler/src/index.ts

-			});
-		}),
-	);
+	return { app, text };


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 };

johnduffell · 2025-01-23T10:28:22Z

This looks good! I left a bunch of pretty minor comments/suggestions to take or leave.

Thanks this is exactly the kind of thing that's useful, I've made all your suggestions apart from the undefined one as that broke the type checker/linter. In retrospect I think it's kind of nice to be explicit, in scala I'd use an Option but I don't think that's usual TS.

johnduffell changed the title ~~aid testability by changing the logic to be an effectful crust around…~~ aid alarms-handler testability by making the main logic a more pure function Jan 15, 2025

johnduffell commented Jan 15, 2025

View reviewed changes

aid testability by changing the logic to be an effectful crust around…

98c37b6

… a more pure function

johnduffell force-pushed the jd-alarms-handler-testability branch from 8f1bc45 to 98c37b6 Compare January 15, 2025 16:45

This was referenced Jan 15, 2025

get all tags in alarm handler rather than just App #2636

Merged

use alarm tags to add diagnostic urls to chat messages #2637

Open

tomrf1 reviewed Jan 16, 2025

View reviewed changes

handlers/alarms-handler/src/alarmMappings.ts Outdated Show resolved Hide resolved

tomrf1 reviewed Jan 16, 2025

View reviewed changes

johnduffell mentioned this pull request Jan 16, 2025

use alarm tags to add log urls to chat messages #2641

Merged

johnduffell added 2 commits January 16, 2025 17:49

more idiomatic constructor syntax

3a1a36b

add type to getChatMessages function

fccef3a

johnduffell requested review from tjmw, tomrf1 and AndreaDiotallevi January 22, 2025 10:38

tjmw approved these changes Jan 22, 2025

View reviewed changes

minor tweaks PR feedback

c2645ba

johnduffell merged commit 7a662d6 into main Jan 23, 2025
49 checks passed

johnduffell deleted the jd-alarms-handler-testability branch January 23, 2025 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johnduffell commented Jan 15, 2025 •

edited

Loading

johnduffell Jan 15, 2025

tomrf1 Jan 16, 2025

johnduffell Jan 16, 2025

johnduffell Jan 15, 2025

johnduffell Jan 15, 2025

tomrf1 Jan 16, 2025

johnduffell Jan 16, 2025

tomrf1 Jan 17, 2025

johnduffell Jan 20, 2025

johnduffell Jan 20, 2025

tjmw left a comment

tjmw Jan 22, 2025

tjmw Jan 22, 2025

johnduffell Jan 23, 2025

johnduffell Jan 23, 2025

tjmw Jan 23, 2025

tjmw Jan 23, 2025 •

edited

Loading

tjmw Jan 22, 2025

johnduffell Jan 23, 2025

tjmw Jan 22, 2025

johnduffell Jan 23, 2025

tjmw Jan 22, 2025

johnduffell Jan 23, 2025

tjmw Jan 22, 2025

johnduffell commented Jan 23, 2025

		};

		export const ProdAlarmMappings = buildAlarmMappings(teamToAppMappings);

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

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

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

Conversation

johnduffell commented Jan 15, 2025 • 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

tjmw 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

tjmw Jan 23, 2025 • 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

johnduffell commented Jan 23, 2025

johnduffell commented Jan 15, 2025 •

edited

Loading

tjmw Jan 23, 2025 •

edited

Loading