-
-
Notifications
You must be signed in to change notification settings - Fork 794
Fix excessive framework noise in error stack traces #2442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix excessive framework noise in error stack traces #2442
Conversation
Add AsyncLocalStorage to LINES_TO_IGNORE (removes OpenTelemetry noise) Apply error stack trace filtering for createJsonErrorObject Add test for framework stack trace filtering Add changeset This change improves readability of error traces in user-facing contexts (Slack alerts, email notifications, API responses, run streams) by filtering out internal framework noise that clutters error messages.
🦋 Changeset detectedLatest commit: cf9be9f The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
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 |
WalkthroughUpdates error stack-trace processing in packages/core/src/v3/errors.ts: BUILT_IN_ERROR now uses correctErrorStackTrace with removeFirstLine to normalize and omit the first line, and AsyncLocalStorage frames are added to the ignore list. Adds a unit test in packages/core/test/errors.test.ts validating that user frames are preserved and internal/framework frames (including AsyncLocalStorage and internal modules) are filtered in createJsonErrorObject output. Introduces a Changeset entry to bump @trigger.dev/core with a patch noting improved readability of user-facing error stack traces. No public API changes. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/v3/errors.ts (1)
378-379
: Broaden (or tighten) AsyncLocalStorage filtering to reduce false positives and cover more runtimes/AsyncLocalStorage/ may hide legitimate user frames and might miss other node internals. Consider adding targeted patterns for node built-ins and OTEL context manager while keeping current behavior.
Apply:
const LINES_TO_IGNORE = [ /ConsoleInterceptor/, /TriggerTracer/, /TaskExecutor/, /EXECUTE_TASK_RUN/, /@trigger.dev\/core/, /packages\/core\/src\/v3/, /safeJsonProcess/, /__entryPoint.ts/, /ZodIpc/, /startActiveSpan/, /processTicksAndRejections/, - /AsyncLocalStorage/, + /AsyncLocalStorage/, // keep for backward compatibility + /AsyncLocalStorageContextManager/, // otel context mgr + /node:async_hooks/, // node built-in scheme + /node:internal\//, // other node internals + /@opentelemetry\//, // otel frames ];packages/core/test/errors.test.ts (1)
25-47
: Also assert header removal and preserved name/messageSince removeFirstLine is now applied, assert the stack no longer contains the header, and that name/message are still surfaced.
Apply:
const jsonError = createJsonErrorObject(taskRunError); + // Header should be removed from stackTrace but preserved in fields + expect(jsonError.stackTrace).not.toContain("Error: Network error occurred"); + expect(jsonError.name).toBe("Error"); + expect(jsonError.message).toBe("Network error occurred");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/dry-taxis-wash.md
(1 hunks)packages/core/src/v3/errors.ts
(2 hunks)packages/core/test/errors.test.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
packages/core/test/errors.test.ts
packages/core/src/v3/errors.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
packages/core/test/errors.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/test/errors.test.ts
packages/core/src/v3/errors.ts
🧬 Code graph analysis (1)
packages/core/test/errors.test.ts (1)
packages/core/src/v3/errors.ts (1)
createJsonErrorObject
(222-251)
🔇 Additional comments (3)
.changeset/dry-taxis-wash.md (1)
1-6
: Patch bump and message look goodClear, scoped changeset; patch level is appropriate for behavior-only change.
packages/core/src/v3/errors.ts (1)
230-233
: Apply stack filtering for BUILT_IN_ERROR — good fixRouting BUILT_IN_ERROR through correctErrorStackTrace with removeFirstLine improves user-facing stacks and aligns with other paths.
packages/core/test/errors.test.ts (1)
1-48
: Strong, targeted test — preserves user frames and filters framework noiseCovers the key regression. Nice.
stackTrace: correctErrorStackTrace(enhancedError.stackTrace, undefined, { | ||
removeFirstLine: true, | ||
}), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify no consumers rely on the “Error: …” header being present in stackTrace
Since removeFirstLine drops the header line from stackTrace, double-check downstream formatters (Slack/email/API clients) don’t expect it there.
Run:
🏁 Script executed:
#!/bin/bash
# Find usages that might format jsonError.stackTrace directly
rg -nP --type=ts --type=tsx -C2 '\bcreateJsonErrorObject\s*\('
rg -nP --type=ts --type=tsx -C2 '\bstackTrace\b' apps packages
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Rerun search for createJsonErrorObject across TS and TSX files in apps and packages
rg -n 'createJsonErrorObject\(' -C2 -g '*.{ts,tsx}' apps packages
# Rerun search for direct stackTrace property access in apps and packages
rg -n '\.stackTrace\b' -C2 -g '*.{ts,tsx}' apps packages
Length of output: 12404
Adjust downstream consumers for new stackTrace format
We’ve confirmed that several formatting routines rely on the first line of stackTrace
being the "Error: …"
header. Since we now strip that line by default, these consumers need to be updated to use the separate name
/message
fields rather than parsing stackTrace
directly:
• packages/cli-v3/src/mcp/formatters.ts (around line 51)
Current:
if (run.error.stackTrace) {
lines.push(`Stack: ${run.error.stackTrace.split("\n")[0]}`);
}
Suggested:
- lines.push(`Stack: ${run.error.stackTrace.split("\n")[0]}`);
+ // Use the explicit name/message fields now that header’s removed
+ lines.push(`Stack: ${run.error.name || "Error"}: ${run.error.message}`);
• packages/cli-v3/src/dev/devOutput.ts (around line 260)
Current:
return `\n\n${error.stackTrace.replace(/^Error: /, chalkError("X Error: "))}\n`;
Suggested:
- ${error.stackTrace.replace(/^Error: /, chalkError("X Error: "))}
+ ${chalkError("X Error: ")}${error.message}\n${error.stackTrace}
• apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (line 705)
Current wraps error.stackTrace ?? error.message
in a code block. To preserve context, consider rendering Error: ${error.name}: ${error.message}
separately and then the trace.
• apps/webapp/app/routes/…/route.tsx (around line 911)
UI renders enhancedError.stackTrace
without the header—verify that the header is shown elsewhere (e.g. via error.name
+ error.message
).
Please update these formatters/UI components to reference the dedicated error fields instead of assuming the header lives in stackTrace
.
Problem
Users reported excessive framework noise in Slack error alerts, making debugging difficult. Issue #2097 shows stack traces cluttered with trigger.dev internals like:
_RunTimelineMetricsAPI.measureMetric
ConsoleInterceptor.intercept
taskExecutor.ts
internal callsSolution
Applied existing correctErrorStackTrace filtering to
BUILT_IN_ERROR
case in createJsonErrorObject - it was the only error type returning raw stack traces without filtering.Changes:
enhancedError.stackTrace
with correctErrorStackTrace() for filtering stack tracesAsyncLocalStorage
pattern toLINES_TO_IGNORE
to remove OpenTelemetry noiseImpact
Cleaner error experiences across all user-facing error contexts using createJsonErrorObject:
Closes #2097
✅ Checklist
Testing
Changelog
Improve user-facing error readability by filtering framework noise from stack traces
Screenshots
N/A