Add parameter to power-tune only cold starts #206
Conversation
| @@ -5,7 +5,8 @@ | |||
| "Effect": "Allow", | |||
| "Action": [ | |||
| "lambda:InvokeFunction", | |||
| "lambda:GetFunctionConfiguration" | |||
| "lambda:GetFunctionConfiguration", | |||
There was a problem hiding this comment.
@sfloresk all the Terraform edits here will need to be ported to the new repo :)
There was a problem hiding this comment.
Link to the terraform repo
https://github.com/sfloresk/terraform-aws-lambda-power-tuning
|
Ok, merging the latest wasn't too bad after all :) |
# Conflicts: # lambda/utils.js # template.yml # test/unit/test-utils.js
| @@ -5,7 +5,8 @@ | |||
| "Effect": "Allow", | |||
| "Action": [ | |||
| "lambda:InvokeFunction", | |||
| "lambda:GetFunctionConfiguration" | |||
| "lambda:GetFunctionConfiguration", | |||
There was a problem hiding this comment.
Link to the terraform repo
https://github.com/sfloresk/terraform-aws-lambda-power-tuning
|
Implemented the Terraform changes in the separate repo, happy to open a PR once we have the green light on the changes here |
…hine transitions cost
lambda/executor.js
Outdated
| // use results (which include logs) to compute average duration ... | ||
|
|
||
| const durations = utils.parseLogAndExtractDurations(results); | ||
| if (onlyColdStarts) { | ||
| // we care about the init duration in this case | ||
| const initDurations = utils.parseLogAndExtractInitDurations(results); |
There was a problem hiding this comment.
@mriccia this is new - I am manually adding the init duration to the billed duration. This might not be the ideal solution for all cases, but it does work for what we need when onlyColdStarts=true. Does that make sense to you?
There was a problem hiding this comment.
I think there's a problem with this. The cost is derived from the durations array, now that we're adding the initDuration in case of cold start the cost calculation will be inaccurate.
There was a problem hiding this comment.
I changed the way the duration and the cost are calculated.
Please take a look when you have a moment, there are some Tests missing (planning to add early next week), but should be good to go if you're happy with this.
| // defaulting the index to 0 as the index is required for onlyColdStarts | ||
| let aliasToInvoke = utils.buildAliasString(lambdaAlias, onlyColdStarts, 0); | ||
| // We need the architecture, regardless of onlyColdStarts or not | ||
| const {architecture, isPending} = await utils.getLambdaConfig(lambdaARN, aliasToInvoke); |
There was a problem hiding this comment.
Looking at what the Initializer is doing (very similarly), I would consider merging utils.getLambdaPower and utils.getLambdaConfig since they are both using GetFunctionConfigurationCommand and they're simply retrieving different fields from the result.
There was a problem hiding this comment.
Not blocking for this PR - just a reminder for myself in the future :)
| return { | ||
| power: config.MemorySize, | ||
| // we need to fetch env vars only to add a new one and force a cold start | ||
| description: config.Description, |
There was a problem hiding this comment.
As mentioned in another comment, this implementation of getLambdaPower has become very similar to getLambdaConfig below. It used to make more sense because it was a specialized version that only cared about config.MemorySize and returned a single value. But now it returns a structure with various config parameters, so I'd consider merging getLambdaPower and getLambdaConfig back into one single implementation where we fetch all the config parameters we need.
Fix broken tests based on new logic
Implement #176
This is another implementation based on #177
With this implementation the mechanism for publishing new versions is decoupled, and the Step Function workflow implements a loop to publish the versions sequentially.
This is done to avoid hitting the Timeout on the Lambda function publishing the Versions.