Skip to content

fix(session-replay): Use low priority queue for dispatch queue wrapper #5280

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

Merged
merged 5 commits into from
May 22, 2025

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented May 22, 2025

📜 Description

Uses low-priority instead of background queues to reduce the chance of session replay internal multi-threading processes being dropped.

💡 Motivation and Context

While bumping the Cocoa SDK to v8.51.0 in React Native that contained the improved internal multi-threading of session replay we noticed that the Session Replay E2E tests were failing on iOS. Further investigation showed that the session replay processes were not being executed due to the QOS_CLASS_BACKGROUND priority on the CI environment that has limited resources. Bumping the priority to low (QOS_CLASS_UTILITY) worked as expected without compromising the improvements introduced in #5018
Note that locally on a dev machine the mentioned E2E tests do pass and we haven't managed to reproduce this edge case on a device or emulator.

💚 How did you test it?

Manual, CI

📝 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

codecov bot commented May 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.894%. Comparing base (22e759d) to head (cafb32f).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #5280       +/-   ##
=============================================
- Coverage   92.899%   92.894%   -0.005%     
=============================================
  Files          688       688               
  Lines        86275     86277        +2     
  Branches     31202     31207        +5     
=============================================
- Hits         80149     80147        -2     
- Misses        6026      6028        +2     
- Partials       100       102        +2     
Files with missing lines Coverage Δ
Sources/Sentry/SentryDispatchFactory.m 100.000% <100.000%> (ø)
Sources/Sentry/SentrySessionReplayIntegration.m 86.981% <100.000%> (ø)
...onReplay/SentrySessionReplayIntegrationTests.swift 99.049% <100.000%> (ø)
...entryTests/Networking/SentryDispatchFactoryTests.m 100.000% <100.000%> (ø)
...Tests/Networking/SentryDispatchQueueWrapperTests.m 100.000% <100.000%> (ø)

... and 14 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 22e759d...cafb32f. 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 May 22, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.60 ms 1244.60 ms 19.00 ms
Size 23.76 KiB 821.44 KiB 797.68 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2172278 1229.58 ms 1245.00 ms 15.42 ms
8aec30e 1235.73 ms 1255.87 ms 20.14 ms
7fe37ab 1228.92 ms 1243.86 ms 14.94 ms
005bb4c 1237.38 ms 1255.54 ms 18.16 ms
1ae5768 1232.08 ms 1248.16 ms 16.08 ms
cb5c79c 1229.31 ms 1251.92 ms 22.61 ms
ee8b48f 1196.80 ms 1213.48 ms 16.68 ms
455619d 1231.40 ms 1237.70 ms 6.30 ms
45d3ca5 1248.27 ms 1255.48 ms 7.21 ms
39b1c35 1235.90 ms 1257.37 ms 21.47 ms

App size

Revision Plain With Sentry Diff
2172278 21.58 KiB 542.28 KiB 520.70 KiB
8aec30e 21.58 KiB 616.75 KiB 595.17 KiB
7fe37ab 21.58 KiB 542.28 KiB 520.70 KiB
005bb4c 20.76 KiB 419.70 KiB 398.94 KiB
1ae5768 21.58 KiB 655.72 KiB 634.14 KiB
cb5c79c 22.30 KiB 730.30 KiB 708.00 KiB
ee8b48f 21.58 KiB 418.70 KiB 397.11 KiB
455619d 20.76 KiB 432.87 KiB 412.11 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB

Copy link
Contributor

@philprime philprime 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 for the investigation!

@antonis antonis marked this pull request as ready for review May 22, 2025 13:41
@antonis
Copy link
Collaborator Author

antonis commented May 22, 2025

Note that the CocoaPods Integration Tests also fail on main.

@philprime
Copy link
Contributor

@antonis I went ✅ after restart, so should be fine to merge

@antonis antonis merged commit ab197ed into main May 22, 2025
86 of 89 checks passed
@antonis antonis deleted the antonis/use-low-priority-queue branch May 22, 2025 15:33
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.

2 participants