Optionally disable run debug logs #2116
Conversation
🦋 Changeset detectedLatest commit: e5e901f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change introduces a configuration option to control the sending of run debug logs within the supervisor component. A new environment variable, ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🔭 Outside diff range comments (1)
apps/supervisor/src/workloadServer/index.ts (1)
581-603:⚠️ Potential issueInconsistent debug log handling in notifyRun method.
The
notifyRunmethod callssendDebugLogwithout checking theenv.SEND_RUN_DEBUG_LOGSflag, which means debug logs will still be sent from this method even when the feature is disabled. This undermines the purpose of the feature flag.Apply this pattern to make the debug log calls consistent:
- this.workerClient.sendDebugLog(run.friendlyId, { - time: new Date(), - message: "run:notify socket not found on supervisor", - }); + if (env.SEND_RUN_DEBUG_LOGS) { + this.workerClient.sendDebugLog(run.friendlyId, { + time: new Date(), + message: "run:notify socket not found on supervisor", + }); + }Apply similar conditional checks to the other
sendDebugLogcalls in this method (lines 592-595 and 599-602).
🧹 Nitpick comments (2)
apps/webapp/app/routes/engine.v1.dev.runs.$runFriendlyId.logs.debug.ts (1)
77-79: Consider making this endpoint configurable instead of permanently disabled.Unlike other components in this PR that use the
SEND_RUN_DEBUG_LOGSenvironment variable for configuration, this endpoint is permanently disabled. Consider making it configurable for consistency and flexibility.+import { env } from "~/env.server"; // Add environment import + export function action() { + // Make this configurable like other components + if (!env.SEND_RUN_DEBUG_LOGS) { return new Response(null, { status: 204 }); + } + + // Could restore original functionality here if needed + return new Response(null, { status: 204 }); }packages/core/src/v3/runEngineWorker/supervisor/http.ts (1)
36-36: Good addition of the configuration property.The property is properly encapsulated as private readonly, which is appropriate for configuration that's set at initialization time.
Consider adding a documentation comment to clarify the property's purpose:
+ /** Whether to send run debug logs to the API */ private readonly sendRunDebugLogs: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/fuzzy-snakes-beg.md(1 hunks)apps/supervisor/src/env.ts(1 hunks)apps/supervisor/src/index.ts(1 hunks)apps/supervisor/src/workloadServer/index.ts(2 hunks)apps/webapp/app/routes/engine.v1.dev.runs.$runFriendlyId.logs.debug.ts(1 hunks)apps/webapp/app/routes/healthcheck.tsx(1 hunks)packages/core/src/v3/runEngineWorker/supervisor/http.ts(3 hunks)packages/core/src/v3/runEngineWorker/supervisor/session.ts(1 hunks)packages/core/src/v3/runEngineWorker/supervisor/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/supervisor/src/env.ts (1)
apps/supervisor/src/envUtil.ts (1)
BoolEnv(6-12)
apps/supervisor/src/workloadServer/index.ts (1)
apps/supervisor/src/env.ts (1)
env(93-93)
apps/webapp/app/routes/healthcheck.tsx (1)
apps/webapp/app/db.server.ts (1)
prisma(100-100)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/routes/healthcheck.tsx (1)
6-6: Excellent performance optimization for health check!Replacing
prisma.user.count()withprisma.$queryRawSELECT 1`` is a smart improvement. This change:
- Eliminates the overhead of counting potentially millions of user records
- Provides the same database connectivity verification
- Uses a lightweight constant query that executes quickly
- Maintains the same error handling behavior
This is a standard pattern for health check endpoints and will significantly improve response times.
.changeset/fuzzy-snakes-beg.md (1)
1-6: Well-documented changeset entry.The changeset correctly documents the patch to
@trigger.dev/corepackage and provides a clear, concise description of the new HTTP client option for disabling debug logs.packages/core/src/v3/runEngineWorker/supervisor/session.ts (1)
24-24:✅ Verification successful
Good addition of the debug log control option.
The
sendRunDebugLogs?: booleanproperty is correctly added to the type definition. The optional nature ensures backward compatibility.Let me verify that this new option is properly passed through to the HTTP client:
🏁 Script executed:
#!/bin/bash # Description: Verify that sendRunDebugLogs is properly passed from session options to HTTP client # Expected: The option should be passed to SupervisorHttpClient constructor # Search for SupervisorHttpClient constructor usage in this file rg -A 5 -B 5 "new SupervisorHttpClient" packages/core/src/v3/runEngineWorker/supervisor/session.ts # Search for how the option is used in the HTTP client rg -A 10 "sendRunDebugLogs" packages/core/src/v3/runEngineWorker/supervisor/Length of output: 5041
Confirmed:
sendRunDebugLogsIs Properly Passed Through
- In
session.ts, the entireoptsobject—including the optionalsendRunDebugLogsflag—is forwarded tonew SupervisorHttpClient(opts).- In
http.ts, the constructor readsopts.sendRunDebugLogsand defaults it tofalse(this.sendRunDebugLogs = opts.sendRunDebugLogs ?? false), then conditionally sends debug logs only when set totrue.No further changes needed.
apps/supervisor/src/env.ts (1)
90-90: Well-implemented environment variable addition.The
SEND_RUN_DEBUG_LOGSenvironment variable is properly implemented:
- Uses the existing
BoolEnvschema for consistent boolean handling- Defaults to
false, which is appropriate for a debug feature- Logically placed in the Debug section with other debug-related variables
- Follows the established naming convention
The
BoolEnvschema properly handles string-to-boolean conversion, accepting "true"/"1" as true and other values as false.packages/core/src/v3/runEngineWorker/supervisor/types.ts (1)
9-9: LGTM! Clean type definition for the new feature flag.The optional boolean property is well-named and appropriately typed for controlling debug log transmission.
apps/supervisor/src/index.ts (1)
133-133: LGTM! Correct implementation of environment variable passing.The configuration is properly passed to the SupervisorSession constructor, enabling runtime control of debug log sending.
apps/supervisor/src/workloadServer/index.ts (1)
378-380: LGTM! Correct conditional logic for disabling debug log sending.The environment flag check properly prevents debug log transmission when the feature is disabled.
packages/core/src/v3/runEngineWorker/supervisor/http.ts (2)
45-45: LGTM! Proper initialization with sensible default.The use of nullish coalescing with
falseas the default is appropriate, ensuring debug logs are disabled by default while allowing explicit enablement when needed.
209-211: Excellent implementation of the conditional debug log sending.The early return pattern efficiently avoids network calls when debug logs are disabled, improving performance without affecting the existing error handling logic.
SEND_RUN_DEBUG_LOGS