add log links to alarm chat messages #2487
Conversation
| @@ -17,7 +18,8 @@ const sharedMobilePurchasesApps = [ | |||
| 'mobile-purchases-google-update-subscriptions', | |||
| ]; | |||
|
|
|||
| const teamToAppMappings: Record<Team, string[]> = { | |||
| type AppInfo = string | { app: string; logGroups: string[] }; | |||
There was a problem hiding this comment.
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.
|
|
||
| mappings[app] = teams; | ||
| } | ||
| export class AlarmMappings { |
There was a problem hiding this comment.
I made this into a class so I could use simplified mappings in the tests more easily should handle captured CloudWatch alarm message
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| return mappings; | ||
| }; | ||
|
|
||
| private buildAppToLogGroupOverrides = ( |
There was a problem hiding this comment.
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.
johnduffell
force-pushed
the
jd-alarm-log-links
branch
from
b77f1c0 to
ea42307
Compare
| ): Record<string, Team[]> => { | ||
| const entries: Array<[Team, AppInfo[]]> = Object.entries( | ||
| theMappings, | ||
| ) as Array<[Team, AppInfo[]]>; // `as` - hmm? |
There was a problem hiding this comment.
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?
| app: 'workers', | ||
| logGroups: [ | ||
| '/aws/lambda/CreatePaymentMethod', | ||
| '/aws/lambda/CreateZuoraSubscription', //etc |
There was a problem hiding this comment.
in future it might be useful to have a link to the state machine executions logs also, rather than just to the lambdas.
| const mappings: Record<string, Team[]> = Object.fromEntries( | ||
| Object.entries(groups).map(([app, info]) => [ | ||
| app, | ||
| info.map(({ team }) => team), | ||
| ]), | ||
| ); |
There was a problem hiding this comment.
tempted to make an object mapValues function - if there isn't already such a thing?
| 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), | |
| ) |
There was a problem hiding this comment.
Same comment as above - I am not sure why this needs changing.
There was a problem hiding this comment.
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.
|
Interesting addition! I have a comment regarding where to get the log group from. I appreciate this is a prototype, but I am not sure placing the log group info in the alarm mapping (making it more complex) is the right place. The alarm mapping is used for ownership and it should stay simple I believe. If we talk about "change". If for instance that app log group changes, what is the reason of the change? It is not ownership, it is likely a change in application code. So it should be changed in the producer application code. Probably it should be stored in an alarm tag (via IAC) (e.g. |
|
Thanks Andrea - after a bit of chat the solution is going to be to look for a |
| const entries = tags.flatMap((tag: Tag) => | ||
| tag.Key && tag.Value ? [[tag.Key, tag.Value]] : [], | ||
| ); | ||
| return Object.fromEntries(entries) as Tags; |
There was a problem hiding this comment.
Nice improvement!
| for (const record of event.Records) { | ||
| console.log(record); |
There was a problem hiding this comment.
What is the benefit of changing the way we handle CloudWatch alarms v SNS publish messages?
What I expected was just to just update the handleCloudWatchAlarmMessage function, by using the new tag and modifying the text message.
If this is refactoring it should probably done in a separate PR? In any case, in this case the SNS publish case is only the 1% of the cases and it's very different from the CloudWatch case (input, text message, how you get the App info), so I feel it is more clearer to separate them as early as possible up front.
There was a problem hiding this comment.
thanks for the review, so far I had a first pass at making the change we agreed (to use tags rather than hard coded), but I should have first made the PR into draft so it's clear it's back to in progress. I will go through it again myself and review it before it's ready.
Regarding your question, this change was to make it more testable as I could just check the return value rather than having to mock http calls, however I will have to confirm whether that test is still needed once I've made the updated.
| | undefined, | ||
| StateChangeTime: Date, | ||
| ) { | ||
| const assumedTimeForCompositeAlarms = 300; |
There was a problem hiding this comment.
Nice - Interesting to see how this will help us practically.
There was a problem hiding this comment.
I don't think it would be a help - just compostite alarms do not have a specific period reported from SNS. So it would just choose a default period. It might be possible to fetch the alarm details from AWS CLI and work something out, if it appears to cause confusion.
|
Thanks John for taking in the discussion and experimenting with the new tag. I have just left a few more comments on coding paradigms between OOP and functional with Typescript and whether the refactoring should be done in a separate PR or even if we needed it at all. See them as suggestions. |
thanks for the comments, despite that it's still WIP I think this is helpful when I get it ready for another review. I think there's probably some scala thinking coming through, for better or worse, but I will look through when I get more time and let you know when it's ready |
edit: see discussion on chat thread for improvements: https://chat.google.com/room/AAAAA9BipFs/a9ENymXhPsw/a9ENymXhPsw?cls=10
This PR adds log links to alarm chat messages.
It assumes that logs are /aws/lambda/app-stage by default, but it can be overridden if that is not the case.
Possible future improvement would be to link to other things like step function execution logs.
tested in CODE
(link as text: https://eu-west-1.console.aws.amazon.com/cloudwatch/home?region=eu-west-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fdiscount-api-PROD/log-events$3Fstart$3D1728499576235$26filterPattern$3D$26end$3D1728499876235 )