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

feat(nextjs): Do not strip origin information from different origin stack frames #15418

Merged
merged 15 commits into from
Feb 24, 2025

Conversation

lforst
Copy link
Member

@lforst lforst commented Feb 14, 2025

EDIT(cg): Moved this functionality behind an _experimental.thirdPartyOriginStackFrames config flag

This change should drastically improve transparency on stack frames that come from third party origins while keeping existing sourcemapping functionality.

Example: https://plausible.io/js/script.js was always turned into app:///js/script.js, leading the user to believe "wtf is this".

By not stripping scripts on different origins from their origin in the stack trace, it should be way more transparent where errors originate from, potentially even improving sourcemaps via scraping if these scripts have propper source mapping urls/debug IDs.

Additionally, we should no longer strip js-in-html stack frames, also making those more transparent.

Considerations:

  • May mess with grouping: I will go ahead and say the upsides outweigh the downsides here.
  • Backwards compat: This should be fully backwards compatible with exisiting upload setups.

@lforst lforst marked this pull request as ready for review February 14, 2025 14:54
@lforst lforst requested a review from chargome February 14, 2025 14:55
@chargome chargome force-pushed the lforst-improve-diff-origin-stack-frames branch from 72c5969 to c38281a Compare February 19, 2025 13:02
Copy link
Contributor

github-actions bot commented Feb 19, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.05 KB - -
@sentry/browser - with treeshaking flags 22.84 KB - -
@sentry/browser (incl. Tracing) 36.07 KB - -
@sentry/browser (incl. Tracing, Replay) 73.08 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 66.55 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 77.34 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 90.28 KB - -
@sentry/browser (incl. Feedback) 40.2 KB - -
@sentry/browser (incl. sendFeedback) 27.68 KB - -
@sentry/browser (incl. FeedbackAsync) 32.48 KB - -
@sentry/react 24.87 KB - -
@sentry/react (incl. Tracing) 37.95 KB - -
@sentry/vue 27.23 KB - -
@sentry/vue (incl. Tracing) 37.76 KB - -
@sentry/svelte 23.09 KB - -
CDN Bundle 24.25 KB - -
CDN Bundle (incl. Tracing) 36.1 KB - -
CDN Bundle (incl. Tracing, Replay) 70.94 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.11 KB - -
CDN Bundle - uncompressed 70.91 KB - -
CDN Bundle (incl. Tracing) - uncompressed 107.17 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 218.45 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 231.02 KB - -
@sentry/nextjs (client) 39.11 KB +0.44% +174 B 🔺
@sentry/sveltekit (client) 36.48 KB - -
@sentry/node 129.25 KB - -
@sentry/node - without tracing 98.03 KB - -
@sentry/aws-serverless 107.45 KB -0.01% -1 B 🔽

View base workflow run

Copy link

codecov bot commented Feb 21, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4647 1 4646 324
View the top 1 failed test(s) by shortest run time
should report ANR when event loop blocked worker can be stopped and restarted
Stack Traces | 15s run time
Error: thrown: "Exceeded timeout of 15000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
    at .../suites/anr/test.ts:204:3
    at _dispatchDescribe (.../jest-circus/build/index.js:98:26)
    at describe (.../jest-circus/build/index.js:60:5)
    at Object.<anonymous> (.../suites/anr/test.ts:109:1)
    at Runtime._execModule (.../jest-runtime/build/index.js:1646:24)
    at Runtime._loadModule (.../jest-runtime/build/index.js:1185:12)
    at Runtime.requireModule (.../jest-runtime/build/index.js:1009:12)
    at jestAdapter (.../build/legacy-code-todo-rewrite/jestAdapter.js:79:13)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at runTestInternal (.../jest-runner/build/runTest.js:389:16)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

I think it would be good to use this option in an E2E test to test it.

Comment on lines +48 to +49
const { origin } = new URL(frame.filename as string);
frame.filename = frame.filename?.replace(origin, 'app://').replace(rewriteFramesAssetPrefixPath, '');
Copy link
Member

Choose a reason for hiding this comment

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

L: That's a minor code-style comment - I would move this logic (which is shorter) upward to improve readability.

@lforst lforst merged commit bac7387 into develop Feb 24, 2025
148 checks passed
@lforst lforst deleted the lforst-improve-diff-origin-stack-frames branch February 24, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants