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.

Sign up for GitHub

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

Jump to bottom

aws_lambda_nodejs: Unexisting AWS_NODEJS_CONNECTION_REUSE_ENABLED env variable set to 1 as default #29497

Closed
garysassano opened this issue Mar 14, 2024 · 4 comments · Fixed by #30117
Closed

aws_lambda_nodejs: Unexisting AWS_NODEJS_CONNECTION_REUSE_ENABLED env variable set to 1 as default #29497

garysassano opened this issue Mar 14, 2024 · 4 comments · Fixed by #30117
Assignees
@TheRealAmazonKendra @scanlonp
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@garysassano
Copy link

Describe the bug

The AWS SDK for JavaScript v3 is included by default in AWS Lambda Node.js 18 and Node.js 20 runtimes.

NodejsFunction construct comes with an awsSdkConnectionReuse optional prop, which is set to true by default. This default automatically sets the AWS_NODEJS_CONNECTION_REUSE_ENABLED environment variable to 1 in the Lambda function's configuration.

However, it has been identified that the AWS_NODEJS_CONNECTION_REUSE_ENABLED environment variable does not exist in AWS SDK for JavaScript v3, rendering the variable obsolete for those utilizing the more recent Lambda runtimes. This situation not only leads to unnecessary configuration, but it also has a performance impact. The inclusion of this redundant environment variable is associated with a 20ms increase in cold start times, highlighting an opportunity for performance improvement by removing this default setting for those who do not require it.

Expected Behavior

see above

Current Behavior

see above

Reproduction Steps

see above

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.132.1

Framework Version

No response

Node.js Version

20.11.1

OS

Ubuntu 22.04.3 LTS

Language

TypeScript

Language Version

No response

Other information

No response
@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 14, 2024
@garysassano
Copy link
Author

The new default for awsSdkConnectionReuse should either be always false, or:

  • false for nodejs20.x and nodejs18.x
  • true for nodejs16.x

@github-actions github-actions bot mentioned this issue Mar 18, 2024
Closed
@pahud
Copy link
Contributor

pahud commented Mar 18, 2024

Yes I agree with your proposed solution. Making this a p1 but we welcome PRs as well.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2024
@badmintoncryer badmintoncryer mentioned this issue Mar 19, 2024
Closed
1 task
@shikha372 shikha372 self-assigned this Mar 21, 2024
@TheRealAmazonKendra
Copy link
Contributor

This will actually become a moot point in just a few weeks or so. We are removing use of v2 from the CDK.

@github-actions github-actions bot mentioned this issue Apr 1, 2024
Closed
@markusl markusl mentioned this issue Apr 4, 2024
Open
@TheRealAmazonKendra TheRealAmazonKendra self-assigned this Apr 11, 2024
@scanlonp scanlonp assigned scanlonp and unassigned shikha372 May 7, 2024
@scanlonp scanlonp mentioned this issue May 8, 2024
Merged
@mergify mergify bot closed this as completed in #30117 May 20, 2024
mergify bot pushed a commit that referenced this issue May 20, 2024
@scanlonp
chore(lambda-nodejs): do not add unused environment variable by defau…
cf3599b 
…lt for sdk v3 lambda runtimes (#30117)

Closes #29497
Related to #29538

### Reason for this change

The `AWS_NODEJS_CONNECTION_REUSE_ENABLED` does not exist in SDK v3. Including it in the environment can add to cold start times.

### Description of changes

For Lambda runtimes >= Node 18, do not set the variable by default. If set explicitly, give the user a warning.

We can plan to simplify this logic & deprecate the property after we deprecate Node 16 and remove it as the default runtime for this construct.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions GitHub Actions
Copy link

⚠️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.

mazyu36 pushed a commit to mazyu36/aws-cdk that referenced this issue May 22, 2024
@scanlonp @mazyu36
chore(lambda-nodejs): do not add unused environment variable by defau…
e577502 
…lt for sdk v3 lambda runtimes (aws#30117)

Closes aws#29497
Related to aws#29538

### Reason for this change

The `AWS_NODEJS_CONNECTION_REUSE_ENABLED` does not exist in SDK v3. Including it in the environment can add to cold start times.

### Description of changes

For Lambda runtimes >= Node 18, do not set the variable by default. If set explicitly, give the user a warning.

We can plan to simplify this logic & deprecate the property after we deprecate Node 16 and remove it as the default runtime for this construct.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
atanaspam pushed a commit to atanaspam/aws-cdk that referenced this issue Jun 3, 2024
@scanlonp @atanaspam
chore(lambda-nodejs): do not add unused environment variable by defau…
75aa95d 
…lt for sdk v3 lambda runtimes (aws#30117)

Closes aws#29497
Related to aws#29538

### Reason for this change

The `AWS_NODEJS_CONNECTION_REUSE_ENABLED` does not exist in SDK v3. Including it in the environment can add to cold start times.

### Description of changes

For Lambda runtimes >= Node 18, do not set the variable by default. If set explicitly, give the user a warning.

We can plan to simplify this logic & deprecate the property after we deprecate Node 16 and remove it as the default runtime for this construct.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@TheRealAmazonKendra TheRealAmazonKendra

@scanlonp scanlonp

Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Milestone
No milestone
6 participants
@pahud @godwingrs22 @garysassano @shikha372 @TheRealAmazonKendra @scanlonp