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

revert: Moved message stack trace back to exception (fixes symbolication) #3635

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Feb 27, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR rollsback the behaviour of moving the stack-trace from the threads to the exception, allowing message stack-trace to be symbolicated.

Before the change:
image

After the change:
https://sentry-sdks.sentry.io/issues/5016987060/events/2ae9c7850dde44c4a5458f6820d71724/?project=5428561
image

Fixes #3097

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

@lucas-zimerman lucas-zimerman added the Breaking-change should go in a major release (breaks apps, changes default configs in a major way) label Feb 27, 2024
Copy link
Contributor

github-actions bot commented Feb 27, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 393.89 ms 436.35 ms 42.46 ms
Size 17.73 MiB 19.93 MiB 2.19 MiB

@lucas-zimerman
Copy link
Collaborator Author

@krystofwoldrich I added a break-change because it generated a new issue group compared to not using this change.

Q: Should we keep the debugsymbolicator code for threads?

} else if (event.threads) {
// RN JS doesn't have threads
// syntheticException is used for Sentry.captureMessage() threads
symbolicatedFrames && this._replaceThreadFramesInEvent(event, symbolicatedFrames);
}

@lucas-zimerman lucas-zimerman marked this pull request as ready for review February 27, 2024 16:40
@krystofwoldrich
Copy link
Member

@lucas-zimerman Thank you, can you confirm #2131 still works after moving the stack back to expcetion, as that was the reason to move it to threads?

@krystofwoldrich
Copy link
Member

@krystofwoldrich I added a break-change because it generated a new issue group compared to not using this change.

Q: Should we keep the debugsymbolicator code for threads?

} else if (event.threads) {
// RN JS doesn't have threads
// syntheticException is used for Sentry.captureMessage() threads
symbolicatedFrames && this._replaceThreadFramesInEvent(event, symbolicatedFrames);
}

Yes, let's keep the symbolication for threads in, but let's remove the comment about thread being used in captureMessage.

@krystofwoldrich krystofwoldrich added this to the 6.0.0 milestone Feb 29, 2024
@krystofwoldrich
Copy link
Member

I agree with the breaking-change label, I'll create v6 branch and we can merge it there.

@lucas-zimerman
Copy link
Collaborator Author

@lucas-zimerman Thank you, can you confirm #2131 still works after moving the stack back to expcetion, as that was the reason to move it to threads?

will check and update this comment

@krystofwoldrich krystofwoldrich changed the base branch from main to v6 March 19, 2024 09:58
@krystofwoldrich krystofwoldrich changed the title Fix: Messages stack-trace are now symbolicated revert: Moved message stack trace back to exception (fixes symbolication) Mar 19, 2024
Copy link
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.52 ms 1224.81 ms 2.29 ms
Size 2.36 MiB 2.92 MiB 569.85 KiB

Copy link
Contributor

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 370.29 ms 432.08 ms 61.80 ms
Size 7.15 MiB 8.20 MiB 1.05 MiB

@krystofwoldrich krystofwoldrich merged commit 1aa3660 into v6 Mar 19, 2024
52 of 54 checks passed
@krystofwoldrich krystofwoldrich deleted the fix/cmessages branch March 19, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-change should go in a major release (breaks apps, changes default configs in a major way)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentry.captureMessage Stack Trace is not symbolicated
2 participants