Provide sensible defaults for GuLambda memorySize and timeout #188
Conversation
| } else { | ||
| switch (runtime.family) { | ||
| case RuntimeFamily.JAVA: | ||
| return 1024; |
There was a problem hiding this comment.
| case RuntimeFamily.JAVA: | ||
| return 1024; | ||
| default: | ||
| return 512; |
There was a problem hiding this comment.
I think the other main family that we want to cater for is NODE, so I've based this on https://github.com/guardian/tag-janitor/blob/main/cdk/lib/cdk-stack.ts#L54 and https://github.com/guardian/google-chat-bots/blob/main/cdk/lib/constants.ts#L29.
src/constructs/lambda/lambda.ts
Outdated
| export class GuLambdaFunction extends Function { | ||
| constructor(scope: GuStack, id: string, props: GuFunctionProps) { | ||
| const bucket = Bucket.fromBucketName(scope, `${id}-bucket`, props.code.bucket); | ||
| const code = Code.fromBucket(bucket, props.code.key); | ||
| const { memorySize, timeout, ...rest } = props; |
There was a problem hiding this comment.
Do these still need to be extracted from props or would it be easier just to use props.memorySize on line 45?
There was a problem hiding this comment.
Ah yes thanks, I didn't realise this simpler version would work (but now I've read the docs and re-tested I can see that my version was needlessly complicated 😀).
src/constructs/lambda/lambda.ts
Outdated
| ...props, | ||
| ...rest, | ||
| memorySize: defaultMemorySize(props.runtime, memorySize), | ||
| timeout: props.timeout ? props.timeout : Duration.seconds(30), |
There was a problem hiding this comment.
Minor but the following should achieve the same result with marginally less code
| timeout: props.timeout ? props.timeout : Duration.seconds(30), | |
| timeout: props.timeout ?? Duration.seconds(30), |
| switch (runtime.family) { | ||
| case RuntimeFamily.JAVA: | ||
| return 1024; | ||
| default: | ||
| return 512; | ||
| } |
What does this change?
Whilst testing #186, I noticed that
memorySizeandtimeoutare optional parameters for aGuLambda. If these values are omitted, CloudFormation defaults to 128MB and 3 seconds, respectively.I don't think that these default values are particularly helpful for many of the Guardian use-cases that I've seen (they are especially unhelpful for Scala lambdas!), so I've tried to suggest something more appropriate.
Does this change require changes to existing projects or CDK CLI?
There are no required changes, but some projects may automatically increase the memory or timeout values for lambda functions. This change would show up in a snapshot diff though, so people could set these low values again manually if they felt strongly about keeping them.
How to test
I think this is adequately covered by unit tests added in this PR.
How can we measure success?
When setting projects up from scratch developers should save time, as they are less likely to encounter timeouts or OOM errors immediately after deploying for the first time.
Have we considered potential risks?
I suppose there is a small risk that this could incur unnecessary costs (due to the way that lambda pricing works).
There is also a risk that we will over or under provision lambdas due to my assumptions about what 'sensible' defaults are. An alternative approach would be to make these properties mandatory for a
GuLambda. This would force developers to make these choices up-front, but at least it would avoid the annoying case where you end up with the super-low CloudFormation defaults!