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

Add rate limited notifications queue #4162

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

davecreaser
Copy link
Contributor

@davecreaser davecreaser commented Jan 22, 2025

Description

Testing

  • ⚠️ You'll need to restart your dev environment and make sure you run npm run dev --notifications to enable notifications locally.
  • Once it's up and running, stop the block ingestor in docker. Open .env of the CDapp and copy the auto generated MAGICBELL_DEV_KEY, and add it to the .env of your block ingestor. Now run the block ingestor locally with npm run dev.
  • Now you need a way to trigger more than 60 notifications inside a minute. The easiest way is probably to open src/handlers/actions/mintTokens.ts in the block ingestor, and go to line 35 where it calls sendPermissionsActionNotifications. Just add a for loop around this call to fire it like 80 times when a mint token action happens.
for (let index = 0; index < 80; index++) {
      sendPermissionsActionNotifications({
        creator: initiatorAddress,
        colonyAddress,
        transactionHash,
        notificationCategory: NotificationCategory.Payment,
      });
    }
  • Open localhost, and mint some tokens. You should see 30 notifications come in pretty quickly, and after waiting a minute, more should come through.
  • Check the block ingestor logs, there should be no errors about API rate limits being hit.

Diffs

Changes 🏗

@davecreaser davecreaser marked this pull request as ready for review January 22, 2025 15:56
@davecreaser davecreaser requested a review from a team as a code owner January 22, 2025 15:56
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Really nice work on this one @davecreaser 🙌

Applied the changes in the local block-ingestor and created a mint tokens action
Screenshot 2025-01-23 at 09 24 45
There were no errors in the console
Screenshot 2025-01-23 at 09 28 26
Added an extra log to check the queue size and it seems the extra 20 notifications compared to the queue cap were added
Screenshot 2025-01-23 at 09 29 18

However @davecreaser, after creating many more Mint tokens actions, I did manage to still spot the API limit reached error. Not sure if I did something wrong?

Screenshot 2025-01-23 at 09 36 51

@davecreaser davecreaser requested a review from mmioana January 23, 2025 11:14
@davecreaser
Copy link
Contributor Author

Thanks @mmioana, turns out they had more hidden rate limits 🙄 I think this one happened because the front end was also sending a fetch request each time a new notification came in (to get that notification) and these were all combining to then hit the rate limit again.

I've reduced the block ingestor rate to only send 30 per minute, which will keep us nicely within the limits, and leave a big buffer for the front end to make requests.

Also added a retry mechanism with priority to the notifications queue, so that notifications which still hit the rate limit (unlikely hopefully) will get retried.

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice one 😎 And a really nice choice of package, so we don't need to run our own heavy duty queue instances for simple stuff like this 👌
image
image
Waited for a minute and posted some dank memes into our chat, and voila, here is the rest!
image

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Thanks for your really cool work on this issue @davecreaser 🥇

Applied the local changes to the block ingestor and created a Mint tokens action
Immediately I got 30 notifications
Screenshot 2025-01-27 at 15 49 20
No errors on the block ingestor side
Screenshot 2025-01-27 at 15 50 06
After 1 minute I got 30 more new notifications
Screenshot 2025-01-27 at 15 50 18
And after another minute, all notifications were there
Screenshot 2025-01-27 at 15 53 05

Awesome work! 🎉

@davecreaser davecreaser self-assigned this Jan 29, 2025
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Rate limiting working as expected, nothing triggered in the block ingestor logs, while notifications showing up in the frontend end.

Nice job fixing this

Screenshot from 2025-01-29 19-59-53
Screenshot from 2025-01-29 19-59-55

@davecreaser davecreaser merged commit 8b9b89b into master Jan 29, 2025
2 checks passed
@davecreaser davecreaser deleted the fix/notifications-rate-limiting branch January 29, 2025 18:21
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