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(browser): Mark stack trace from captureMessage with attachStacktrace: true as synthetic #14668

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Dec 11, 2024

Background:
Whenever the attachStackTrace SDK option is set to true, the SDK will create and attach a synthetic error alongside events where we couldn't extract an actual stack trace from.

This PR adds the synthetic: true flag to the mechanism of a synthetic exception captured alongside a captureMessage call. Setting this property marks the exception as synthetic which, according to our develop spec, influences issue grouping.

We previously did not include any mechanism in such exceptions, lest did we mark them as synthetic, when calling Sentry.captureMessage(). This PR fixes that.

PR for server/node: #14670

Copy link
Member

@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.

Is synthetic: true used in grouping in any way? That's the only negative about adding this right now.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 23.29 KB +0.02% +4 B 🔺
@sentry/browser - with treeshaking flags 21.96 KB +0.03% +5 B 🔺
@sentry/browser (incl. Tracing) 35.79 KB +0.02% +6 B 🔺
@sentry/browser (incl. Tracing, Replay) 73.01 KB +0.01% +4 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.41 KB +0.01% +5 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 77.32 KB +0.01% +4 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 89.81 KB +0.01% +4 B 🔺
@sentry/browser (incl. Feedback) 40.04 KB +0.01% +4 B 🔺
@sentry/browser (incl. sendFeedback) 27.89 KB +0.02% +4 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.69 KB +0.02% +5 B 🔺
@sentry/react 25.96 KB +0.04% +8 B 🔺
@sentry/react (incl. Tracing) 38.6 KB +0.02% +6 B 🔺
@sentry/vue 27.49 KB +0.02% +5 B 🔺
@sentry/vue (incl. Tracing) 37.63 KB +0.02% +5 B 🔺
@sentry/svelte 23.45 KB +0.02% +4 B 🔺
CDN Bundle 24.43 KB +0.02% +4 B 🔺
CDN Bundle (incl. Tracing) 37.46 KB +0.01% +3 B 🔺
CDN Bundle (incl. Tracing, Replay) 72.64 KB +0.01% +3 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 78.01 KB +0.01% +4 B 🔺
CDN Bundle - uncompressed 71.74 KB +0.03% +21 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 111.05 KB +0.02% +21 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 225.12 KB +0.01% +21 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 238.33 KB +0.01% +21 B 🔺
@sentry/nextjs (client) 38.89 KB +0.02% +5 B 🔺
@sentry/sveltekit (client) 36.29 KB +0.02% +6 B 🔺
@sentry/node 162.5 KB - -
@sentry/node - without tracing 98.69 KB - -
@sentry/aws-serverless 126.36 KB - -

View base workflow run

@Lms24
Copy link
Member Author

Lms24 commented Dec 11, 2024

According to develop, synthetic influences grouping, yes. In my eyes, this is still a fix but we can't rule out that it'd create new issue groups for users. I will ask the issues team about this to confirm

@AbhiPrasad AbhiPrasad changed the title fix(browser): Mark stack trace from captureMessage with attatchStackTrace: true as synthetic fix(browser): Mark stack trace from captureMessage with attachStacktrace: true as synthetic Dec 11, 2024
@Lms24
Copy link
Member Author

Lms24 commented Dec 12, 2024

@AbhiPrasad I checked with the issues team and the synthetic flag doesn't influence a lot for grouping, especially not in our case where there is only one generic error type (Error) from our synthetic event. I also sent some test events with and without the synthetic flag. The events were grouped under the same issue. I think it's reasonably safe for us to make this change.

@Lms24 Lms24 requested a review from AbhiPrasad December 12, 2024 16:13
Copy link
Member

@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.

:shipit:

@Lms24 Lms24 merged commit 825fe89 into develop Dec 12, 2024
124 checks passed
@Lms24 Lms24 deleted the lms/fix-browser-synthetic-true-captureMessage branch December 12, 2024 18:29
Lms24 added a commit that referenced this pull request Dec 12, 2024
…race: true` as synthetic (#14670)

Analogously to #14668 for browser, this patch marks synthetic exceptions
captured during a `captureException()` call as synthetic.
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