Skip to content
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

fix(node): Include debug_meta with ANR events #14203

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Nov 7, 2024

Sends a map of filename -> debug ids to the ANR worker thread and sends an updated list every time more modules are loaded. These are then used and included with events so they can be symbolicated

Copy link

codecov bot commented Nov 7, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
241 1 240 8
View the full list of 1 ❄️ flaky tests
sse.test.ts Waits for sse streaming when sse has been explicitly aborted

Flake rate in main: 48.57% (Passed 54 times, Failed 51 times)

Stack Traces | 1.27s run time
sse.test.ts:37:5 Waits for sse streaming when sse has been explicitly aborted

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

@timfish timfish marked this pull request as ready for review November 11, 2024 10:27
let debugImages: Record<string, string> = getFilenameToDebugIdMap(initOptions.stackParser);

onModuleLoad(() => {
debugImages = getFilenameToDebugIdMap(initOptions.stackParser);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this potentially called every require/import? That means we re-calculate this every time. That feels like a lot of work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could debounce the require/import but any delay sending the debug ids to the worker could result in missing debug ids.

If global._sentryDebugIds is undefined, getFilenameToDebugIdMap returns immediately so this only impacts those with injected debug id polyfills. getFilenameToDebugIdMap already caches a lot. If there have been no additions to global._sentryDebugIds, no stack parsing occurs. There is still scope for it to cache more though and simplify the work done on each call. That would probably be a good start 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright let's proceed for now, we should re-visit this though.

// eslint-disable-next-line deprecation/deprecation
diagnosticsChannel.channel('module.require.end').subscribe(() => callback());
// eslint-disable-next-line deprecation/deprecation
diagnosticsChannel.channel('module.import.asyncEnd').subscribe(() => callback());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these deprecated?

Copy link
Collaborator Author

@timfish timfish Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a new API in diagnostics_channel to subscribe to a channel but we need to support older Node versions where this doesn't exist so we rely on the deprecated API.

@AbhiPrasad AbhiPrasad merged commit fac7209 into develop Nov 11, 2024
133 checks passed
@AbhiPrasad AbhiPrasad deleted the timfish/fix/node-anr-debug-meta branch November 11, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants