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.

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

feat(lambda): autoscaling for lambda aliases #8883

Merged
merged 47 commits into from
Aug 11, 2020
Merged

feat(lambda): autoscaling for lambda aliases #8883

merged 47 commits into from
Aug 11, 2020

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Jul 2, 2020

Add support for autoscaling provisioned concurrency on lambda (closes #6400).
To set autoscaling on a function alias use the addAutoScaling() method:

const alias = new lambda.Alias(stack, 'Alias', {
  aliasName: 'prod',
  version,
});

// Create AutoScaling Target
const as = alias.addAutoScaling({ maxCapacity: 50 })

// Configure target tracking
as.scaleOnUtilization({ utilizationTarget: 0.5 });

// or configure schedule scaling
as.scaleOnSchedule('ScaleUpInTheMorning', {
  schedule: appscaling.Schedule.cron({ hour: '8', minute: '0'}),
  minCapacity: 20,
});

It's possible to set PC on a function version, why didn't you add this option?
Following guidance from @iph:

The resource: AWS::Lambda::Version today is brittle and a sharp edge for many customers in how it behaves. We have a small flag in our cfn docs which needs to be bolder, in red, and said twice for ProvisionedConcurrencyConfig.
Versions do not support updates, so if you try to set a PC value on a version that already exists -- your cfn rolls back permanently. If you try to only update a PC configuration on a version cfn rolls back permanently. If you replace a version by changing the logical id, but set your version to retain-- your previous PC Configuration values are stranded, costing the customer. Overall, the Lambda team went back and forth whether we wanted to keep this functionality at all in cloudformation and decided "some customers may find a way to use it, and we should not inhibit them -- but direct everyone else to Alias-PC".
I would like us to retain that same philosophy in the L2 by not exposing the sharp edge to customers --they can still use it in the L1 if they so desire.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@kaizencc kaizencc marked this pull request as draft July 2, 2020 23:45
@kaizencc kaizencc requested a review from NetaNir July 2, 2020 23:48
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/lambda-version.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/lambda-version.ts Outdated Show resolved Hide resolved
NetaNir
NetaNir previously requested changes Jul 6, 2020
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/lambda-version.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/lambda-version.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/alias.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/alias.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed NetaNir’s stale review July 7, 2020 01:21

Pull request has been modified.

@kaizencc kaizencc marked this pull request as ready for review July 7, 2020 01:44
@kaizencc kaizencc requested a review from NetaNir July 7, 2020 15:25
@kaizencc kaizencc changed the title feat(lambda): add autoscaling target for provisioned concurrency lambdas feat(lambda): add autoscaling target for lambda aliases Jul 7, 2020
@NetaNir NetaNir requested a review from eladb July 28, 2020 18:17
packages/@aws-cdk/aws-lambda/package.json Outdated Show resolved Hide resolved
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 31, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. A few small comments below.

packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
*
* @param options Autoscaling options
*/
public addAutoScaling(options: AutoScalingOptions): IScalableFunctionAttribute {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be a better user experience if we change this to a getter autoscaling that (instantiates, if not already, and) returns scalableAlias?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to keep the same pattern as we have in all other L2 which implements autoscaling (ecs, ddb, ec2)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with returning IScalableFunctionAttribute in the addAutoScaling() method.

My question is if this should be -

export class ScalableFunctionAttribute extends appscaling.BaseScalableAttribute implements IScalableFunctionAttribute {

Keeps the type expectations in the correct place and any mismatches will be reported at the correct locations by the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, you are right, I have change it!

packages/@aws-cdk/aws-lambda/test/integ.autoscaling.ts Outdated Show resolved Hide resolved
Comment on lines 29 to 31
* Allowed values: 0.1 - 0.9.
*/
readonly utilizationTarget: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a nice default we can configure here? 0.9 maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping. This comment is not addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no good default here, every use case will have a different "best" target value, depending on the customer provision concurrency value, target cost and other usage pattern, requiring the value here makes the customer fully aware of the cost associate with the value.

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Aug 10, 2020
Comment on lines 29 to 31
* Allowed values: 0.1 - 0.9.
*/
readonly utilizationTarget: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ping. This comment is not addressed.

*
* @param options Autoscaling options
*/
public addAutoScaling(options: AutoScalingOptions): IScalableFunctionAttribute {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with returning IScalableFunctionAttribute in the addAutoScaling() method.

My question is if this should be -

export class ScalableFunctionAttribute extends appscaling.BaseScalableAttribute implements IScalableFunctionAttribute {

Keeps the type expectations in the correct place and any mismatches will be reported at the correct locations by the compiler.

Comment on lines 45 to 46
*
* Allowed values: 0.1 - 0.9.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Drop this in favour of the documentation on utilizationTarget property.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have removed it in all places you commented on, since I rename the file it does not show it.

maxCapacity: 20,
});

new cdk.CfnOutput(this, 'Output', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new cdk.CfnOutput(this, 'Output', {
new cdk.CfnOutput(this, 'FunctionName', {

*/
public scaleOnUtilization(options: UtilizationScalingOptions) {
if ( !Token.isUnresolved(options.utilizationTarget) && (options.utilizationTarget < 0.1 || options.utilizationTarget > 0.9)) {
throw new Error('Utilization Target should be between 0.1 and 0.9.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error('Utilization Target should be between 0.1 and 0.9.');
throw new Error(`Utilization Target should be between 0.1 and 0.9. Found ${options.utilizationTarget}`);

@NetaNir NetaNir removed the pr/do-not-merge This PR should not be merged at this time. label Aug 11, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 90138af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@NetaNir
Copy link
Contributor

NetaNir commented Aug 11, 2020

@Mergifyio refresh

@NetaNir NetaNir merged commit d9d9b90 into aws:master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-lambda: Simplify Lambda provisioned concurrency autoscaling
6 participants