-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Use process.on('SIGTERM')
for flushing in Vercel functions
#17583
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
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
if (process.env.VERCEL) { | ||
process.on('SIGTERM', async () => { | ||
// We have 500ms for processing here, so we try to make sure to have enough time to send the events | ||
await client.flush(200); |
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.
Did you test this on a vercel function? Generally makes sense but we could bump this more up I'd say?
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.
did not test this so far at all, maybe you can take this PR over when you have some time and properly test it. The reason I went with 200 here is because this is just the timeout used for finishing events, we still need time to actually make the http request so there needs to be some wiggle room -to the max. 500ms - if we spend 500ms on processing events, we have no more time to actually send the events 😅
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.
Ah yeah that makes sense I thought this time is included already, I'll take it over
This adds a better way to ensure we flush in node-based vercel functions.
Note that this does not work in edge functions, so we need to keep the
vercelWaitUntil
code around for this - but we can noop it in node and instead rely on the better way to handle this, I believe.@chargome when you're back, can you verify if this makes sense? 😅
Closes #17567