Rate limit alerts by channel for task run alerts using generic cell rate algo #1679
Conversation
|
WalkthroughThis pull request introduces several new environment properties for configuring alert rate limiting and Redis integration. It adds a new GCRA-based rate limiter implementation with a dedicated Redis Lua command and integrates it with the alert creation workflow. Both deployment and task run alert services now delegate alert handling to a centralized DeliverAlertService. Additionally, a singleton alertsRateLimiter is established to initialize the Redis client, and thorough unit tests have been added to validate the new rate limiter functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Alert Service
participant DAS as DeliverAlertService
participant ARL as alertsRateLimiter
participant GCRA as GCRARateLimiter
participant Redis as Redis
participant DB as Database
participant Queue as AlertQueue
Client->>DAS: createAndSendAlert(params)
DAS->>ARL: Check rate limit (if taskRunId provided)
ARL->>GCRA: check(identifier)
GCRA->>Redis: Execute custom Lua script
Redis-->>GCRA: Return rate limit result
GCRA-->>ARL: Pass result (allowed/retryAfter)
ARL-->>DAS: Return rate check outcome
DAS->>DB: Create alert record (if allowed)
DAS->>Queue: Enqueue alert for delivery
sequenceDiagram
participant Caller as Service
participant GCRA as GCRARateLimiter
participant Redis as Redis
Caller->>GCRA: Invoke check(identifier)
GCRA->>Redis: Run 'gcra' command with Lua script
Redis-->>GCRA: Respond with rate limit status
GCRA-->>Caller: Return RateLimitResult
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/webapp/app/env.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/v3/GCRARateLimiter.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/v3/services/alerts/deliverAlert.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/webapp/app/v3/GCRARateLimiter.server.ts (2)
58-62: Enhance concurrency safety by documenting Redis cluster usage.
If there's a possibility of using Redis cluster mode with ephemeral cluster node changes or failovers, consider documenting the expected behavior and ensuring that all nodes have the Lua script replicated.
145-169: Remove or refine the try/catch block.
The catch clause rethrows the same error, making it effectively useless. You could handle the error more purposefully (e.g., log additional context) or remove the try/catch for clarity.Possible fix:
- try { - const result: [number, number] = await this.redis.gcra( - key, - now, - this.emissionInterval, - this.burstTolerance, - this.keyExpiration - ); - const allowed = result[0] === 1; - if (allowed) { - return { allowed: true }; - } else { - return { allowed: false, retryAfter: result[1] }; - } - } catch (error) { - throw error; - } + const result: [number, number] = await this.redis.gcra( + key, + now, + this.emissionInterval, + this.burstTolerance, + this.keyExpiration + ); + const allowed = result[0] === 1; + if (allowed) { + return { allowed: true }; + } else { + return { allowed: false, retryAfter: result[1] }; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 168-169: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts (1)
49-58: Validate and handle errors from DeliverAlertService.
Consider wrapping this call in a try/catch or verifying the returned result is successful. If alert creation fails, you may want to retry or log the issue for incident analysis.apps/webapp/app/env.server.ts (1)
311-343: Add documentation for rate limiting parameters.Consider adding JSDoc comments to explain:
- The meaning and impact of
ALERT_RATE_LIMITER_EMISSION_INTERVAL- The meaning and impact of
ALERT_RATE_LIMITER_BURST_TOLERANCE- Example values and their effects on rate limiting behavior
Similar to how API rate limiting parameters are documented (see lines 193-203).
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (1)
1125-1149: Consider enhancing rate limiter error handling.The current implementation swallows rate limiter errors and continues with alert creation. Consider:
- Adding metrics/monitoring for rate limiter failures
- Defining a fallback strategy when rate limiting fails
- Adding context about the rate limiter configuration to the error logs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/v3/GCRARateLimiter.server.ts(1 hunks)apps/webapp/app/v3/alertsRateLimiter.server.ts(1 hunks)apps/webapp/app/v3/services/alerts/deliverAlert.server.ts(3 hunks)apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts(1 hunks)apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts(1 hunks)apps/webapp/test/GCRARateLimiter.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/v3/GCRARateLimiter.server.ts
[error] 168-169: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/v3/GCRARateLimiter.server.ts (2)
70-71: Validate default key expiration calculation.
You calculate the default expiration by taking the max between 60 seconds and (emissionInterval + burstTolerance). This logic seems correct, but ensure it aligns with your business use case. Large key expirations could keep data around longer than needed.
[approve]
73-125: Lua script is well-structured.
The GCRA algorithm implementation looks clear and self-explanatory. The script’s comments comprehensively describe the logic.apps/webapp/app/v3/alertsRateLimiter.server.ts (1)
9-30: Confirm Redis client failover handling.
The code looks good, but if the Redis deployment is frequently changing hosts or running in cluster mode, ensure the internal createRedisClient function handles reconnection correctly and prevents partial initializations.Do you want me to generate a shell script to search your codebase for any references or edge-case handling around Redis failovers?
apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts (1)
49-58: LGTM! Good refactoring.The changes effectively delegate alert creation and delivery to
DeliverAlertService, which centralizes alert handling and enables rate limiting functionality.apps/webapp/test/GCRARateLimiter.test.ts (1)
1-217: Excellent test coverage for the rate limiter!The test suite thoroughly covers essential rate limiting scenarios:
- Basic rate limiting functionality
- Burst handling
- Recovery periods
- Independent rate limiting
- Error cases
- Redis integration
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (1)
1107-1165: LGTM! Well-implemented rate limiting.The implementation:
- Correctly applies rate limiting only to task run alerts
- Uses channel ID as the rate limiting key for proper isolation
- Logs rate limiting decisions for observability
- Creates and enqueues alerts using a transaction
Summary by CodeRabbit