Skip to content

Conversation

@jnsdls
Copy link
Member

@jnsdls jnsdls commented Apr 11, 2025

TL;DR

Updated the rate limiting implementation to improve handling of concurrent requests and added support for custom increment values.

What changed?

  • Modified the Redis interface to use get and incrBy instead of just incr
  • Changed the rate limiting logic to first read the current request count before incrementing
  • Added support for a custom increment parameter to allow incrementing by values other than 1
  • Improved error handling for Redis connection failures
  • Added more comprehensive test cases for various scenarios including Redis failures, concurrent requests, and custom increment values

How to test?

  • Run the existing test suite which now includes additional test cases for:
    • Redis connection failures
    • Handling of null/zero responses
    • Low sample rates
    • Concurrent requests with Redis lag
    • Custom increment values
  • Manually test the rate limiting functionality with different increment values

Why make this change?

This change addresses race conditions that could occur with concurrent requests by separating the read and increment operations. It also adds flexibility with the new increment parameter, allowing services to count certain requests as multiple units against the rate limit. The improved error handling makes the system more resilient to Redis connection issues.


PR-Codex overview

This PR focuses on updating the rateLimit function in the service-utils package to enhance its functionality by allowing custom increments and improving error handling for Redis operations.

Detailed summary

  • Added get, expire, and incrBy methods to the IRedis type.
  • Introduced an increment parameter to the rateLimit function.
  • Modified the logic to read request counts from Redis using get.
  • Implemented a new asynchronous increment logic for Redis.
  • Updated tests to reflect changes in Redis interactions and added new test cases for error handling and custom increments.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@vercel vercel bot temporarily deployed to Preview – wallet-ui April 11, 2025 19:17 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 April 11, 2025 19:17 Inactive
@vercel
Copy link

vercel bot commented Apr 11, 2025

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

Name Status Preview Comments Updated (UTC)
docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2025 8:28pm
login ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2025 8:28pm
thirdweb_playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2025 8:28pm
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2025 8:28pm
wallet-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2025 8:28pm

@vercel vercel bot temporarily deployed to Preview – login April 11, 2025 19:17 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground April 11, 2025 19:17 Inactive
@jnsdls jnsdls marked this pull request as ready for review April 11, 2025 19:17
@jnsdls jnsdls requested a review from a team as a code owner April 11, 2025 19:17
Copy link
Member Author

jnsdls commented Apr 11, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge-queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 11, 2025

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 47.67 KB (0%) 954 ms (0%) 264 ms (+196.04% 🔺) 1.3 s
thirdweb (cjs) 130.06 KB (0%) 2.7 s (0%) 353 ms (+38.74% 🔺) 3 s
thirdweb (minimal + tree-shaking) 5.62 KB (0%) 113 ms (0%) 108 ms (+1204.27% 🔺) 220 ms
thirdweb/chains (tree-shaking) 514 B (0%) 11 ms (0%) 68 ms (+2971.82% 🔺) 78 ms
thirdweb/react (minimal + tree-shaking) 19.36 KB (0%) 388 ms (0%) 120 ms (+602.47% 🔺) 507 ms

@jnsdls jnsdls force-pushed the Refactor_rate_limiting_to_use_separate_read/write_Redis_operations branch from 44861c1 to 76c1b47 Compare April 11, 2025 19:31
@vercel vercel bot temporarily deployed to Preview – login April 11, 2025 19:31 Inactive
@changeset-bot
Copy link

changeset-bot bot commented Apr 11, 2025

🦋 Changeset detected

Latest commit: aadc142

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@thirdweb-dev/service-utils Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel vercel bot temporarily deployed to Preview – wallet-ui April 11, 2025 19:31 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground April 11, 2025 19:31 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 April 11, 2025 19:31 Inactive
@jnsdls jnsdls force-pushed the Refactor_rate_limiting_to_use_separate_read/write_Redis_operations branch from 76c1b47 to 7cdf546 Compare April 11, 2025 19:32
@vercel vercel bot temporarily deployed to Preview – docs-v2 April 11, 2025 19:32 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground April 11, 2025 19:32 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui April 11, 2025 19:32 Inactive
@vercel vercel bot temporarily deployed to Preview – login April 11, 2025 19:32 Inactive
@jnsdls jnsdls force-pushed the Refactor_rate_limiting_to_use_separate_read/write_Redis_operations branch from 7cdf546 to 4221cb4 Compare April 11, 2025 19:43
@jnsdls jnsdls force-pushed the Refactor_rate_limiting_to_use_separate_read/write_Redis_operations branch from e987d55 to 9c30d3f Compare April 11, 2025 19:54
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground April 11, 2025 19:54 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 April 11, 2025 19:54 Inactive
@vercel vercel bot temporarily deployed to Preview – login April 11, 2025 19:54 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui April 11, 2025 19:54 Inactive
@jnsdls jnsdls merged commit d036b87 into main Apr 11, 2025
27 of 29 checks passed
@jnsdls jnsdls deleted the Refactor_rate_limiting_to_use_separate_read/write_Redis_operations branch April 11, 2025 20:26
@joaquim-verges joaquim-verges mentioned this pull request Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants