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): storeEnvelope updates session when passed unhandled event #4073

Merged
merged 13 commits into from
Jun 20, 2024

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Jun 14, 2024

📜 Description

This PR add session update based on the stored envelope.

Stored envelope with handled: false ends session as crashed and doe not start a new session. This is important because store envelope will be called by RN only for unhandled errors (not user set but based on RN error handler) these error are re-thrown by the JS engine on the native layers which will cause the application to crash.

If the stored envelope from RN is actual crash the application crashes, when the app is opened a new session is started? Is this wanted behaviour? Does it matches native crashes?

RN on Android handled: false increments errors count. When handled:false does not crash the app the same session is being updated. This is unwanted and will be updated in getsentry/sentry-java#3480

💡 Motivation and Context

💚 How did you test it?

RN sample app, unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.126%. Comparing base (fa1ec44) to head (fe9d4b9).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4073       +/-   ##
=============================================
+ Coverage   91.090%   91.126%   +0.035%     
=============================================
  Files          611       610        -1     
  Lines        47838     47872       +34     
=============================================
+ Hits         43576     43624       +48     
+ Misses        4262      4248       -14     
Files Coverage Δ
SentryTestUtils/TestHub.swift 72.000% <100.000%> (+11.130%) ⬆️
Sources/Sentry/SentrySDK.m 89.595% <100.000%> (-0.060%) ⬇️
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift 99.625% <100.000%> (+0.062%) ⬆️
Sources/Sentry/SentryHub.m 98.066% <95.000%> (-0.225%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa1ec44...fe9d4b9. Read the comment docs.

Copy link

github-actions bot commented Jun 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.00 ms 1234.59 ms 13.59 ms
Size 21.58 KiB 670.46 KiB 648.88 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
42f4107 1224.52 ms 1244.77 ms 20.25 ms
aeec206 1211.31 ms 1229.18 ms 17.87 ms
caa37b6 1221.19 ms 1238.71 ms 17.52 ms
160a073 1260.72 ms 1270.10 ms 9.38 ms
b11cd87 1243.19 ms 1260.84 ms 17.65 ms
b064665 1230.04 ms 1254.08 ms 24.04 ms
90d17d3 1261.18 ms 1278.18 ms 17.00 ms
ea73af6 1230.96 ms 1244.98 ms 14.02 ms
a366f3b 1230.65 ms 1251.88 ms 21.23 ms
1656cf6 1229.59 ms 1245.52 ms 15.93 ms

App size

Revision Plain With Sentry Diff
42f4107 21.58 KiB 614.92 KiB 593.34 KiB
aeec206 20.76 KiB 434.88 KiB 414.12 KiB
caa37b6 21.58 KiB 424.34 KiB 402.76 KiB
160a073 22.85 KiB 408.88 KiB 386.03 KiB
b11cd87 21.58 KiB 631.85 KiB 610.27 KiB
b064665 22.85 KiB 413.42 KiB 390.57 KiB
90d17d3 20.76 KiB 432.17 KiB 411.41 KiB
ea73af6 20.76 KiB 425.75 KiB 404.99 KiB
a366f3b 21.58 KiB 614.73 KiB 593.15 KiB
1656cf6 21.58 KiB 546.19 KiB 524.61 KiB

Previous results on branch: kw/fix-store-envelope-updates-session

Startup times

Revision Plain With Sentry Diff
d2f6b88 1218.40 ms 1231.74 ms 13.35 ms
90feb2d 1217.77 ms 1233.50 ms 15.73 ms
e118e16 1216.98 ms 1226.94 ms 9.96 ms

App size

Revision Plain With Sentry Diff
d2f6b88 21.58 KiB 670.58 KiB 649.00 KiB
90feb2d 21.58 KiB 669.96 KiB 648.38 KiB
e118e16 21.58 KiB 669.72 KiB 648.14 KiB

@krystofwoldrich krystofwoldrich marked this pull request as ready for review June 17, 2024 13:56
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this in the Cocoa SDK.

Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift Outdated Show resolved Hide resolved
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift Outdated Show resolved Hide resolved
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift Outdated Show resolved Hide resolved
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

It looks good. Just some nitpicks on top of Philipp already commented

Sources/Sentry/SentryHub.m Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryHub+Private.h Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) June 20, 2024 08:03
@krystofwoldrich krystofwoldrich merged commit 8001609 into main Jun 20, 2024
66 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/fix-store-envelope-updates-session branch June 20, 2024 08:09
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
3 participants