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

add log links to alarm chat messages #2487

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

add log links to alarm chat messages #2487

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
ea42307
add log links to alarm chat messages
johnduffell Oct 9, 2024
af88e5f
assume missing stage is PROD
johnduffell Oct 10, 2024
cc2b430
use aws tags instead of hard coded log links
johnduffell Oct 10, 2024
80f209f
groupMap and make "as" safer
johnduffell Oct 15, 2024
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
115 changes: 85 additions & 30 deletions handlers/alarms-handler/src/alarmMappings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { groupBy } from '@modules/arrayFunctions';
import { getIfDefined } from '@modules/nullAndUndefined';

type Team = 'VALUE' | 'GROWTH' | 'PORTFOLIO' | 'PLATFORM' | 'SRE';
Expand All @@ -17,7 +18,8 @@ const sharedMobilePurchasesApps = [
'mobile-purchases-google-update-subscriptions',
];

const teamToAppMappings: Record<Team, string[]> = {
type AppInfo = string | { app: string; logGroups: string[] };
Copy link
Member Author

Choose a reason for hiding this comment

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

now an app can have either just a name, or it can have a name and a list of associated log groups.
In future, rather than hard coding, this could maybe be generated by a call to AWS to find all log groups associated with lambdas/ec2 instances.

export const teamToAppMappings: Record<Team, AppInfo[]> = {
GROWTH: [
'acquisition-events-api',
'admin-console',
Expand Down Expand Up @@ -71,11 +73,17 @@ const teamToAppMappings: Record<Team, string[]> = {
'zuora-creditor',

// support-frontend
'frontend',
{ app: 'frontend', logGroups: ['support-frontend'] },
'it-test-runner',
'stripe-intent',
'workers',
'payment-api',
{
app: 'workers',
logGroups: [
'/aws/lambda/CreatePaymentMethod',
'/aws/lambda/CreateZuoraSubscription', //etc
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 future it might be useful to have a link to the state machine executions logs also, rather than just to the lambdas.

],
},
{ app: 'payment-api', logGroups: ['support-payment-api'] },

// support-service-lambdas
'digital-voucher-suspension-processor',
Expand Down Expand Up @@ -107,33 +115,80 @@ const teamToAppMappings: Record<Team, string[]> = {
],
};

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

for (const [team, apps] of Object.entries(teamToAppMappings)) {
for (const app of apps) {
const teams = mappings[app] ?? [];
teams.push(team as Team);

mappings[app] = teams;
}
export class AlarmMappings {
Copy link
Member Author

@johnduffell johnduffell Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I made this into a class so I could use simplified mappings in the tests more easily should handle captured CloudWatch alarm message

Copy link
Contributor

Choose a reason for hiding this comment

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

With Typescript you can use both OOP or functional style but at the Guardian we usually use a functional style with typescript.

So in this scenario I am not sure about the benefits or changing this part of the code to use a class since the PR does not change the behaviour of this part of the code (how mappings work).

Copy link
Member Author

Choose a reason for hiding this comment

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

ok yes thanks for that, I get super confused between all the different ways to write a function/method. The idea behind making it parameterised rather than a singleton was so I could write a test that used mocked data rather than having to use the real data (which might get edited).
In scala I usually use classes because then you get better help from the compiler and IDE, but I will see how to boil it down to a function.

constructor(mappings: Record<string, AppInfo[]> = teamToAppMappings) {
this.appToTeamMappings = this.buildAppToTeamMappings(mappings);
this.appToLogGroupOverrides = this.buildAppToLogGroupOverrides(mappings);
}
return mappings;
};

const appToTeamMappings: Record<string, Team[]> = buildAppToTeamMappings();

export const getTeams = (appName?: string): Team[] => {
if (appName && appToTeamMappings[appName]) {
return appToTeamMappings[appName] as Team[];
}
private buildAppToTeamMappings = (
theMappings: Record<Team, AppInfo[]>,
): Record<string, Team[]> => {
const entries: Array<[Team, AppInfo[]]> = Object.entries(
theMappings,
) as Array<[Team, AppInfo[]]>; // `as` - hmm?
Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason calling Object.entries on a Record<Team, AppInfo[]> gives Array<[string, AppInfo[]]> so I had to give the type system the info back again. Not sure if this is the safest way of doing it though?


const teamToApp: Array<{ app: string; team: Team }> = entries.flatMap(
([team, appInfos]) =>
appInfos.map((appInfo) => {
const app = typeof appInfo === 'string' ? appInfo : appInfo.app;
return { team, app };
}),
);
const groups = groupBy(teamToApp, ({ app }) => app);

const mappings: Record<string, Team[]> = Object.fromEntries(
Object.entries(groups).map(([app, info]) => [
app,
info.map(({ team }) => team),
]),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

tempted to make an object mapValues function - if there isn't already such a thing?

Suggested change
const mappings: Record<string, Team[]> = Object.fromEntries(
Object.entries(groups).map(([app, info]) => [
app,
info.map(({ team }) => team),
]),
);
const mappings: Record<string, Team[]> =
mapValues(
groups,
(info) => info.map(({ team }) => team),
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above - I am not sure why this needs changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

this was because I changed it from a simple team->app mapping to a team->(app|{app, loglink[]}) mapping so I could extract the log links.
When I wrote the log link extractor I couldn't work out the equivalent to groupMap in typescript so I ended up with a mess! I think the groupMap implementation is there in a later commit. It made sense to update the existing mappings extractor to be consistent but now that it's not needed I will have a look to see if it's still adding value.


return mappings;
};

private buildAppToLogGroupOverrides = (
Copy link
Member Author

Choose a reason for hiding this comment

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

if an app is in multiple teams with different log groups, it will just pick one arbitrarily.
I could have got around it by having separate app->group mapping but I didn't think that would be as maintainable. It does mean the type system doesn't help us out 100% though.

theMappings: Record<Team, AppInfo[]>,
): Record<string, string[]> => {
return Object.fromEntries(
Object.values(theMappings)
.flatMap((appInfos) => appInfos)
.flatMap((appInfo) =>
typeof appInfo !== 'string' ? [[appInfo.app, appInfo.logGroups]] : [],
),
);
};

private appToTeamMappings: Record<string, Team[]>;
private appToLogGroupOverrides: Record<string, string[]>;

getTeams = (appName?: string): Team[] => {
if (appName && this.appToTeamMappings[appName]) {
return this.appToTeamMappings[appName] as Team[];
}

return ['SRE'];
};
return ['SRE'];
};

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

getLogGroups = (appName: string, stage: string): string[] => {
// currently we assume the log group is /aws/lambda/<app>-<stage>, we can add overrides to the appToTeamMappings later
const logGroup = this.appToLogGroupOverrides[appName];
if (logGroup === undefined) {
// assume it's a lambda
console.log('logGroup', logGroup);
const lambdaName = appName + '-' + stage;

const logGroupName = '/aws/lambda/' + lambdaName;
return [logGroupName];
}

export const getTeamWebhookUrl = (team: Team): string => {
return getIfDefined<string>(
process.env[`${team}_WEBHOOK`],
`${team}_WEBHOOK environment variable not set`,
);
};
return logGroup.map((override) => override + '-' + stage);
};
}
23 changes: 12 additions & 11 deletions handlers/alarms-handler/src/cloudwatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,25 @@ const buildCloudwatchClient = (awsAccountId: string): CloudWatchClient => {
return new CloudWatchClient({ region: 'eu-west-1' });
};

const getTags = async (
export type Tags = {
App?: string;
Stage?: string;
};

export const getTags = async (
alarmArn: string,
awsAccountId: string,
): Promise<Tag[]> => {
): Promise<Tags> => {
const client = buildCloudwatchClient(awsAccountId);

const request = new ListTagsForResourceCommand({
ResourceARN: alarmArn,
});

const response = await client.send(request);
return response.Tags ?? [];
};

export const getAppNameTag = async (
alarmArn: string,
awsAccountId: string,
): Promise<string | undefined> => {
const tags = await getTags(alarmArn, awsAccountId);
return tags.find((tag: Tag) => tag.Key === 'App')?.Value;
const tags = response.Tags ?? [];
const entries = tags.flatMap((tag: Tag) =>
tag.Key && tag.Value ? [[tag.Key, tag.Value]] : [],
);
return Object.fromEntries(entries) as Tags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement!

};
Loading
Loading