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 31 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 20.00000% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.156%. Comparing base (e5c0f86) to head (a4c08ec).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Sentry/SentrySessionTracker.m 0.000% 16 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 (e5c0f86) and HEAD (a4c08ec). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (e5c0f86) HEAD (a4c08ec)
2 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #5121        +/-   ##
=============================================
- Coverage   92.763%   9.156%   -83.607%     
=============================================
  Files          687      362       -325     
  Lines        86135    26538     -59597     
  Branches     29983      121     -29862     
=============================================
- Hits         79902     2430     -77472     
- Misses        6139    24108     +17969     
+ Partials        94        0        -94     
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%> (-13.310%) ⬇️
Sources/Sentry/SentrySessionTracker.m 0.000% <0.000%> (-98.980%) ⬇️

... and 675 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 e5c0f86...a4c08ec. 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 1231.18 ms 1249.28 ms 18.09 ms
Size 23.76 KiB 822.00 KiB 798.24 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
be2977c 1202.51 ms 1221.32 ms 18.81 ms
d413317 1203.27 ms 1215.02 ms 11.75 ms
a4ffa76 1241.51 ms 1255.57 ms 14.06 ms
7207796 1223.80 ms 1235.92 ms 12.11 ms
5e65d2b 1216.08 ms 1236.08 ms 20.00 ms
9454d5d 1230.55 ms 1243.42 ms 12.87 ms
8aec30e 1222.72 ms 1237.73 ms 15.01 ms
7fe37ab 1236.10 ms 1258.83 ms 22.73 ms
6b16a50 1240.27 ms 1258.86 ms 18.59 ms
4d5eb78 1197.86 ms 1215.73 ms 17.88 ms

App size

Revision Plain With Sentry Diff
be2977c 22.85 KiB 407.67 KiB 384.83 KiB
d413317 20.76 KiB 420.71 KiB 399.95 KiB
a4ffa76 21.58 KiB 705.65 KiB 684.07 KiB
7207796 21.58 KiB 418.14 KiB 396.56 KiB
5e65d2b 21.58 KiB 616.67 KiB 595.09 KiB
9454d5d 20.76 KiB 436.29 KiB 415.53 KiB
8aec30e 21.58 KiB 616.76 KiB 595.18 KiB
7fe37ab 21.58 KiB 542.28 KiB 520.70 KiB
6b16a50 22.31 KiB 760.66 KiB 738.34 KiB
4d5eb78 21.58 KiB 418.74 KiB 397.16 KiB

Previous results on branch: philprime/issue-5069

Startup times

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

App size

Revision Plain With Sentry Diff
fe0a171 23.76 KiB 822.09 KiB 798.33 KiB
d7a0706 23.76 KiB 821.96 KiB 798.21 KiB
ff955bc 23.76 KiB 870.40 KiB 846.63 KiB
7a1e241 23.76 KiB 820.07 KiB 796.31 KiB
7e28ca5 23.76 KiB 870.39 KiB 846.63 KiB
5be643e 23.76 KiB 820.06 KiB 796.30 KiB
4deddc5 23.76 KiB 822.08 KiB 798.32 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.

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