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(core): Decode filename and module stack frame properties in Node stack parser #14544

Merged
merged 8 commits into from
Dec 5, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Dec 2, 2024

While working on #14525, I noticed that for some reason, I wasn't getting any context lines for the source JS file but instead a bunch of debug messages about the SDK not being able to read the file. The file path happened to contain spaces which were URL-encoded (%20). It turns out that in ESM, the Node stack trace filenames are URL-encoded. I checked and this and it only seems to be the case in ESM, not in CJS.

Specifically, this PR:

  • decodes the filename stack frame property
  • decodes the module property (which is derived from the raw filename)
  • adds a bunch of unit tests for the module name extraction logic (general tests as well as for encoded file names)
  • adds unit and integration tests (ESM and CJS) for file names with spaces

@Lms24 Lms24 self-assigned this Dec 2, 2024
@Lms24 Lms24 force-pushed the lms/fix-core-node-stack-parser-decode-filename branch from a4ee349 to 73eda05 Compare December 3, 2024 15:17
@AbhiPrasad AbhiPrasad requested a review from timfish December 3, 2024 15:29
Copy link
Collaborator

@timfish timfish left a comment

Choose a reason for hiding this comment

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

Nice find!

@Lms24 Lms24 changed the title fix(core): Decode file name in Node stack parser fix(core): Decode filename and module stack frame properties in Node stack parser Dec 3, 2024
@Lms24 Lms24 marked this pull request as ready for review December 3, 2024 16:01
@Lms24 Lms24 requested review from lforst and AbhiPrasad December 3, 2024 16:01
Copy link
Member Author

Choose a reason for hiding this comment

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

the module name extraction logic was previously untested. Thought I'd add some general tests as well.

Comment on lines +49 to +50
const moduleName = dir.slice(normalizedBase.length + 1).replace(/\//g, '.');
return moduleName ? `${moduleName}:${decodedFile}` : decodedFile;
Copy link
Member Author

@Lms24 Lms24 Dec 3, 2024

Choose a reason for hiding this comment

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

This change also includes a tiny refactor to use a template string instead of concatenation. I made the change after writing the tests btw, so this should be fine

@Lms24 Lms24 force-pushed the lms/fix-core-node-stack-parser-decode-filename branch from 37d1091 to 681b591 Compare December 4, 2024 16:06
Copy link
Contributor

github-actions bot commented Dec 4, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.17 KB added added
@sentry/browser - with treeshaking flags 21.85 KB added added
@sentry/browser (incl. Tracing) 35.66 KB added added
@sentry/browser (incl. Tracing, Replay) 72.57 KB added added
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.99 KB added added
@sentry/browser (incl. Tracing, Replay with Canvas) 76.88 KB added added
@sentry/browser (incl. Tracing, Replay, Feedback) 89.36 KB added added
@sentry/browser (incl. Feedback) 39.93 KB added added
@sentry/browser (incl. sendFeedback) 27.78 KB added added
@sentry/browser (incl. FeedbackAsync) 32.57 KB added added
@sentry/react 25.84 KB added added
@sentry/react (incl. Tracing) 38.47 KB added added
@sentry/vue 27.37 KB added added
@sentry/vue (incl. Tracing) 37.49 KB added added
@sentry/svelte 23.33 KB added added
CDN Bundle 24.33 KB added added
CDN Bundle (incl. Tracing) 37.32 KB added added
CDN Bundle (incl. Tracing, Replay) 72.19 KB added added
CDN Bundle (incl. Tracing, Replay, Feedback) 77.53 KB added added
CDN Bundle - uncompressed 71.5 KB added added
CDN Bundle (incl. Tracing) - uncompressed 110.78 KB added added
CDN Bundle (incl. Tracing, Replay) - uncompressed 223.85 KB added added
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 237.07 KB added added
@sentry/nextjs (client) 38.77 KB added added
@sentry/sveltekit (client) 36.15 KB added added
@sentry/node 134.86 KB added added
@sentry/node - without tracing 96.86 KB added added
@sentry/aws-serverless 109.18 KB added added

@Lms24 Lms24 merged commit d8bd0f5 into develop Dec 5, 2024
153 checks passed
@Lms24 Lms24 deleted the lms/fix-core-node-stack-parser-decode-filename branch December 5, 2024 08:26
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.

3 participants