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

Qstash implementation #135

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Qstash implementation #135

wants to merge 3 commits into from

Conversation

rozig
Copy link

@rozig rozig commented Aug 17, 2022

Description

Implemented Qstash to handle webhook events asynchronously.

Workflow:

  1. Mux webhook delivers message to /api/webhooks/mux
  2. stream.new verifies webhook signature
  3. stream.new publishes webhook event to qStash for later processing
  4. stream.new responds to mux webhook

In the background:

  1. qStash delivers published message to /api/webhooks/qstash
  2. stream.new verifies qstash signature
  3. stream.new processes webhook event

@vercel
Copy link

vercel bot commented Aug 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
stream-new ✅ Ready (Inspect) Visit Preview Aug 23, 2022 at 0:15AM (UTC)

@rozig rozig requested a review from dylanjha August 17, 2022 01:03
Copy link
Collaborator

@dylanjha dylanjha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good and QStash looks very nice and easy to use.

Curious:

  • It looks like QStash is Redis-backed by UpStash, is there an option to bring our own Redis?
  • How long can the QStash jobs run for? Is there a limit to those requests that hit the /api/webhooks/qstash endpoint?

I think the next step here would be to get feedback internally from other folks on DevEx to see how we feel about making this part of the official example.

Assuming we decide on that, the README will need some updates in the section around webhook handling. We may even want to tweak the implementation a bit to make the QStash part optional so that:

  • If QSTASH_* ENV vars are set, then we use QStash
  • If not, then we process the webhook synchronously

Many of the features of stream.new are implemented in that optional way based on the existence of env vars (Google content moderation, Hive content moderation, Slackbot moderator, etc.). The reason for this is that folks use stream.new as a reference application, so it gives some flexibility to get the application running quickly with minimal setup and then enabling functionality as needed.

Overall, I really like this and it'll solve the webhook timeout issues we've been seeing 🙌🏼

pages/api/webhooks/qstash.ts Outdated Show resolved Hide resolved
.env.local.example Outdated Show resolved Hide resolved
@rozig
Copy link
Author

rozig commented Aug 17, 2022

This looks really good and QStash looks very nice and easy to use.

Curious:

  • It looks like QStash is Redis-backed by UpStash, is there an option to bring our own Redis?
  • How long can the QStash jobs run for? Is there a limit to those requests that hit the /api/webhooks/qstash endpoint?

I think the next step here would be to get feedback internally from other folks on DevEx to see how we feel about making this part of the official example.

Assuming we decide on that, the README will need some updates in the section around webhook handling. We may even want to tweak the implementation a bit to make the QStash part optional so that:

  • If QSTASH_* ENV vars are set, then we use QStash
  • If not, then we process the webhook synchronously

Many of the features of stream.new are implemented in that optional way based on the existence of env vars (Google content moderation, Hive content moderation, Slackbot moderator, etc.). The reason for this is that folks use stream.new as a reference application, so it gives some flexibility to get the application running quickly with minimal setup and then enabling functionality as needed.

Overall, I really like this and it'll solve the webhook timeout issues we've been seeing 🙌🏼

Thanks for the review @dylanjha :)

About using our own redis, let me get back to you after doing some research.
About timeout and limit, you can get all the information through this link: https://docs.upstash.com/qstash/pricing

Also I already started working on making QStash optional depending on the variable (just like how we have mux webhooks). One of the other solution I was thinking is using BullMQ (library), it allows us to use redis as message queue and will also provide more features and control to us. But it won't be serverless, we will need another instance to run BullMQ workers.

Read more about BullMQ: https://docs.bullmq.io/

@rozig
Copy link
Author

rozig commented Aug 17, 2022

  • It looks like QStash is Redis-backed by UpStash, is there an option to bring our own Redis?

It looks like Upstash does not provide an option to bring our own redis cluster. They do provide an option to publish message additionally to kafka (only kafka) topic, but can't find anything to replace underlying redis.

Comment on lines +9 to +21
type WebhookRequestBody = {
type: string;
object: any;
id: string;
environment: any;
data: any;
playback_ids?: any[];
duration: number;
created_at: string;
accessor_source: string;
accessor: string;
request_id: string;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a place where I can get all available fields of the webhook request?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has been some work on this recently by techops team, ask in #devex-dev channel

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the spec/webhook/samples directory in this PR: https://github.com/muxinc/openapi-specification/pull/181

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jaredsmith :)

@dylanjha
Copy link
Collaborator

It looks like Upstash does not provide an option to bring our own redis cluster.

Cool, not a blocker, was just curious

Also I already started working on making QStash optional depending on the variable (just like how we have mux webhooks). One of the other solution I was thinking is using BullMQ (library), it allows us to use redis as message queue and will also provide more features and control to us. But it won't be serverless, we will need another instance to run BullMQ workers.

@rozig makes sense. I say we stick with the serverless solution so you go one of two ways:

  • QSTASH env vars (process async)
  • no QSTASH env vars (process sync)

About timeout and limit, you can get all the information through this link: https://docs.upstash.com/qstash/pricing

Makes sense on the QStash side of things, but there is a concern on the Vercel side. The endpoint hits /api/webhooks/qstash, and that request runs on Vercel. I found the [limits here](https://vercel.com/docs/concepts/limits/overview], looks like 10seconds for Hobby, 60 seconds for Pro and 900 seconds for Enterprise. We're definitely not Hobby, so we should be good with the 60 second limit

@chronark
Copy link

Hey guys,
I built QStash and just stumbled across this.

  1. No we do not support running your own redis server and there are no plans for this.
  2. By default the timeout is 30s, but just let us know if you need more.

Thanks for choosing QStash and please let me know if you have any questions.
I'd be happy to help out.

@dylanjha
Copy link
Collaborator

Thanks @chronark! Appreciate the comment. Some of the folks on the team are busy with a conference this week, but we intend to loop back on this when we get back and get this wrapped up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants