Skip to content

fix(v8/utils): Stack parser skip frames (not lines of stack string)#10560

Merged
krystofwoldrich merged 6 commits intodevelopfrom
@krystofwoldrich/fix-frames-to-pop
Mar 6, 2024
Merged

fix(v8/utils): Stack parser skip frames (not lines of stack string)#10560
krystofwoldrich merged 6 commits intodevelopfrom
@krystofwoldrich/fix-frames-to-pop

Conversation

@krystofwoldrich
Copy link
Copy Markdown
Contributor

@krystofwoldrich krystofwoldrich commented Feb 7, 2024

In happy scenarios, the framesToPop should not make a difference as the Sentry backend will mark nonInApp frames and hide them by default from the user.

In other scenarios, when source maps are missing for example, the frames will be visible and shown as the root of an error which is misleading and confusing to our users.

Before this PR

We used framesToPop for two unrelated purposes.

  • To skip lines in the stack string that could potentially be parsed as frames although they are part of the message string.
  • To skip n number of frames.

After this PR

We have two variables each having one purpose.

  • skipFirstLines to skip potentially dangerous lines that could be parsed as frames but aren't frames.
  • framesToPop to remove parsed frames.

Notes

This is especially useful for React applications as the framework internally uses the framesToPop property. And also for our own SDK.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 7, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.4 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.6 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.51 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.16 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.85 KB (+0.05% 🔺)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.85 KB (+0.05% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.36 KB (+0.06% 🔺)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.37 KB (+0.06% 🔺)
@sentry/browser - Webpack (gzipped) 22.57 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.47 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.08 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 32.94 KB (+0.1% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 23.98 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 210.59 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.39 KB (+0.05% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 71.67 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.09 KB (+0.11% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.88 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped) 22.6 KB (+0.06% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.3 KB (+0.02% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 49.72 KB (+0.03% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 17.41 KB (0%)

Copy link
Copy Markdown
Contributor

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Let's add a note to MIGRATION.md and we can merge this in!

Copy link
Copy Markdown
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.

Looks good. My only minor concern is that this modifies the test case to pass and these tests have been around for a long time. But if they were wrong, they were wrong!

@krystofwoldrich
Copy link
Copy Markdown
Contributor Author

@AbhiPrasad I've added the migration information, let me know if it's understandable, or if I should include an example.

@krystofwoldrich krystofwoldrich merged commit c52b1dd into develop Mar 6, 2024
@krystofwoldrich krystofwoldrich deleted the @krystofwoldrich/fix-frames-to-pop branch March 6, 2024 10:36
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.

SentryParser counts Error message lines in error.framesToPop

3 participants