Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: Add unhandled rejection mechanism #4457

Merged
merged 6 commits into from
Jan 21, 2025

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Jan 16, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Unhandled rejection were lacking the Mechanism field, so I added it.

💡 Motivation and Context

#4093

💚 How did you test it?

Unit test and https://sentry-sdks.sentry.io/issues/6018946733/events/91e85dfdbd5340c487e7d279378441d4/?project=5428561

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

@lucas-zimerman lucas-zimerman marked this pull request as ready for review January 16, 2025 18:16
Copy link
Contributor

github-actions bot commented Jan 16, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1238.00 ms 1230.91 ms -7.09 ms
Size 2.63 MiB 3.69 MiB 1.05 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
9c48b2c+dirty 1246.96 ms 1255.73 ms 8.77 ms
70caa60+dirty 1218.27 ms 1230.30 ms 12.03 ms
575f9da+dirty 1266.22 ms 1274.84 ms 8.62 ms
52a8031+dirty 1280.88 ms 1289.78 ms 8.90 ms
5571a20+dirty 1203.57 ms 1204.57 ms 1.00 ms
abb7058+dirty 1255.42 ms 1268.86 ms 13.44 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms
9dabcce+dirty 1231.39 ms 1238.02 ms 6.63 ms
7bc4d75+dirty 1233.40 ms 1229.56 ms -3.83 ms

App size

Revision Plain With Sentry Diff
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
9c48b2c+dirty 2.36 MiB 2.85 MiB 495.77 KiB
70caa60+dirty 2.36 MiB 2.83 MiB 479.27 KiB
575f9da+dirty 2.36 MiB 2.87 MiB 520.20 KiB
52a8031+dirty 2.36 MiB 2.82 MiB 469.44 KiB
5571a20+dirty 2.36 MiB 2.92 MiB 569.93 KiB
abb7058+dirty 2.36 MiB 2.87 MiB 520.42 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB
9dabcce+dirty 2.36 MiB 3.10 MiB 757.52 KiB
7bc4d75+dirty 2.36 MiB 3.10 MiB 752.58 KiB

Previous results on branch: lz/fix/add-missing-mechanism

Startup times

Revision Plain With Sentry Diff
bc7d1c7+dirty 1241.52 ms 1234.18 ms -7.34 ms

App size

Revision Plain With Sentry Diff
bc7d1c7+dirty 2.63 MiB 3.69 MiB 1.05 MiB

Copy link
Contributor

github-actions bot commented Jan 16, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.70 ms 1247.52 ms 16.82 ms
Size 3.19 MiB 4.25 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
9c48b2c+dirty 1253.39 ms 1256.30 ms 2.91 ms
70caa60+dirty 1279.08 ms 1281.54 ms 2.46 ms
575f9da+dirty 1272.00 ms 1284.38 ms 12.38 ms
52a8031+dirty 1255.96 ms 1273.00 ms 17.04 ms
5571a20+dirty 1228.09 ms 1233.45 ms 5.36 ms
abb7058+dirty 1260.28 ms 1266.56 ms 6.28 ms
15c80ab+dirty 1248.41 ms 1251.24 ms 2.83 ms
9dabcce+dirty 1247.71 ms 1239.18 ms -8.53 ms
7bc4d75+dirty 1222.13 ms 1216.39 ms -5.74 ms

App size

Revision Plain With Sentry Diff
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
9c48b2c+dirty 2.92 MiB 3.41 MiB 499.97 KiB
70caa60+dirty 2.92 MiB 3.39 MiB 486.04 KiB
575f9da+dirty 2.92 MiB 3.43 MiB 524.26 KiB
52a8031+dirty 2.92 MiB 3.38 MiB 475.71 KiB
5571a20+dirty 2.92 MiB 3.48 MiB 575.54 KiB
abb7058+dirty 2.92 MiB 3.43 MiB 524.53 KiB
15c80ab+dirty 2.92 MiB 3.39 MiB 481.56 KiB
9dabcce+dirty 2.92 MiB 3.67 MiB 770.02 KiB
7bc4d75+dirty 2.92 MiB 3.66 MiB 757.15 KiB

Previous results on branch: lz/fix/add-missing-mechanism

Startup times

Revision Plain With Sentry Diff
bc7d1c7+dirty 1226.84 ms 1213.61 ms -13.23 ms

App size

Revision Plain With Sentry Diff
bc7d1c7+dirty 3.19 MiB 4.25 MiB 1.06 MiB

