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(sessions): mechanism.handled:false should crash current session #3900

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Jun 18, 2024

📢 Type of change

  • Bugfix

📜 Description

This PR updates the session handling, so native SDK can correctly handle crashing, ending and creating a new session.

Session will be crashed and no new session is started if { handled: false, type: 'onerror' }. Type onerror means the error is from the RN Global Handle => the native application is going to crash.

Session will be crashed and new session is started if { handled: false, type: '!onerror' }. Other types genetic... and others mean the exception is not from RN Global Handled (can be user set) and the native application is not going to crash.

Session will be crashed as no new session is started for native crashed.

🛑 Blocked by

💡 Motivation and Context

💚 How did you test it?

sample app

📝 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

Copy link
Contributor

github-actions bot commented Jun 18, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.63 ms 1219.75 ms 3.12 ms
Size 2.36 MiB 3.04 MiB 698.27 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
148f924+dirty 1214.76 ms 1215.73 ms 0.97 ms
31fcca2+dirty 1209.17 ms 1216.21 ms 7.04 ms
3ffcddd+dirty 1244.47 ms 1264.14 ms 19.67 ms
27ef4ee+dirty 1293.52 ms 1296.08 ms 2.56 ms
e5c9b8b+dirty 1258.57 ms 1267.32 ms 8.75 ms
e2b64fe+dirty 1232.22 ms 1255.20 ms 22.98 ms
1d86dd6+dirty 1249.71 ms 1279.16 ms 29.45 ms
c398f67+dirty 1219.67 ms 1225.66 ms 5.99 ms
575f9da+dirty 1266.22 ms 1274.84 ms 8.62 ms
2534337+dirty 1225.08 ms 1230.26 ms 5.17 ms

App size

Revision Plain With Sentry Diff
148f924+dirty 2.36 MiB 3.04 MiB 696.25 KiB
31fcca2+dirty 2.36 MiB 2.90 MiB 552.95 KiB
3ffcddd+dirty 2.36 MiB 2.84 MiB 489.60 KiB
27ef4ee+dirty 2.36 MiB 2.85 MiB 500.03 KiB
e5c9b8b+dirty 2.36 MiB 2.87 MiB 520.43 KiB
e2b64fe+dirty 2.36 MiB 2.85 MiB 495.80 KiB
1d86dd6+dirty 2.36 MiB 2.89 MiB 535.43 KiB
c398f67+dirty 2.36 MiB 3.04 MiB 696.27 KiB
575f9da+dirty 2.36 MiB 2.87 MiB 520.20 KiB
2534337+dirty 2.36 MiB 2.88 MiB 525.47 KiB

@lucas-zimerman
Copy link
Collaborator

It is missing the changelog, but other than that it looks good to me.
Not approving it for the moment because there are still blocked issues required for this PR that are still open.

Copy link
Contributor

github-actions bot commented Jun 26, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.71 ms 1220.87 ms -4.84 ms
Size 2.92 MiB 3.61 MiB 705.15 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
148f924+dirty 1220.72 ms 1221.30 ms 0.58 ms
31fcca2+dirty 1222.04 ms 1226.51 ms 4.47 ms
3ffcddd+dirty 1272.22 ms 1273.98 ms 1.76 ms
27ef4ee+dirty 1236.41 ms 1244.90 ms 8.49 ms
e5c9b8b+dirty 1276.90 ms 1280.92 ms 4.02 ms
e2b64fe+dirty 1285.78 ms 1297.56 ms 11.78 ms
1d86dd6+dirty 1289.25 ms 1293.36 ms 4.11 ms
c398f67+dirty 1227.31 ms 1230.00 ms 2.69 ms
575f9da+dirty 1272.00 ms 1284.38 ms 12.38 ms
2534337+dirty 1220.87 ms 1221.47 ms 0.60 ms

App size

Revision Plain With Sentry Diff
148f924+dirty 2.92 MiB 3.60 MiB 701.88 KiB
31fcca2+dirty 2.92 MiB 3.46 MiB 557.31 KiB
3ffcddd+dirty 2.92 MiB 3.40 MiB 494.39 KiB
27ef4ee+dirty 2.92 MiB 3.41 MiB 503.72 KiB
e5c9b8b+dirty 2.92 MiB 3.43 MiB 524.50 KiB
e2b64fe+dirty 2.92 MiB 3.41 MiB 499.97 KiB
1d86dd6+dirty 2.92 MiB 3.44 MiB 538.27 KiB
c398f67+dirty 2.92 MiB 3.60 MiB 701.89 KiB
575f9da+dirty 2.92 MiB 3.43 MiB 524.26 KiB
2534337+dirty 2.92 MiB 3.43 MiB 529.76 KiB

Copy link
Contributor

github-actions bot commented Jul 3, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 478.88 ms 532.66 ms 53.78 ms
Size 17.73 MiB 19.95 MiB 2.21 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
70e6261 482.65 ms 495.70 ms 13.05 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
5571a20 410.55 ms 441.06 ms 30.51 ms
1d86dd6 405.14 ms 411.06 ms 5.92 ms
d0bf494+dirty 375.37 ms 395.14 ms 19.77 ms
148f924 492.65 ms 500.28 ms 7.63 ms
27ef4ee 317.40 ms 321.70 ms 4.30 ms
5bb8d5f 431.21 ms 459.40 ms 28.19 ms
2534337 394.15 ms 415.12 ms 20.97 ms
80b2ce3 385.02 ms 387.36 ms 2.34 ms

App size

Revision Plain With Sentry Diff
70e6261 17.73 MiB 19.94 MiB 2.21 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
5571a20 17.73 MiB 19.93 MiB 2.19 MiB
1d86dd6 17.73 MiB 19.86 MiB 2.12 MiB
d0bf494+dirty 17.73 MiB 19.75 MiB 2.02 MiB
148f924 17.73 MiB 19.94 MiB 2.21 MiB
27ef4ee 17.73 MiB 19.82 MiB 2.08 MiB
5bb8d5f 17.73 MiB 19.93 MiB 2.20 MiB
2534337 17.73 MiB 19.84 MiB 2.11 MiB
80b2ce3 17.73 MiB 19.75 MiB 2.02 MiB

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

LGMT!

@krystofwoldrich krystofwoldrich merged commit 5290b95 into main Jul 3, 2024
59 of 61 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/fix-sessions branch July 3, 2024 15:19
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- `mechanism.handled:false` should crash current session ([#3900](https://github.com/getsentry/sentry-react-native/pull/3900))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 7fe38fd

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.

iOS doesn't update session when handled:false set in JS
2 participants