fix: cf cache ratelimits #2112
fix: cf cache ratelimits #2112
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 48 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes encompass the addition of new test files for validating rate limits on identities and API keys within the Unkey SDK. Modifications were made to logging behaviors in various components, including the circuit breaker and event router services. Enhancements to the caching mechanism in the rate limiter class were implemented, alongside adjustments to the calculation of acceptable limits in existing tests. These updates aim to refine the accuracy and reliability of rate limiting functionality across the system. 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: 8
Outside diff range and nitpick comments (3)
apps/agent/integration/keys/ratelimits_test.go (2)
45-46: Optimize Test Duration for CI EfficiencyThe test durations of 1 minute and 5 minutes can significantly lengthen the CI pipeline. Consider reducing the
testDurationor implementing mock rate limits to simulate the desired conditions more efficiently.Also applies to: 49-50
81-83: Improve Variable Naming for ClarityRenaming variables
totalandpassedtototalRequestsandsuccessfulRequestsenhances readability and better reflects their purpose.Apply this diff for clearer naming:
-total := 0 -passed := 0 +totalRequests := 0 +successfulRequests := 0 ... -for valid := range results { - total++ - if valid { - passed++ +for isValid := range results { + totalRequests++ + if isValid { + successfulRequests++ } } ... t.Logf("Total: %d, Passed: %d, lowerLimit: %d, exactLimit: %d, upperLimit: %d", total, passed, lowerLimit, exactLimit, upperLimit) +// Update log statement with new variable names +t.Logf("Total: %d, Passed: %d, lowerLimit: %d, exactLimit: %d, upperLimit: %d", totalRequests, successfulRequests, lowerLimit, exactLimit, upperLimit) ... -require.GreaterOrEqual(t, passed, lowerLimit) -require.LessOrEqual(t, passed, upperLimit) +require.GreaterOrEqual(t, successfulRequests, lowerLimit) +require.LessOrEqual(t, successfulRequests, upperLimit)Also applies to: 99-106
apps/agent/integration/identities/identities_ratelimits_accuracy_test.go (1)
39-39: Review commented-out test cases for multiple keysAt line 39, the loop over
nKeysonly includes[1], with other values commented out (//, 3, 10, 1000}). If you intend to test with multiple keys, consider uncommenting these values to include them in the test. If they are not needed, removing them can clean up the code.
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 (11)
- apps/agent/integration/identities/identities_ratelimits_accuracy_test.go (1 hunks)
- apps/agent/integration/keys/ratelimits_test.go (1 hunks)
- apps/agent/pkg/circuitbreaker/lib.go (1 hunks)
- apps/agent/pkg/clickhouse/flush.go (0 hunks)
- apps/agent/pkg/testutil/attack.go (1 hunks)
- apps/agent/services/eventrouter/service.go (0 hunks)
- apps/api/src/pkg/middleware/init.ts (2 hunks)
- apps/api/src/pkg/middleware/metrics.ts (1 hunks)
- apps/api/src/pkg/ratelimit/client.ts (7 hunks)
- apps/api/src/routes/v1_keys_verifyKey.ratelimit_accuracy.test.ts (1 hunks)
- apps/api/src/routes/v1_ratelimit_limit.accuracy.test.ts (1 hunks)
Files not reviewed due to no reviewable changes (2)
- apps/agent/pkg/clickhouse/flush.go
- apps/agent/services/eventrouter/service.go
Files skipped from review due to trivial changes (2)
- apps/agent/pkg/circuitbreaker/lib.go
- apps/api/src/pkg/middleware/metrics.ts
Additional comments not posted (14)
apps/agent/pkg/testutil/attack.go (3)
10-13: LGTM!The
Ratestruct is well-defined with clear and self-explanatory field names. TheFreqfield represents the frequency of function calls, and thePerfield represents the time period over which the function calls should occur.
15-17: LGTM!The
String()method provides a clear and concise string representation of theRatestruct by including both theFreqandPerfields. The use offmt.Sprintfis appropriate for formatting the string.
24-69: LGTM!The
Attackfunction is well-implemented and suitable for performance testing or stress testing scenarios. The use of goroutines and channels for concurrent execution is appropriate and efficient. The function signature is well-defined and uses generics to allow for any response type. The use of a wait group to track the completion of all function calls is a good practice. The logging statements provide useful information about the execution state.apps/api/src/routes/v1_keys_verifyKey.ratelimit_accuracy.test.ts (1)
97-97: Consider the impact on test sensitivity.The change increases the lower limit from 75% to 95% of the exact limit. This means the test will now permit a slightly higher number of passed results before failing.
While this allows for some margin to account for rate limit variations, it's important to ensure that the test remains effective at catching rate limit violations that are close to the exact limit.
Please consider the trade-off between allowing some leniency and maintaining the test's ability to identify violations. If the increased tolerance is acceptable based on the specific requirements and expected behavior of the rate limiting functionality being tested, then this change should be good to merge.
apps/api/src/routes/v1_ratelimit_limit.accuracy.test.ts (1)
100-100: Verify the reasoning behind the change in the lower limit calculation.The change in the calculation of the
lowerLimitfrom 75% to 95% of theexactLimitmay affect the pass/fail criteria of the tests, potentially making them more lenient.Please ensure that the new lower limit still provides an adequate level of accuracy and effectiveness in testing the rate limiting functionality. Consider providing additional context or justification for this change to ensure it aligns with the expected behavior of the rate limiting functionality.
apps/api/src/pkg/middleware/init.ts (1)
40-44: LGTM!The changes ensure that the
isolateCreatedAtvalue is initialized only once with the current timestamp when it is undefined. This fixes the issue where the timestamp was being reset with each invocation of theinitfunction.The benefits of this fix include:
- Preserving the original creation timestamp of the isolate across subsequent requests.
- Ensuring consistency of the
isolateCreatedAtvalue for the same isolate.- Improving the reliability of the timestamp for tracking and debugging purposes.
Setting the
isolateCreatedAtvalue in the context object makes it accessible to other parts of the application.apps/api/src/pkg/ratelimit/client.ts (7)
17-17: LGTM!The addition of the
blockedproperty to the cache entry type aligns with the PR objective and enhances the caching mechanism to track blocked requests.
23-23: Looks good!The constructor has been updated to accept a cache with the new cache entry type, ensuring consistency with the
blockedproperty addition.
38-38: Nice refactor!The method rename and the addition of the
blockedparameter improve the clarity and functionality of the cache setting method.
58-58: LGTM!Setting the cache with the updated structure, including the
blockedproperty, ensures that the cache accurately reflects the state of the rate limiter.
126-126: Looks good!The default value for
cachedhas been updated to include theblockedproperty, ensuring consistency with the updated cache entry type.
Line range hint
127-134: Great optimization!Checking the
blockedstate early and returning a blocked response iftrueallows the system to bypass further processing for already blocked requests. This optimization can lead to more efficient request handling.
173-173: LGTM!Updating the cache with the correct
blockedstate based on the rate limiting results ensures that the cache accurately reflects the state of the requests. Settingblockedto!res.val.passand explicitly setting it tofalsewhen necessary maintains the integrity of the cache.Also applies to: 184-184, 207-207
apps/agent/integration/keys/ratelimits_test.go (1)
34-36: Remove Redundant Check for EmptybaseURLSince
UNKEY_BASE_URLis already required to be set and not empty (line 28), the conditionif baseURL != ""is unnecessary. You can simplify the code by directly appending theWithServerURLoption.Apply this diff to streamline the code:
if baseURL != "" { options = append(options, unkey.WithServerURL(baseURL)) } +// Since baseURL is guaranteed to be non-empty, the check is redundant. +options = append(options, unkey.WithServerURL(baseURL))Likely invalid or redundant comment.
apps/agent/integration/identities/identities_ratelimits_accuracy_test.go
Show resolved
Hide resolved
apps/agent/integration/identities/identities_ratelimits_accuracy_test.go
Show resolved
Hide resolved
apps/agent/integration/identities/identities_ratelimits_accuracy_test.go
Outdated
Show resolved
Hide resolved
apps/agent/integration/identities/identities_ratelimits_accuracy_test.go
Show resolved
Hide resolved
* fix: default ratelimits * revert * fix: cache ratelimits on cloudflare correctly * chore: remove logs * chore: remove log * perf: remove unnecessary switch * fix: track isolate start time * test: tighten lower ratelimit threshold * fix: only cache ratelimit blocks * chore: sync lockfile * test: improve accuracy of lower limit calculation in rate limit tests * fix: address rabbit suggestions
* fix: default ratelimits * revert * fix: cache ratelimits on cloudflare correctly * chore: remove logs * chore: remove log * perf: remove unnecessary switch * fix: track isolate start time * test: tighten lower ratelimit threshold * fix: only cache ratelimit blocks * chore: sync lockfile * test: improve accuracy of lower limit calculation in rate limit tests * fix: address rabbit suggestions
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor