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

feature: Make logger support async calls #425

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

mehulmpt
Copy link
Contributor

We use kysely on AWS lambdas (serverless) with context.callbackWaitsForEmptyEventLoop = false option. This helps us keep DB connection pool opened in background when lambda gets reused by AWS.

We want to log specific errors and immediately inform us over Slack if something unexpected happens with a specific query/error. We can setup a pipeline via Cloudwatch/lambda triggers however the use-case can also work for us if log function inside Kysely class can be awaited by Kysely internally.

The reason we cannot use .then() inside the logger function for making async network requests is because we run with callbackWaitsForEmptyEventLoop = false option where AWS freezes lambda the moment the callback is complete (which will happen unless kysely internally awaits the logger to return)

Please let me know if this needs any clarification

@igalklebanov igalklebanov added enhancement New feature or request api Related to library's API breaking change Includes breaking changes labels Apr 15, 2023
@koskimas
Copy link
Member

@igalklebanov Is this really a breaking change? Won't all the old code work with this change, or am I missing something?

@igalklebanov
Copy link
Member

igalklebanov commented Apr 15, 2023

@mehulmpt

Hey 👋

Thank you! 🙏

We use kysely on AWS lambdas (serverless) with context.callbackWaitsForEmptyEventLoop = false option. This helps us keep DB connection pool opened in background when lambda gets reused by AWS.

Best practice in AWS lambda is to manage connections/pools in global context (outside of your handler function scope). This way pools stay open as long as the lambda instance is up (can even last for 2-3 hours if it keeps getting requests).

Take advantage of execution environment reuse to improve the performance of your function. 
Initialize SDK clients and database connections outside of the function handler, and cache static assets locally in the /tmp directory. 
Subsequent invocations processed by the same instance of your function can reuse these resources. 
This saves cost by reducing function run time.

https://docs.aws.amazon.com/lambda/latest/dg/best-practices.html

This'll also remove the need to use context.callbackWaitsForEmptyEventLoop = false. It just brings more overhead/complexity with it. I have been developing in serverless environments for 5~ years now, and never needed this flag. There are better ways.

We want to log specific errors and immediately inform us over Slack if something unexpected happens with a specific query/error. We can setup a pipeline via Cloudwatch/lambda triggers however the use-case can also work for us if log function inside Kysely class can be awaited by Kysely internally.

Have you tried using something like Lumigo? You can group errors together and configure alerts that can be sent to Slack.

@igalklebanov igalklebanov removed the breaking change Includes breaking changes label Apr 15, 2023
@igalklebanov
Copy link
Member

igalklebanov commented Apr 15, 2023

@koskimas

@igalklebanov Is this really a breaking change? Won't all the old code work with this change, or am I missing something?

You're right, it's not. My bad.

@mehulmpt
Copy link
Contributor Author

mehulmpt commented Apr 15, 2023

Best practice in AWS lambda is to manage connections/pools in global context (outside of your handler function scope). This way pools stay open as long as the lambda instance is up (can even last for 2-3 hours if it keeps getting requests).

Just looked into RDS Proxy - we'll likely go with it. Coming from MongoDB Atlas, we weren't aware of this. Thanks!

Have you tried using something like Lumigo? You can group errors together and configure alerts that can be sent to Slack.

We had tried Sentry way back before. To say the least, in our experience at least it caused more problems than it solved (we traced back a very subtle bug with Sentry somewhere that crashed 1 in 100 requests with a 502 status code randomly). Removing sentry fixed it.

We currently have AWS X ray enabled with Cloudwatch - but would probably go with DB query errors manually for now (hence the PR).

Thanks for the quick approval and useful insights!

@igalklebanov igalklebanov merged commit 1b63aa8 into kysely-org:master Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants