chore(redis-worker): add otel spans to fair queue processing pipeline #2815
Conversation
|
WalkthroughThis pull request implements batched OpenTelemetry span management for the fair queue consumer loop. It exports Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (4)
packages/redis-worker/src/fair-queue/index.ts (2)
772-776: UnusedparentSpanparameter.The
parentSpanparameter is declared but never referenced in the method body. Consider removing it if not needed, or use it for span linking/attributes.🔎 Suggested fix
- async #processMasterQueueShard( - loopId: string, - shardId: number, - parentSpan?: Span - ): Promise<void> { + async #processMasterQueueShard(loopId: string, shardId: number): Promise<void> {And update the call site at line 744:
- await this.#processMasterQueueShard(loopId, shardId, span); + await this.#processMasterQueueShard(loopId, shardId);
1087-1087: UnusedparentSpanparameter (same as Line 775).Same issue as
#processMasterQueueShard- theparentSpanparameter is declared but unused.packages/redis-worker/src/fair-queue/telemetry.ts (2)
438-471: Consider usingtypeinstead ofinterfaceper coding guidelines.The coding guidelines specify "Use types over interfaces for TypeScript." While this is a minor style preference, you might consider converting these to type aliases for consistency.
🔎 Suggested change
-export interface ConsumerLoopState { +export type ConsumerLoopState = { /** Countdown of iterations before starting a new span */ perTraceCountdown: number; // ... rest of fields -} +}; -export interface BatchedSpanManagerOptions { +export type BatchedSpanManagerOptions = { /** The tracer to use for creating spans */ tracer?: Tracer; // ... rest of fields -} +};Based on coding guidelines, prefer types over interfaces.
588-605: UseSpanKind.CONSUMERconstant instead of magic number.Line 592 uses the magic number
1forSpanKind.CONSUMER. SinceSpanKindis available from the tracing imports, use the named constant for clarity and maintainability.🔎 Suggested fix
First, add the import:
-import { context, trace, SpanStatusCode, ROOT_CONTEXT } from "@internal/tracing"; +import { context, trace, SpanStatusCode, ROOT_CONTEXT, SpanKind } from "@internal/tracing";Then update line 592:
{ - kind: 1, // SpanKind.CONSUMER + kind: SpanKind.CONSUMER, attributes: {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal-packages/tracing/src/index.tspackages/redis-worker/src/fair-queue/index.tspackages/redis-worker/src/fair-queue/telemetry.tspackages/redis-worker/src/fair-queue/types.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/redis-worker/src/fair-queue/types.tspackages/redis-worker/src/fair-queue/telemetry.tspackages/redis-worker/src/fair-queue/index.tsinternal-packages/tracing/src/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/redis-worker/src/fair-queue/types.tspackages/redis-worker/src/fair-queue/telemetry.tspackages/redis-worker/src/fair-queue/index.tsinternal-packages/tracing/src/index.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/redis-worker/src/fair-queue/types.tspackages/redis-worker/src/fair-queue/telemetry.tspackages/redis-worker/src/fair-queue/index.tsinternal-packages/tracing/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Control concurrency using the `queue` property with `concurrencyLimit` option
Applied to files:
packages/redis-worker/src/fair-queue/types.ts
🧬 Code graph analysis (2)
packages/redis-worker/src/fair-queue/telemetry.ts (1)
internal-packages/tracing/src/index.ts (8)
Span(42-42)Context(49-49)Tracer(14-14)Attributes(15-15)ROOT_CONTEXT(48-48)trace(39-39)context(40-40)SpanStatusCode(45-45)
packages/redis-worker/src/fair-queue/index.ts (2)
packages/redis-worker/src/fair-queue/telemetry.ts (3)
BatchedSpanManager(479-709)FairQueueTelemetry(81-429)FairQueueAttributes(18-29)packages/redis-worker/src/fair-queue/schedulers/weighted.ts (1)
queues(237-293)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Cursor Bugbot
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
internal-packages/tracing/src/index.ts (1)
28-50: LGTM! Clean exports for batched span management.The addition of
ROOT_CONTEXTandContexttype exports properly supports the newBatchedSpanManagerthat usesROOT_CONTEXTto create independent parent spans for consumer loop batches. Usingtype Contextfor the type-only export aligns with TypeScript best practices.packages/redis-worker/src/fair-queue/types.ts (1)
367-372: LGTM! Well-documented configuration options.The new tracing configuration options follow the established pattern in
FairQueueOptions. The defaults (500 iterations, 60 seconds) provide reasonable batching that balances span granularity with volume reduction.packages/redis-worker/src/fair-queue/index.ts (9)
2-15: LGTM! Correct imports for tracing integration.The imports are properly organized, bringing in
BatchedSpanManagerfrom the telemetry module and the requiredSpantype. The explicit imports from./telemetry.jskeep the dependency explicit.
97-101: LGTM! Member variables for tracing configuration.Clean addition of configuration state for consumer tracing.
160-166: LGTM! BatchedSpanManager initialization.The manager is correctly instantiated after
FairQueueTelemetrywith the same tracer and name for consistent span naming.
676-690: LGTM! Proper cleanup of batched spans on close.The
cleanupAll()call ensures any remaining active spans are ended before closing other resources. The placement afterstop()is correct since individual loops also callcleanup()in their finally blocks.
729-770: LGTM! Well-structured batched span lifecycle for master queue consumer.The implementation follows a clean pattern: initialize on start, wrap iterations with
withBatchedSpan, mark for rotation on errors, and cleanup on abort or completion. Thefinallyblock ensures cleanup even if an unexpected error propagates.
799-811: LGTM! Good stat tracking for observability.The incremented stats (
empty_iterations,tenants_selected,queues_selected) will be recorded as span attributes when the batched span rotates, providing useful aggregated metrics.
931-1008: LGTM! Consistent batched span pattern for worker queue consumer.The worker queue loop follows the same reliable pattern as the master queue loop. The tracking of
blocking_pop_timeoutsandinvalid_message_keysprovides valuable debugging information for operational issues.
1040-1085: LGTM! Consistent pattern for direct consumer loop.The direct consumer loop correctly implements the batched span lifecycle.
1110-1122: LGTM! Consistent stat tracking in direct processing.The same stats (
empty_iterations,tenants_selected,queues_selected,cooloff_skipped,messages_processed,process_failures) are tracked for the direct processing path, ensuring consistent observability regardless of worker queue mode.packages/redis-worker/src/fair-queue/telemetry.ts (6)
1-13: LGTM! Correct imports for batched span management.The imports properly bring in
Contextas a type and the runtime utilities (context,trace,SpanStatusCode,ROOT_CONTEXT) needed for span context manipulation.
479-506: LGTM! Clean initialization and state management.The
BatchedSpanManagerconstructor andinitializeLoopmethod properly initialize state with sensible defaults. Using aMapforloopStatessupports concurrent consumer loops effectively.
538-548: LGTM! Comprehensive rotation check.The
shouldRotatemethod correctly checks all conditions that should trigger span rotation: iteration limit, time limit, missing context, and explicit rotation flag.
623-691: LGTM! RobustwithBatchedSpanimplementation.The method correctly:
- Auto-initializes state if missing (defensive)
- Checks and performs rotation as needed
- Falls back to
noopSpanwhen tracing is disabled- Creates child spans within the parent context using
context.with()- Records exceptions to both iteration and parent spans on error
- Properly updates iteration counters in the
finallyblock regardless of success/failure
669-676: Good error propagation to parent span.Recording the exception on both
iterationSpanandstate.currentSpanensures the error is visible at both the individual iteration level and the batched parent span level. SettingendSpanInNextIteration = trueensures the problematic span batch ends cleanly.
696-708: LGTM! Clean cleanup implementation.The
cleanupmethod properly ends any active span before removing state, andcleanupAlliterates through all loops safely.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
No description provided.