Support for logically separated redis instances #1647
Conversation
…qs, and the pub/sub stuff (they all default to the previously used env vars for redis)
|
|
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/services/authorizationRateLimitMiddleware.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/services/apiRateLimit.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.
WalkthroughThis pull request introduces comprehensive changes to the application's Redis configuration management. The modifications involve adding new environment variables in the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 2
🧹 Nitpick comments (5)
apps/webapp/app/services/realtimeClientGlobal.server.ts (1)
11-16: Consider using dedicated Redis instance for realtime functionalityThe realtime client is using the rate limiting Redis instance (
RATE_LIMIT_REDIS_*), which seems architecturally incorrect. Realtime functionality and rate limiting serve different purposes and may have different scaling requirements. Consider:
- Using a dedicated
REALTIME_REDIS_*configuration- Or using the existing pub/sub Redis instance since realtime features often rely on pub/sub mechanisms
This separation would:
- Prevent potential performance impacts between rate limiting and realtime features
- Allow independent scaling of Redis instances based on their specific usage patterns
- Maintain clearer architectural boundaries
references/v3-catalog/src/trigger/queues.ts (1)
44-59: Document the purpose of API traffic inspectionThe task's purpose and testing methodology should be documented. Consider:
- Adding JSDoc comments explaining the task's purpose
- Documenting expected outcomes
- Adding parameters to configure the number of calls and timing
+/** + * Task to inspect API traffic patterns and response headers. + * @param {Object} options + * @param {number} [options.numberOfCalls=100] - Number of API calls to make + * @param {number} [options.delayBetweenCalls=0] - Delay in seconds between calls + */ export const inspectApiTraffic = task({ id: "queues/inspect-api-traffic", - run: async (payload: unknown, { ctx }) => { + run: async (payload: { numberOfCalls?: number; delayBetweenCalls?: number } = {}, { ctx }) => { + const { numberOfCalls = 100, delayBetweenCalls = 0 } = payload;apps/webapp/app/env.server.ts (1)
94-179: LGTM! Well-structured Redis configuration with proper fallbacks.The changes introduce three logically separated Redis configurations (rate limiting, caching, pub/sub) while maintaining backward compatibility through fallbacks to generic Redis configuration.
Consider adding JSDoc comments to document:
- The purpose of each Redis configuration set
- The fallback behavior to generic Redis configuration
- Example configuration values
apps/webapp/app/v3/eventRepository.server.ts (2)
1102-1108: LGTM! Efficient handling of multiple publish operations.The code efficiently handles multiple events by deduplicating trace IDs and using Promise.allSettled for resilient publishing.
Consider caching the timestamp to avoid multiple Date object creations:
+ const timestamp = new Date().toISOString(); await Promise.allSettled( Array.from(uniqueTraces).map((traceId) => - this._redisPublishClient.publish(traceId, new Date().toISOString()) + this._redisPublishClient.publish(traceId, timestamp) ) );
1146-1151: LGTM! Good separation of Redis configurations.The use of PUBSUB-specific Redis configuration with secure defaults (TLS enabled by default) and performance optimization (auto-pipelining) is well implemented.
Consider adding connection timeout options to prevent hanging connections:
{ port: env.PUBSUB_REDIS_PORT, host: env.PUBSUB_REDIS_HOST, username: env.PUBSUB_REDIS_USERNAME, password: env.PUBSUB_REDIS_PASSWORD, enableAutoPipelining: true, + connectTimeout: 5000, + maxRetriesPerRequest: 3, ...(env.PUBSUB_REDIS_TLS_DISABLED === "true" ? {} : { tls: {} }), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/services/apiRateLimit.server.ts(1 hunks)apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts(1 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/services/rateLimiter.server.ts(1 hunks)apps/webapp/app/services/realtimeClientGlobal.server.ts(1 hunks)apps/webapp/app/v3/eventRepository.server.ts(3 hunks)apps/webapp/app/v3/marqs/devPubSub.server.ts(1 hunks)apps/webapp/app/v3/marqs/fairDequeuingStrategy.server.ts(0 hunks)apps/webapp/app/v3/marqs/index.server.ts(0 hunks)apps/webapp/app/v3/marqs/v2.server.ts(0 hunks)apps/webapp/app/v3/services/projectPubSub.server.ts(1 hunks)apps/webapp/test/fairDequeuingStrategy.test.ts(0 hunks)references/v3-catalog/src/trigger/queues.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/webapp/app/v3/marqs/v2.server.ts
- apps/webapp/test/fairDequeuingStrategy.test.ts
- apps/webapp/app/v3/marqs/fairDequeuingStrategy.server.ts
- apps/webapp/app/v3/marqs/index.server.ts
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/services/platform.v3.server.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/webapp/app/v3/marqs/devPubSub.server.ts (1)
24-30: LGTM! Proper use of dedicated pub/sub Redis instanceThe changes correctly utilize the dedicated pub/sub Redis instance for development pub/sub functionality. This aligns well with the architectural goal of logically separating Redis instances by their purpose.
apps/webapp/app/v3/services/projectPubSub.server.ts (1)
25-30: LGTM! Consistent use of pub/sub Redis configurationThe changes maintain consistency with other pub/sub related modifications by properly utilizing the dedicated pub/sub Redis instance.
apps/webapp/app/services/apiRateLimit.server.ts (1)
8-13: LGTM! Redis configuration updated for rate limiting.The changes correctly update the Redis configuration to use rate limit specific environment variables, providing better isolation for rate limiting functionality.
apps/webapp/app/services/rateLimiter.server.ts (1)
31-36: LGTM! Redis configuration updated consistently.The changes correctly update the Redis configuration in the RateLimiter class to use rate limit specific environment variables, maintaining consistency with other rate limiting components.
apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts (1)
166-171: LGTM! Redis configuration updated consistently.The changes correctly update the Redis configuration in the authorization rate limit middleware to use rate limit specific environment variables, maintaining consistency with other rate limiting components.
apps/webapp/app/v3/eventRepository.server.ts (2)
994-1006: LGTM! Performance improvement in Redis subscription mechanism.The change from pattern-based subscription (
psubscribe) to direct channel subscription (subscribe) reduces the overhead of pattern matching and simplifies the event handling logic.
1011-1013: LGTM! Proper cleanup in unsubscribe logic.The unsubscription logic correctly handles channel unsubscription, connection cleanup, and subscriber count management.
| // Retrieve the run 100 times | ||
| for (let i = 0; i < 100; i++) { | ||
| await runs.retrieve(ctx.run.id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the API traffic inspection approach
Making 100 identical API calls to retrieve the same run is inefficient and could:
- Unnecessarily consume API rate limits
- Impact system performance
- Create unnecessary load on the server
Consider:
- Reducing the number of calls or making them configurable
- Adding delays between calls using
wait.for() - Using a more efficient sampling approach
@trigger.dev/build
@trigger.dev/react-hooks
trigger.dev
@trigger.dev/rsc
@trigger.dev/sdk
@trigger.dev/core
commit: |
Summary by CodeRabbit
Configuration
Pub/Sub and Event Handling
Queueing Strategy
New Features
inspectApiTrafficfor retrieving API traffic information