-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
... and 676 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
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 |
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). |
🚨 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:
|
📜 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 theSentryFileManager
and would require more work.Adds a
SentryWatchdogTerminationContextProcessor
to process and persist the context to the file system. It uses the newly introduced `SentryTo not add even more logic to the
SentryFileManager
I added a wrapper storeSentryScopeContextPersistentStore
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 theSentryWatchdogTerminationContextProcessor
, therefore it's functionality is now slim and we could consider moving the method directly to theSentryWatchdogTerminationScopeObserver
, 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:
sendDefaultPII
is enabled.Blocked by