Skip to content

fix(session-tracker): add session start for SDK start after didBecomeActive #5121

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

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

philprime
Copy link
Contributor

@philprime philprime commented Apr 22, 2025

📜 Description

On SDK startup we check if the app is already in foreground and manually trigger the logic of didBecomeActive.

💡 Motivation and Context

Currently the session tracker expects the didBecomeActive notification from the application life cycle after the SDK start. If we start the SDK after the app is opened, e.g. after fetching a remote config and initializing the SDK again, or if the user is asked for consent first, session tracking will not start until the app is in background for a while again.

Please see this comment for further details.

Closes #5069

💚 How did you test it?

  • Added unit tests
  • Started the sample "SessionReplay-CameraTest", set a breakpoint at SentryHub.startSession and tapped on toggle "Session Replay Enabled" as it closes and restarts the SDK. Before the changes the breakpoint would not be hit on toggles, but after it toggles everytime

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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
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.

@philprime it would be great if we could first have a PR to only fix the problem that the SDK doesn't start a session when it's started after didBecomeActive to keep the change small. I think that's a standalone improvement that has nothing to do with SR.

@philprime
Copy link
Contributor Author

@philipphofmann I agree, it seems like somehow other changes leaked into the PR during rebasing

Copy link

codecov bot commented Apr 22, 2025

Codecov Report

Attention: Patch coverage is 17.85714% with 23 lines in your changes missing coverage. Please review.

Project coverage is 9.183%. Comparing base (f96ab19) to head (04962a4).

Files with missing lines Patch % Lines
Sources/Sentry/SentrySessionTracker.m 0.000% 19 Missing ⚠️
Sources/Sentry/SentryNSNotificationCenterWrapper.m 0.000% 2 Missing ⚠️
Sources/Sentry/SentryUIApplication.m 60.000% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f96ab19) and HEAD (04962a4). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (f96ab19) HEAD (04962a4)
4 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #5121        +/-   ##
=============================================
- Coverage   92.903%   9.183%   -83.721%     
=============================================
  Files          691      362       -329     
  Lines        86707    26548     -60159     
  Branches     31235      121     -31114     
=============================================
- Hits         80554     2438     -78116     
- Misses        6054    24110     +18056     
+ Partials        99        0        -99     
Files with missing lines Coverage Δ
Sources/Sentry/SentryDependencyContainer.m 50.375% <100.000%> (-39.562%) ⬇️
Sources/Sentry/SentryNSNotificationCenterWrapper.m 0.000% <0.000%> (-100.000%) ⬇️
Sources/Sentry/SentryUIApplication.m 36.690% <60.000%> (-20.532%) ⬇️
Sources/Sentry/SentrySessionTracker.m 0.000% <0.000%> (-98.980%) ⬇️

... and 678 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 f96ab19...04962a4. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented Apr 22, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.04 ms 1249.15 ms 28.11 ms
Size 23.76 KiB 831.53 KiB 807.77 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
de033da 1216.91 ms 1222.84 ms 5.92 ms
fc350a4 1240.47 ms 1263.45 ms 22.98 ms
7db7c42 1221.73 ms 1249.59 ms 27.86 ms
0418702 1226.94 ms 1246.08 ms 19.14 ms
156e771 1228.06 ms 1242.64 ms 14.58 ms
f1c36e0 1215.18 ms 1223.62 ms 8.43 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
2124551 1231.78 ms 1264.94 ms 33.16 ms
9f0d9e0 1206.55 ms 1219.41 ms 12.86 ms
b9b0f0a 1235.61 ms 1237.40 ms 1.79 ms

App size

Revision Plain With Sentry Diff
de033da 21.58 KiB 418.15 KiB 396.57 KiB
fc350a4 21.90 KiB 708.34 KiB 686.44 KiB
7db7c42 22.31 KiB 775.27 KiB 752.95 KiB
0418702 21.58 KiB 704.20 KiB 682.62 KiB
156e771 20.76 KiB 419.70 KiB 398.94 KiB
f1c36e0 21.58 KiB 670.40 KiB 648.81 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
2124551 22.85 KiB 411.69 KiB 388.84 KiB
9f0d9e0 21.58 KiB 424.28 KiB 402.70 KiB
b9b0f0a 20.76 KiB 434.93 KiB 414.17 KiB

Previous results on branch: philprime/issue-5069

Startup times

Revision Plain With Sentry Diff
d7a0706 1228.17 ms 1246.63 ms 18.47 ms
fe0a171 1206.90 ms 1235.47 ms 28.57 ms
ff955bc 1219.57 ms 1251.20 ms 31.63 ms
4deddc5 1220.81 ms 1234.41 ms 13.60 ms
9c7c623 1231.18 ms 1249.28 ms 18.09 ms
7e28ca5 1223.96 ms 1248.59 ms 24.63 ms
5be643e 1217.51 ms 1246.15 ms 28.64 ms
7a1e241 1214.75 ms 1242.17 ms 27.42 ms

App size

Revision Plain With Sentry Diff
d7a0706 23.76 KiB 821.96 KiB 798.21 KiB
fe0a171 23.76 KiB 822.09 KiB 798.33 KiB
ff955bc 23.76 KiB 870.40 KiB 846.63 KiB
4deddc5 23.76 KiB 822.08 KiB 798.32 KiB
9c7c623 23.76 KiB 822.00 KiB 798.24 KiB
7e28ca5 23.76 KiB 870.39 KiB 846.63 KiB
5be643e 23.76 KiB 820.06 KiB 796.30 KiB
7a1e241 23.76 KiB 820.07 KiB 796.31 KiB

@philprime philprime marked this pull request as ready for review April 23, 2025 15:27
@philprime philprime requested a review from armcknight as a code owner April 23, 2025 15:27
@philprime philprime self-assigned this Apr 23, 2025
@philprime philprime moved this to Needs Review in [DEPRECATED] Mobile SDKs Apr 23, 2025
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.

There are a few tests failing. Either something is wrong with the test setup or the session logic is broken.

@philprime philprime marked this pull request as draft May 6, 2025 14:45
@philprime
Copy link
Contributor Author

After adding application state simulation to the unit tests, they fail with and without the changes of this PR. I'll will need to look into it with @philipphofmann to see if the tests are actually correct.

@philprime
Copy link
Contributor Author

After a discussion the existing tests are valid, therefore indicating that my changes in this PR are not valid yet.

// 1. The init session that was sent when the app started
// 2. The end session that was sent when the app stopped
// 3. The init session that was sent when the app restarted
XCTAssertEqual(fixture.client.captureSessionInvocations.invocations.count, 3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philipphofmann do we have any documentation on what the sequence of events should be in this case? I assumed the following but tests are failing. Now before I get back to the implementation I need to confirm this is expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Session Replay is not cleaned up correctly after calling Sentry.close
3 participants