Skip to content

fix: Add persistence for context to fix watchdog termination events #5242

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 48 commits into
base: main
Choose a base branch
from

Conversation

philprime
Copy link
Contributor

@philprime philprime commented May 16, 2025

📜 Description

  • Adds file persistence of the scope context to serialize for watchdog termination events

  • Changes the watchdog termination scope observer as a collective entry point forwarding events to multiple processors to segment the logic into individual, testable components

  • Refactors the existing breadcrumb persistence logic into a SentryWatchdogTerminationBreadcrumbProcessor. The implementation is not changed in this PR but prepared for future refactoring. A lot of the file operations are split with the SentryFileManager and would require more work.

  • Adds a SentryWatchdogTerminationContextProcessor to process and persist the context to the file system. It uses the newly introduced `Sentry

  • To not add even more logic to the SentryFileManager I added a wrapper store SentryScopeContextPersistentStore which handles encoding/decoding and file operations using the file manager.

  • The SentryWatchdogTerminationContextProcessor is dispatching the saving via the persistent store on a background thread to reduce the impact on the main thread.

  • The dispatch queue wrapper is created by the SentryDispatchFactory

In general I tried to bring a more constructor-based dependency injection style architecture into this PR, to make them replaceable and testable as individual units.

Originally the logic of SentryScopeContextPersistentStore was in the SentryWatchdogTerminationContextProcessor, therefore it's functionality is now slim and we could consider moving the method directly to the SentryWatchdogTerminationScopeObserver, but that would then. not match with the breadcrumb processor.

I believe the additional file is fine for now and can be refactored again at a future point.

💡 Motivation and Context

Closes #5182

💚 How did you test it?

📝 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.

Blocked by

Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 0% with 194 lines in your changes missing coverage. Please review.

Project coverage is 9.134%. Comparing base (3851109) to head (054aadc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ors/SentryWatchdogTerminationBreadcrumbProcessor.m 0.000% 55 Missing ⚠️
...ersistence/SentryScopeContextPersistentStore.swift 0.000% 46 Missing ⚠️
Sources/Sentry/SentryWatchdogTerminationTracker.m 0.000% 25 Missing ⚠️
Sources/Sentry/SentryDependencyContainer.m 0.000% 23 Missing ⚠️
...rs/SentryWatchdogTerminationContextProcessor.swift 0.000% 21 Missing ⚠️
...try/SentryWatchdogTerminationTrackingIntegration.m 0.000% 13 Missing ⚠️
...es/Sentry/SentryWatchdogTerminationScopeObserver.m 0.000% 7 Missing ⚠️
Sources/Sentry/SentryFileManager.m 0.000% 2 Missing ⚠️
Sources/Sentry/SentrySDK.m 0.000% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3851109) and HEAD (054aadc). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (3851109) HEAD (054aadc)
4 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #5242        +/-   ##
=============================================
- Coverage   92.936%   9.134%   -83.802%     
=============================================
  Files          692      365       -327     
  Lines        86781    26647     -60134     
  Branches     31253      121     -31132     
=============================================
- Hits         80651     2434     -78217     
- Misses        6029    24213     +18184     
+ Partials       101        0       -101     
Files with missing lines Coverage Δ
Sources/Sentry/SentryFileManager.m 18.348% <0.000%> (-75.803%) ⬇️
Sources/Sentry/SentrySDK.m 8.591% <0.000%> (-80.149%) ⬇️
...es/Sentry/SentryWatchdogTerminationScopeObserver.m 0.000% <0.000%> (-74.726%) ⬇️
...try/SentryWatchdogTerminationTrackingIntegration.m 0.000% <0.000%> (-84.375%) ⬇️
...rs/SentryWatchdogTerminationContextProcessor.swift 0.000% <0.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m 41.290% <0.000%> (-48.647%) ⬇️
Sources/Sentry/SentryWatchdogTerminationTracker.m 0.000% <0.000%> (-100.000%) ⬇️
...ersistence/SentryScopeContextPersistentStore.swift 0.000% <0.000%> (ø)
...ors/SentryWatchdogTerminationBreadcrumbProcessor.m 0.000% <0.000%> (ø)

... and 676 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 e2347a6...054aadc. 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 16, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1209.27 ms 1239.76 ms 30.48 ms
Size 23.76 KiB 830.18 KiB 806.42 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
56ec5d0 1194.39 ms 1236.94 ms 42.55 ms
302ee8b 1194.02 ms 1223.34 ms 29.32 ms
9f0d9e0 1212.49 ms 1220.27 ms 7.78 ms
542727d 1227.96 ms 1246.88 ms 18.92 ms
bd05478 1233.82 ms 1255.56 ms 21.74 ms
c78683b 1255.78 ms 1261.94 ms 6.16 ms
f1b97be 1212.69 ms 1226.12 ms 13.43 ms
02a972c 1222.73 ms 1235.20 ms 12.47 ms
2d35ccb 1220.40 ms 1239.16 ms 18.77 ms
7192d9e 1221.86 ms 1248.28 ms 26.42 ms

App size

Revision Plain With Sentry Diff
56ec5d0 20.76 KiB 414.44 KiB 393.68 KiB
302ee8b 20.76 KiB 419.62 KiB 398.87 KiB
9f0d9e0 21.58 KiB 424.28 KiB 402.70 KiB
542727d 21.58 KiB 571.85 KiB 550.26 KiB
bd05478 20.76 KiB 432.33 KiB 411.57 KiB
c78683b 20.76 KiB 435.24 KiB 414.47 KiB
f1b97be 21.58 KiB 681.73 KiB 660.15 KiB
02a972c 22.85 KiB 413.42 KiB 390.57 KiB
2d35ccb 21.58 KiB 705.96 KiB 684.38 KiB
7192d9e 20.76 KiB 431.71 KiB 410.95 KiB

Previous results on branch: philprime/issue-5182

Startup times

Revision Plain With Sentry Diff
c9efa02 1229.42 ms 1251.57 ms 22.16 ms
9b69890 1244.68 ms 1258.16 ms 13.48 ms
69ce027 1232.45 ms 1251.65 ms 19.21 ms
35d6c88 1229.02 ms 1254.54 ms 25.52 ms
c5b273a 1212.13 ms 1235.94 ms 23.81 ms
bcd1b71 1217.71 ms 1229.96 ms 12.25 ms
c34e9ed 1223.86 ms 1249.84 ms 25.98 ms
278b49a 1221.16 ms 1246.98 ms 25.82 ms
496853a 1225.18 ms 1244.33 ms 19.15 ms
8c53c21 1223.46 ms 1246.42 ms 22.96 ms

App size

Revision Plain With Sentry Diff
c9efa02 23.76 KiB 829.31 KiB 805.55 KiB
9b69890 23.76 KiB 874.87 KiB 851.11 KiB
69ce027 23.76 KiB 829.42 KiB 805.66 KiB
35d6c88 23.76 KiB 821.23 KiB 797.47 KiB
c5b273a 23.76 KiB 829.41 KiB 805.65 KiB
bcd1b71 23.76 KiB 829.42 KiB 805.66 KiB
c34e9ed 23.76 KiB 829.34 KiB 805.59 KiB
278b49a 23.76 KiB 829.80 KiB 806.04 KiB
496853a 23.76 KiB 830.09 KiB 806.33 KiB
8c53c21 23.76 KiB 829.42 KiB 805.66 KiB

@philprime philprime self-assigned this May 16, 2025
@philprime philprime marked this pull request as ready for review May 21, 2025 14:27
@philprime philprime requested a review from philipphofmann May 22, 2025 08:42
@philprime philprime marked this pull request as draft May 23, 2025 11:45
@philprime
Copy link
Contributor Author

This PR grew quite in size. I am now going to extract smaller PRs for necessary changes than can be seen as isolated changes (i.e. fixing compiler flags or serialization).

Copy link
Contributor

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryFileManager.m

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.

Missing device info on watchdog_termination events
3 participants