feat: use cloudflare's ratelimiter if possible #2544
Conversation
According to our data, these 5 ratelimit configs cover nearly 75% of our ratelimit operations. By using cloudflare for these, I expect our latency to drop and accuracy to increase
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements related to rate limiting functionality across multiple files. A new constant Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/api/wrangler.toml (2)
39-45: Fix typo in comment.There's a typo in the comment: "nameing" should be "naming"
-# The nameing scheme is important, because we're dynamically constructing +# The naming scheme is important, because we're dynamically constructing
Line range hint
38-261: Consider environment-specific rate limits.The rate limit configurations are identical across all environments (development, preview, canary, and production). While this consistency can be beneficial, consider:
- Whether production needs different (possibly stricter) limits
- Whether canary should match production exactly (as noted in the comment that canary settings should match production)
- Whether development/preview environments could benefit from more relaxed limits
apps/api/src/pkg/ratelimit/client.ts (1)
91-93: Include error stack trace in logs for better debuggingWhen logging the error in the catch block, consider including the stack trace to facilitate easier debugging and troubleshooting.
Apply this diff to enhance error logging:
this.logger.error("cfrl failed, falling back to agent", { error: (err as Error).message, + stack: (err as Error).stack, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
- apps/api/src/pkg/env.ts (2 hunks)
- apps/api/src/pkg/ratelimit/client.ts (2 hunks)
- apps/api/wrangler.toml (4 hunks)
- internal/metrics/src/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
apps/api/src/pkg/env.ts (1)
49-53: Document rate limit configurations and verify bindings.While the naming is clear, adding documentation for each rate limit configuration would improve maintainability. Also, let's verify these match the wrangler.toml bindings.
Consider adding JSDoc comments:
+ /** Rate limit: 10 requests per 60 seconds */ RL_10_60s: cloudflareRatelimiter, + /** Rate limit: 30 requests per 60 seconds */ RL_30_60s: cloudflareRatelimiter, + /** Rate limit: 200 requests per 60 seconds */ RL_200_60s: cloudflareRatelimiter, + /** Rate limit: 500 requests per 10 seconds */ RL_500_10s: cloudflareRatelimiter, + /** Rate limit: 200 requests per 10 seconds */ RL_200_10s: cloudflareRatelimiter,Let's verify the bindings in wrangler.toml:
✅ Verification successful
Rate limit configurations are properly bound in wrangler.toml
The verification shows that all rate limit configurations in
env.tshave corresponding bindings inwrangler.tomlwith correct names and types. Each rate limiter is properly configured as a "ratelimit" type with a unique namespace ID. The original suggestion for adding JSDoc comments still stands as a documentation improvement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the rate limit configurations match in wrangler.toml # Expected: Each rate limit should have corresponding bindings in wrangler.toml echo "Checking rate limit bindings in wrangler.toml..." rg "RL_(10|30|200|500)_(10|60)s" -A 2Length of output: 3246
internal/metrics/src/index.ts (2)
82-82: LGTM: Mode enum updated to support Cloudflare ratelimiterThe addition of "cloudflare" to the mode enum aligns with the PR objective of implementing Cloudflare's ratelimiter for specific configurations.
85-85: LGTM: Source enum updated for Cloudflare integrationThe addition of "cloudflare" to the source enum enables proper tracking of metrics from the Cloudflare ratelimiter implementation.
Let's verify the consistency of these enum values across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information to verify the consistency of the enum values across the codebase.
Verified: Source and mode enums are consistently used across the codebase
The verification confirms that:
- The source enum values ("agent", "durable_object", "cloudflare") are consistently defined in
internal/metrics/src/index.tsand correctly used inapps/api/src/pkg/ratelimit/client.ts- The mode enum values ("sync", "async", "cloudflare") are properly implemented and referenced across the codebase, including:
- Schema definition in
internal/metrics/src/index.ts- Implementation in
apps/api/src/pkg/ratelimit/client.ts- Documentation in
apps/docs/ratelimiting/modes.mdx- UI components in the dashboard
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ratelimit modes and sources # Expected: All references should use consistent enum values # Check for ratelimit mode usage echo "Checking ratelimit mode usage..." rg -A 2 'mode.*sync|mode.*async|mode.*cloudflare' # Check for ratelimit source usage echo "Checking ratelimit source usage..." rg -A 2 'source.*agent|source.*durable_object|source.*cloudflare'Length of output: 4272
apps/api/wrangler.toml (3)
38-74: Review the rate limit values for development environment.The rate limit configurations look reasonable, but consider if the development environment should have more lenient limits to facilitate testing and debugging.
Would it make sense to:
- Increase limits in development for easier testing?
- Add documentation about how these limits were chosen (e.g., based on usage patterns)?
44-45: Consider documenting namespace ID collision prevention.The namespace ID schema using
9900prefix is duplicated across all environments. While this might be intentional, it would be helpful to document:
- Whether these IDs need to be unique across environments
- How namespace ID collisions are prevented
- Why the specific prefix
9900was chosen#!/bin/bash # Check if namespace IDs are unique across all environments echo "Checking for duplicate namespace IDs..." rg "namespace_id = \"[0-9]+\"" -o | sort | uniq -dAlso applies to: 103-111, 162-170, 224-232
Line range hint
38-261: Verify dynamic construction of rate limit bindings.The comment mentions that "The naming scheme is important, because we're dynamically constructing these in the api code". Let's verify how these bindings are being used.
✅ Verification successful
Let me try a different search strategy to find how these rate limit bindings are used in the code.
Based on the search results, I can now provide a final response about the rate limit bindings verification:
Rate limit bindings are correctly configured and used in the codebase
The rate limit bindings defined in
wrangler.tomlfollow a specific naming scheme (RL_{LIMIT}_{DURATION}s) that is used dynamically in the API code. The verification shows:
- The bindings are properly defined in
apps/api/src/pkg/env.tswith matching names:RL_10_60s,RL_30_60s,RL_200_60s,RL_500_10s,RL_200_10s- The rate limiter client in
apps/api/src/pkg/ratelimit/client.tsdynamically constructs the binding key using the same pattern:`RL_${req.limit}_${Math.round(req.interval / 1000)}s`- The namespace IDs follow the documented schema with the
9900prefix followed by limit and duration🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dynamic construction of rate limit binding names echo "Searching for rate limit binding usage..." rg "RL_\d+_\d+s" --type ts -B 2 -A 2 # Search for potential parsing of these bindings echo "Searching for binding name parsing..." ast-grep --pattern 'RL_$_$_' --lang tsLength of output: 626
Script:
#!/bin/bash # Search for rate limit related code echo "Searching for rate limit related code..." rg "ratelimit|rateLimiter" --type ts -B 2 -A 2 # Search for env.bindings usage echo "Searching for env.bindings usage..." rg "env\.bindings" --type ts -B 2 -A 2 # Search for specific rate limit binding names echo "Searching for specific rate limit binding names..." rg "RL_10_60s|RL_30_60s|RL_200_60s|RL_500_10s|RL_200_10s" --type ts -B 2 -A 2Length of output: 334069
apps/api/src/pkg/ratelimit/client.ts (2)
3-3: LGTM!The import of
cloudflareRatelimiteris appropriate and correctly references the module from"../env".
69-94: Integration of Cloudflare rate limiter is well-implementedThe try-catch block properly attempts to use the Cloudflare rate limiter when the binding exists and gracefully falls back to the agent rate limiter in case of failure. This implementation enhances reliability by ensuring that rate limiting continues to function even if the Cloudflare rate limiter is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/api/src/pkg/ratelimit/client.ts (1)
89-89: Consider preserving actual values in the responseThe response currently uses -1 as placeholder values for
reset,current, andremaining. This might affect monitoring and debugging capabilities. Consider extracting these values from the Cloudflare response if available.- return Ok({ pass: res.success, reset: -1, current: -1, remaining: -1, triggered: null }); + return Ok({ + pass: res.success, + reset: res.reset ?? -1, + current: res.current ?? -1, + remaining: res.remaining ?? -1, + triggered: null + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
- apps/api/src/pkg/ratelimit/client.ts (2 hunks)
- internal/metrics/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/metrics/src/index.ts
🧰 Additional context used
🔇 Additional comments (2)
apps/api/src/pkg/ratelimit/client.ts (2)
3-5: LGTM: Clean import integrationThe new import for cloudflareRatelimiter is properly integrated and aligns with the PR's objective to use Cloudflare's ratelimiter.
69-96: LGTM: Robust Cloudflare integration with fallbackThe implementation correctly:
- Attempts Cloudflare ratelimiter only for async requests
- Uses a clear naming convention for binding keys
- Gracefully falls back to the agent on failure
- Maintains proper metrics tracking
According to our data, these 5 ratelimit configs cover nearly 75% of our
ratelimit operations.
By using cloudflare for these, I expect our latency to drop and accuracy
to increase
Summary by CodeRabbit
New Features
Bug Fixes
Documentation