Copy link
Contributor

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 352.22 ms 343.33 ms -8.89 ms
Size 17.75 MiB 20.11 MiB 2.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
83f6f6c 418.83 ms 410.94 ms -7.89 ms
8c88ac7 411.15 ms 400.54 ms -10.60 ms
baa882f 354.93 ms 356.18 ms 1.25 ms
12427f4 393.69 ms 414.84 ms 21.14 ms
c830127 407.57 ms 409.50 ms 1.93 ms
5571a20 410.55 ms 441.06 ms 30.51 ms
700cbf4 425.56 ms 436.26 ms 10.70 ms
690220d 468.28 ms 480.06 ms 11.78 ms
75774ea 454.16 ms 467.80 ms 13.64 ms
800171e 479.44 ms 462.86 ms -16.58 ms

App size

Revision Plain With Sentry Diff
83f6f6c 17.74 MiB 20.09 MiB 2.35 MiB
8c88ac7 17.74 MiB 20.08 MiB 2.34 MiB
baa882f 17.73 MiB 20.06 MiB 2.33 MiB
12427f4 17.73 MiB 19.85 MiB 2.12 MiB
c830127 17.74 MiB 20.10 MiB 2.36 MiB
5571a20 17.73 MiB 19.93 MiB 2.19 MiB
700cbf4 17.73 MiB 20.07 MiB 2.33 MiB
690220d 17.74 MiB 20.08 MiB 2.35 MiB
75774ea 17.74 MiB 20.08 MiB 2.35 MiB
800171e 17.75 MiB 20.11 MiB 2.36 MiB

Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

github-actions bot commented Jan 17, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 395.02 ms 455.00 ms 59.98 ms
Size 7.15 MiB 8.38 MiB 1.23 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8d251c2+dirty 376.67 ms 417.91 ms 41.24 ms
6c56eb1+dirty 395.28 ms 378.52 ms -16.76 ms
8ab11b6+dirty 391.36 ms 417.86 ms 26.50 ms
8c88ac7+dirty 402.72 ms 434.32 ms 31.60 ms
c6f01ea+dirty 379.95 ms 439.35 ms 59.40 ms
63ed251+dirty 485.02 ms 531.16 ms 46.14 ms
b8ff156+dirty 386.72 ms 398.18 ms 11.46 ms
acadc0f+dirty 259.04 ms 304.67 ms 45.63 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 ms

App size

Revision Plain With Sentry Diff
8d251c2+dirty 7.15 MiB 8.38 MiB 1.23 MiB
6c56eb1+dirty 7.15 MiB 8.37 MiB 1.22 MiB
8ab11b6+dirty 7.15 MiB 8.37 MiB 1.22 MiB
8c88ac7+dirty 7.15 MiB 8.35 MiB 1.20 MiB
c6f01ea+dirty 7.15 MiB 8.37 MiB 1.22 MiB
63ed251+dirty 7.15 MiB 8.35 MiB 1.20 MiB
b8ff156+dirty 7.15 MiB 8.37 MiB 1.22 MiB
acadc0f+dirty 7.15 MiB 8.03 MiB 903.20 KiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB

Previous results on branch: lz/fix/add-missing-mechanism

Startup times

Revision Plain With Sentry Diff
bc7d1c7+dirty 390.87 ms 433.86 ms 42.99 ms

App size

Revision Plain With Sentry Diff
bc7d1c7+dirty 7.15 MiB 8.38 MiB 1.23 MiB

@@ -81,6 +81,7 @@ function attachUnhandledRejectionHandler(): void {
data: { id },
originalException: error,
syntheticException: isErrorLike(error) ? undefined : createSyntheticError(),
mechanism: { handled: false, type: 'onunhandledrejection' },
Copy link
Member

Choose a reason for hiding this comment

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

We have to mark the rejection handled: true to avoid breaking crash rate calculations.

Rejections do not crash the app in RN at the moment.

#4141

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed from false to true on 36f0f3a

This flag always confused me on other SDKs since depending on the platform an unhandled error could crash or not an app.

Copy link
Member

@krystofwoldrich krystofwoldrich Jan 21, 2025

Choose a reason for hiding this comment

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

Yeah, its usage is less than ideal.

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Although seemingly wrong, we have to mark rejections as handled: true. See my comment above.

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you!

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) January 21, 2025 16:19
@krystofwoldrich krystofwoldrich merged commit 94b806c into main Jan 21, 2025
62 of 64 checks passed
@krystofwoldrich krystofwoldrich deleted the lz/fix/add-missing-mechanism branch January 21, 2025 16:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants