-
-
Notifications
You must be signed in to change notification settings - Fork 357
fix(client): Keep captureMessage stack trace in exception #3988
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
Conversation
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d86dd6 | 405.14 ms | 411.06 ms | 5.92 ms |
| 2534337 | 394.15 ms | 415.12 ms | 20.97 ms |
| 27ef4ee | 317.40 ms | 321.70 ms | 4.30 ms |
| 5571a20 | 410.55 ms | 441.06 ms | 30.51 ms |
| 52a8031+dirty | 311.55 ms | 321.37 ms | 9.82 ms |
| 15c80ab+dirty | 336.27 ms | 350.58 ms | 14.31 ms |
| 62a750b | 395.96 ms | 423.36 ms | 27.41 ms |
| 457e29f | 398.10 ms | 421.39 ms | 23.29 ms |
| f06c879 | 408.41 ms | 424.54 ms | 16.13 ms |
| e73f4ed+dirty | 332.96 ms | 354.33 ms | 21.37 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d86dd6 | 17.73 MiB | 19.86 MiB | 2.12 MiB |
| 2534337 | 17.73 MiB | 19.84 MiB | 2.11 MiB |
| 27ef4ee | 17.73 MiB | 19.82 MiB | 2.08 MiB |
| 5571a20 | 17.73 MiB | 19.93 MiB | 2.19 MiB |
| 52a8031+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
| 15c80ab+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
| 62a750b | 17.73 MiB | 19.93 MiB | 2.20 MiB |
| 457e29f | 17.73 MiB | 19.84 MiB | 2.10 MiB |
| f06c879 | 17.73 MiB | 19.85 MiB | 2.12 MiB |
| e73f4ed+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
kahest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 🚀 also kudos for the great PR description
| (event: Event) => { | ||
| // TMP! Remove this function once JS SDK uses threads for messages | ||
| if (!event.exception?.values || event.exception.values.length <= 0) { | ||
| if (this._options.useThreadsForMessageStack) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So by default users will have the new behavior? otherwise, we could invert the if condition
| if (this._options.useThreadsForMessageStack) { | |
| if (!this._options.useThreadsForMessageStack) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, by default user will have the new behaviour which fixes the symbolication of the message stack trace. And with useThreadsForMessageStack we give user using the old behaviour easy way out until the next major (v6). We do this because based on the reports user expect the new (working symbolication) behaviour.
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment but other than that, LGTM!
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86d6d2c+dirty | 1267.55 ms | 1286.21 ms | 18.66 ms |
| e2b64fe+dirty | 1232.22 ms | 1255.20 ms | 22.98 ms |
| 9433f35+dirty | 1246.94 ms | 1271.45 ms | 24.52 ms |
| 4a6664f+dirty | 1209.49 ms | 1208.63 ms | -0.86 ms |
| 80b2ce3+dirty | 1265.92 ms | 1268.60 ms | 2.69 ms |
| dadc233+dirty | 1223.20 ms | 1236.88 ms | 13.68 ms |
| f06c879+dirty | 1252.64 ms | 1259.66 ms | 7.02 ms |
| 5a22220+dirty | 1209.49 ms | 1220.94 ms | 11.45 ms |
| 3853f43+dirty | 1221.82 ms | 1242.64 ms | 20.82 ms |
| 8900e1a+dirty | 1210.27 ms | 1218.66 ms | 8.39 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86d6d2c+dirty | 2.36 MiB | 2.82 MiB | 462.82 KiB |
| e2b64fe+dirty | 2.36 MiB | 2.85 MiB | 495.80 KiB |
| 9433f35+dirty | 2.36 MiB | 2.85 MiB | 499.80 KiB |
| 4a6664f+dirty | 2.36 MiB | 3.04 MiB | 696.39 KiB |
| 80b2ce3+dirty | 2.36 MiB | 2.84 MiB | 486.98 KiB |
| dadc233+dirty | 2.36 MiB | 2.84 MiB | 486.85 KiB |
| f06c879+dirty | 2.36 MiB | 2.88 MiB | 530.42 KiB |
| 5a22220+dirty | 2.36 MiB | 2.92 MiB | 570.21 KiB |
| 3853f43+dirty | 2.36 MiB | 2.85 MiB | 499.81 KiB |
| 8900e1a+dirty | 2.36 MiB | 2.83 MiB | 479.25 KiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86d6d2c+dirty | 1291.62 ms | 1296.80 ms | 5.18 ms |
| e2b64fe+dirty | 1285.78 ms | 1297.56 ms | 11.78 ms |
| 9433f35+dirty | 1232.24 ms | 1232.74 ms | 0.50 ms |
| 4a6664f+dirty | 1218.77 ms | 1221.07 ms | 2.30 ms |
| 80b2ce3+dirty | 1245.12 ms | 1262.04 ms | 16.92 ms |
| dadc233+dirty | 1266.52 ms | 1282.55 ms | 16.03 ms |
| f06c879+dirty | 1285.14 ms | 1285.86 ms | 0.72 ms |
| 5a22220+dirty | 1246.18 ms | 1249.61 ms | 3.43 ms |
| 3853f43+dirty | 1271.74 ms | 1278.04 ms | 6.30 ms |
| 8900e1a+dirty | 1268.36 ms | 1273.04 ms | 4.68 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86d6d2c+dirty | 2.92 MiB | 3.37 MiB | 464.31 KiB |
| e2b64fe+dirty | 2.92 MiB | 3.41 MiB | 499.97 KiB |
| 9433f35+dirty | 2.92 MiB | 3.41 MiB | 503.55 KiB |
| 4a6664f+dirty | 2.92 MiB | 3.60 MiB | 702.09 KiB |
| 80b2ce3+dirty | 2.92 MiB | 3.40 MiB | 492.75 KiB |
| dadc233+dirty | 2.92 MiB | 3.40 MiB | 492.53 KiB |
| f06c879+dirty | 2.92 MiB | 3.44 MiB | 533.24 KiB |
| 5a22220+dirty | 2.92 MiB | 3.48 MiB | 575.81 KiB |
| 3853f43+dirty | 2.92 MiB | 3.41 MiB | 503.54 KiB |
| 8900e1a+dirty | 2.92 MiB | 3.39 MiB | 485.96 KiB |
📢 Type of change
📜 Description
This PR backports revert: Moved message stack trace back to exception (fixes symbolication) #3635 from prepared RN SDK v6 to v5.
As this change is technically breaking for code using
threadsI addeduseThreadsForMessageStackwhich enables the old behavior.We should release this as non breaking fix, because
threadsare breaking symbolication forcaptureMessageand we are seeing large amount of reports.fixes Duplicate Issues for same event #3963
fixes getSentryExpoConfig(5.26.0) no longer unobfuscates stack traces with source maps #3967
fixes Sentry.captureMessage Stack Trace is not symbolicated #3097
Example event
💚 How did you test it?
sample app, unit tests
📝 Checklist
sendDefaultPIIis enabled