-
Notifications
You must be signed in to change notification settings - Fork 602
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
fix(lambda): reuse http clients #3931
base: main
Are you sure you want to change the base?
Conversation
@@ -1,8 +1,10 @@ | |||
import { GetParameterCommand, PutParameterCommand, SSMClient, Tag } from '@aws-sdk/client-ssm'; | |||
import { getTracedAWSV3Client } from '@terraform-aws-github-runner/aws-powertools-util'; | |||
|
|||
let ssmClient: SSMClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a lib, I don't want to create a new client on import, so we're caching a client only when the below functions are called;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, the caller should pass in a client
Thx for creating the PR. I thought that caching was not needed anymore with v3. I will check your PR next week. |
Have you seen that the canges made are breaking the tests? |
Interesting, I'll take a peek when I get some time 👀 |
Creating Http Clients per request is slow, as the client always has to re-establish a new connection with AWS. Not to mention, they're actually cause for concern when it comes to memory leaks, but fortunately with Lambdas they aren't long lived enough.
This PR changes the code to create single clients so we should see noticeable improvements with successive invocations of the Lambdas.
It's best practice to initialise these outside of the handler.