-
Notifications
You must be signed in to change notification settings - Fork 37.5k
Increase stack trace limit to 100 for improving error telemetry #289289
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?
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
@deepak1556Matched files:
|
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.
Pull request overview
This PR increases the V8 stack trace limit from the default 10 frames to 100 frames to improve error telemetry and debugging capabilities in VS Code.
Changes:
- Added
Error.stackTraceLimit = 100to browser and Electron workbench entry points - Included explanatory comments with reference to V8 Stack Trace API documentation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/vs/code/browser/workbench/workbench.ts |
Sets stack trace limit to 100 at the browser workbench entry point before any imports |
src/vs/code/electron-browser/workbench/workbench.ts |
Sets stack trace limit to 100 at the Electron workbench entry point before any imports |
|
You can add it to https://github.com/microsoft/vscode/blob/main/src/vs/code/browser/workbench/workbench.html and https://github.com/microsoft/vscode/blob/main/src/vs/code/electron-browser/workbench/workbench.html instead to truly configure it at entry. The cost of bumping this limit depends on what we do with the exceptions, so if we are fine with 100 in other processes this should be safe bump here as well. |
bpasero
left a comment
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.
This is not a good location to put this in. If you tell me which processes should have this, we can find a better entry file.
It seems unsupported on Firefox, not sure what it means to set it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stackTraceLimit
|
Yes this is a V8 only thing, should scope it to desktop and only chrome based browsers. |
|
I pushed a change to just move it into here:
I think we likely want this for the extension host as well so we would need to add it there too. I am not entirely sure the impact of this change for performance though. Fun fact: I recall we had a stacktrace limit of 100 for a long time but at one point I think I just removed that, cannot recall anymore why I did that, maybe as part of adopting sandbox for the renderer and back then it was a node.js option only. |
Isn't it covered in Lines 14 to 15 in 90bae3c
Also not sure why we do it again in vscode/src/vs/workbench/api/common/extensionHostMain.ts Lines 40 to 41 in 90bae3c
|
We need to capture more frames to better understand the triggering routes of unhandled errors.
Also 100 frames is consistent from with we set in bootstrap-node.ts and extensionHostMain.ts