chore(run-engine): improve concurrency sweeper logging to get better visibility #2557
Conversation
|
WalkthroughThe changes modify internal logging and refactor part of the run-queue logic in internal-packages/run-engine/src/run-queue/index.ts. Several log statements are raised from debug to info and now include additional context fields (e.g., runIdsLength, completedRunIdsLength). A new private method, markRunsForAck, centralizes the logic for marking completed runs for acknowledgment. The existing processCurrentConcurrencyRunIds method now delegates to this helper. Control flow for cases with no completed runs remains, with logs occurring at info level. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal-packages/run-engine/src/run-queue/index.ts (1)
2532-2551: Wire inmarkCompletedRunsForAckand add its typing
markCompletedRunsForAckis defined in internal-packages/run-engine/src/run-queue/index.ts (line 2532) but never invoked; call it where completed runs are acknowledged and provide its TypeScript signature.
🧹 Nitpick comments (8)
internal-packages/run-engine/src/run-queue/index.ts (8)
1865-1867: Add context to abort-path log (or demote to debug).Info-level here can get noisy during shutdowns. At minimum, add service to the log payload for consistency.
- this.logger.info("Abort signal received, skipping concurrency scan"); + this.logger.info("Abort signal received, skipping concurrency scan", { + service: this.name, + });
1870-1871: Include service context in top-level sweep log.Keeps logs consistent and easier to filter.
- this.logger.info("Scanning concurrency sets for completed runs"); + this.logger.info("Scanning concurrency sets for completed runs", { + service: this.name, + });
1923-1926: Cap payload size when logging concurrency keys.Logging the full key list can explode log volume and cost. Log count plus a sample; include service for filtering.
- this.logger.info("Processing concurrency keys from stream", { - keys: uniqueKeys, - }); + const keysSample = uniqueKeys.slice(0, 10); + this.logger.info("Processing concurrency keys from stream", { + keyCount: uniqueKeys.length, + keysSample, + service: this.name, + });
1993-1997: LGTM: sampled runIds + count is good; add service for consistency.Nice touch to cap at 5. Add service for easier log filtering.
this.logger.info("Processing concurrency set with runs", { concurrencyKey, - runIds: runIds.slice(0, 5), // Log first 5 for debugging, + runIds: runIds.slice(0, 5), // Log first 5 for debugging runIdsLength: runIds.length, + service: this.name, });
2003-2010: Avoid duplicate “no completed runs” logs and skip when callback missing.Unify the checks and prevent noisy info logs when no sweeper is configured.
- // Call the callback to determine which runs are completed - const completedRuns = await this.options.concurrencySweeper?.callback(runIds); - - if (!completedRuns) { - this.logger.info("No completed runs found in concurrency set", { concurrencyKey }); - return; - } - - if (completedRuns.length === 0) { - this.logger.info("No completed runs found in concurrency set", { concurrencyKey }); - return; - } + // Call the callback to determine which runs are completed + const cb = this.options.concurrencySweeper?.callback; + if (!cb) { + this.logger.debug("Concurrency sweeper callback not configured; skipping", { + concurrencyKey, + service: this.name, + }); + return; + } + const completedRuns = await cb(runIds); + if (!completedRuns?.length) { + this.logger.info("No completed runs found in concurrency set", { + concurrencyKey, + service: this.name, + }); + return; + }
2012-2016: Include service context on “completed runs” log.Helps correlate across services.
this.logger.info("Found completed runs to mark for ack", { concurrencyKey, completedRunIds: completedRuns.map((r) => r.id).slice(0, 5), completedRunIdsLength: completedRuns.length, + service: this.name, });
2026-2043: Use server time via Lua command and reduce per-item info logs.
- Prefer the existing Lua command (markCompletedRunsForAck) to avoid client clock skew and ensure atomic scoring with Redis TIME.
- Demote per-run logs to debug; keep the aggregate info log.
- // Prepare arguments: alternating orgId, messageId pairs - const args: Array<number | string> = []; - for (const run of completedRuns) { - this.logger.info("Marking run for acknowledgment", { - orgId: run.orgId, - runId: run.id, - }); - - args.push(Date.now()); - args.push(`${run.orgId}:${run.id}`); - } - - const count = await this.redis.zadd(markedForAckKey, ...args); - - this.logger.info("Marked runs for acknowledgment", { - markedForAckKey, - count, - }); + // Prepare args: alternating orgId, messageId pairs + const args: string[] = []; + for (const run of completedRuns) { + this.logger.debug("Marking run for acknowledgment", { + orgId: run.orgId, + runId: run.id, + service: this.name, + }); + args.push(run.orgId, run.id); + } + + const count = await this.redis.markCompletedRunsForAck(markedForAckKey, ...args); + + this.logger.info("Marked runs for acknowledgment", { + markedForAckKey, + count, + service: this.name, + });Add the command to the Redis typings to keep TS happy:
// In the declare module "@internal/redis" section interface RedisCommander<Context> { markCompletedRunsForAck( markedForAckKey: string, ...args: string[] // alternating orgId, messageId pairs ): Result<number, Context>; }
2039-2042: Avoid double-logging count without context.If you keep the per-run logs at info, the aggregate log is redundant. Either demote the per-run logs (recommended above) or add context here (service already added in the diff).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/run-engine/src/run-queue/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
internal-packages/run-engine/src/run-queue/index.ts
🧬 Code graph analysis (1)
internal-packages/run-engine/src/run-queue/index.ts (1)
internal-packages/run-engine/src/engine/index.ts (1)
runIds(1438-1476)
⏰ 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). (22)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
No description provided.