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

aws_lambda_nodejs: Using Lambda Provided SDK by default in NodejsFunction leads to higher cold starts #25492

Closed
1 of 2 tasks
trivikr opened this issue May 9, 2023 · 6 comments
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@trivikr
Copy link
Member

trivikr commented May 9, 2023

Describe the feature

The BundlingOptions in NodejsFunction construct removes AWS SDK dependencies by default.

This uses Lambda Provided SDK in the resulting function. This has higher cold start than a bundled function with AWS SDK dependencies included.

This happens, because the Node.js runtime has to do module resolution and go through multiple files while reading dependency code in the bundled function which uses Lambda Provided SDK. When SDK in bundled with the function code, the cold starts are lower as the as Node.js runtime has to read single file without any module resolution.

Use Case

The default NodeJsFunction created by CDK Lambda should have low cold starts.

Lambda customers are usually sensitive to cold starts. The bundling of the source code already reduces the function size, which is way below the Lambda Quotas limit of 50 MB.

For repro, refer README of https://github.com/trivikr/aws-cdk-lambda-nodejs-function-cold-start-latency

Here are the benchmark numbers in ms:

{
  'NodejsFunction default (uses Lambda Provided SDK)': 1227.1435,
  'NodejsFunction custom (uses Customer Deployed SDK)': 929.441
}

Notes:

  • The benchmarks were run on count 1000 to ensure they’re reliable.
  • As of 2023-05-09, it ensured that the SDK version is same in both cases, i.e. 3.188.0
  • The numbers show median value for all cold start invocations. For other statistics, refer to output of measure-cold-start Step Function execution.

Proposed Solution

The default NodeJsFunction created by CDK Lambda should not exclude AWS SDK dependencies.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.76.0

Environment details (OS name and version, etc.)

N/A

@trivikr trivikr added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 9, 2023
@pahud
Copy link
Contributor

pahud commented May 9, 2023

Nice callout and thanks for the detailed report. Maybe we should allow users to opt in with an optional property but I am not sure. But yes, the cold start is a pain we can't ignore.

@pahud pahud added p1 effort/medium Medium work item – several days of effort needs-review and removed needs-triage This issue or PR still needs to be triaged. labels May 9, 2023
@trivikr
Copy link
Member Author

trivikr commented May 9, 2023

Maybe we should allow users to opt in with an optional property but I am not sure

Users can pass empty array in externalModules to bundle the AWS SDK dependencies.

The feature request is whether CDK should bundle SDK dependencies by default, as it has lower cold starts.

mergify bot pushed a commit that referenced this issue Mar 5, 2024
…ode asset (#29207)

### Issue # (if applicable)
#25492.

Closes #<issue number here>.
#25492.

### Reason for this change
The BundlingOptions in NodejsFunction construct removes AWS SDK dependencies by default.

This uses Lambda Provided SDK in the resulting function. This has higher cold start than a bundled function with AWS SDK dependencies included.

This happens, because the Node.js runtime has to do module resolution and go through multiple files while reading dependency code in the bundled function which uses Lambda Provided SDK. When SDK in bundled with the function code, the cold starts are lower as the as Node.js runtime has to read single file without any module resolution.

Result from reproduction:

{
'NodejsFunction default (uses Lambda Provided SDK)': 1227.1435,
'NodejsFunction custom (uses Customer Deployed SDK)': 929.441
}

related to this issue: #25492



### Description of changes
While maintaining backward compatibility, an new option `useAwsSDK` was introduced to include the sdk in the code asset


yes kindly refer to the above

### Description of how you validated changes
Added both unit and integration test 


yes 

```
Running integration tests for failed tests...

Running in parallel across regions: us-east-1, us-east-2, us-west-2
Running test /Users/jonife/Documents/dev/lambda-tooling/cdk/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.dependencies.js in us-east-1
  SUCCESS    aws-lambda-nodejs/test/integ.dependencies-LambdaDependencies/DefaultTest 329.553s
       AssertionResultsLambdaInvoke5050b1f640cc49956b59f2a71febe95c - success
      AssertionResultsLambdaInvokee35a5227846e334cb95a90bacfbfb877 - success
      AssertionResultsLambdaInvoke7d0602e4b9f40ae057f935d874b5f971 - success

Test Results: 

Tests:    1 passed, 1 total
✨  Done in 337.42s.
```

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@trivikr
Copy link
Member Author

trivikr commented Mar 29, 2024

Looks like this issue was fixed in #29207

Can it be closed @jonife ?

@jonife
Copy link
Contributor

jonife commented Apr 1, 2024

yes we can close this issue

Copy link

github-actions bot commented Apr 1, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@WtfJoke
Copy link
Contributor

WtfJoke commented Apr 28, 2024

@jonife I am not sure if this should have been closed.

The new variable bundleAwsSDK defaults to false, e.g. slow cold starts.
At least the doc changes to externalModules are not correct, since with the default value false of bundleAwsSDK externalModulesReplacement are still made.

Mentioned docs:

* @default - no replacements are made

Comparing it with the current code:
// Don't automatically externalize any dependencies when using a `latest` runtime which may
// update versions in the future.
// Don't automatically externalize aws sdk if `bundleAwsSDK` is true so it can be
// include in the bundle asset
const defaultExternals = props.runtime?.isVariable || props.bundleAwsSDK ? [] : versionedExternals;
const externals = props.externalModules ?? defaultExternals;

Following (failing) test confirms that behaviour:

test('esbuild bundling includes aws-sdk by default', () => {
  Bundling.bundle(stack, {
    entry,
    projectRoot,
    depsLockFilePath,
    runtime: Runtime.NODEJS_18_X,
    architecture: Architecture.X86_64,
  });

  // Correctly bundles with esbuild
  expect(Code.fromAsset).toHaveBeenCalledWith(path.dirname(depsLockFilePath), {
    assetHashType: AssetHashType.OUTPUT,
    bundling: expect.objectContaining({
      command: [
        'bash', '-c',
        `esbuild --bundle "/asset-input/lib/handler.ts" --target=${STANDARD_TARGET} --platform=node --outfile="/asset-output/index.js"`,
      ],
    }),
  });
});
    -       "esbuild --bundle \"/asset-input/lib/handler.ts\" --target=node18 --platform=node --outfile=\"/asset-output/index.js\"",
    +       "esbuild --bundle \"/asset-input/lib/handler.ts\" --target=node18 --platform=node --outfile=\"/asset-output/index.js\" --external:@aws-sdk/*",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
5 participants