-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5242 +/- ##
=============================================
+ Coverage 92.907% 93.002% +0.095%
=============================================
Files 691 702 +11
Lines 86706 87873 +1167
Branches 31223 31528 +305
=============================================
+ Hits 80556 81724 +1168
+ Misses 6050 6049 -1
Partials 100 100
... and 10 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 |
---|---|---|---|
8cf5bca | 1231.33 ms | 1237.81 ms | 6.48 ms |
de033da | 1206.55 ms | 1217.08 ms | 10.53 ms |
beb7ff7 | 1211.40 ms | 1224.12 ms | 12.73 ms |
5949b9e | 1235.45 ms | 1260.21 ms | 24.76 ms |
4bca912 | 1252.42 ms | 1260.06 ms | 7.64 ms |
313b1d9 | 1240.18 ms | 1258.44 ms | 18.26 ms |
5783680 | 1223.63 ms | 1243.60 ms | 19.97 ms |
2124551 | 1201.23 ms | 1225.34 ms | 24.11 ms |
b9b0f0a | 1220.20 ms | 1229.27 ms | 9.06 ms |
c0ff306 | 1218.92 ms | 1240.64 ms | 21.72 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8cf5bca | 21.58 KiB | 671.30 KiB | 649.72 KiB |
de033da | 21.58 KiB | 418.15 KiB | 396.57 KiB |
beb7ff7 | 21.58 KiB | 669.46 KiB | 647.87 KiB |
5949b9e | 22.30 KiB | 832.29 KiB | 809.98 KiB |
4bca912 | 22.85 KiB | 411.14 KiB | 388.29 KiB |
313b1d9 | 22.85 KiB | 414.11 KiB | 391.26 KiB |
5783680 | 21.58 KiB | 714.18 KiB | 692.60 KiB |
2124551 | 22.85 KiB | 411.69 KiB | 388.84 KiB |
b9b0f0a | 20.76 KiB | 434.94 KiB | 414.18 KiB |
c0ff306 | 20.76 KiB | 434.65 KiB | 413.89 KiB |
Previous results on branch: philprime/issue-5182
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9b69890 | 1244.68 ms | 1258.16 ms | 13.48 ms |
69ce027 | 1232.45 ms | 1251.65 ms | 19.21 ms |
c34e9ed | 1223.86 ms | 1249.84 ms | 25.98 ms |
c5b273a | 1212.13 ms | 1235.94 ms | 23.81 ms |
c3fb252 | 1224.10 ms | 1245.00 ms | 20.90 ms |
bcd1b71 | 1217.71 ms | 1229.96 ms | 12.25 ms |
8c53c21 | 1223.46 ms | 1246.42 ms | 22.96 ms |
c9efa02 | 1229.42 ms | 1251.57 ms | 22.16 ms |
496853a | 1225.18 ms | 1244.33 ms | 19.15 ms |
691ac4a | 1218.51 ms | 1244.67 ms | 26.16 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9b69890 | 23.76 KiB | 874.87 KiB | 851.11 KiB |
69ce027 | 23.76 KiB | 829.42 KiB | 805.66 KiB |
c34e9ed | 23.76 KiB | 829.34 KiB | 805.59 KiB |
c5b273a | 23.76 KiB | 829.41 KiB | 805.65 KiB |
c3fb252 | 23.76 KiB | 835.19 KiB | 811.43 KiB |
bcd1b71 | 23.76 KiB | 829.42 KiB | 805.66 KiB |
8c53c21 | 23.76 KiB | 829.42 KiB | 805.66 KiB |
c9efa02 | 23.76 KiB | 829.31 KiB | 805.55 KiB |
496853a | 23.76 KiB | 830.09 KiB | 806.33 KiB |
691ac4a | 23.76 KiB | 836.21 KiB | 812.45 KiB |
👋 Any idea when this fix might get merged? We are very interested in it 😄 as its blocking us from upgrading Sentry from |
Hi @metcalfealex I believe the PR is ready for review now. As soon as my colleagues get a chance to review it, we'll make sure to merge it soon. This issue has the highest priority right now, but grew larger in size than expected. |
Sources/Sentry/include/HybridPublic/SentryDependencyContainer.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for integrating all the feedback. I found a few minor issues, and one important one to address before we can merge this.
Sources/Sentry/Processors/SentryWatchdogTerminationBreadcrumbProcessor.m
Show resolved
Hide resolved
Sources/Sentry/Processors/SentryWatchdogTerminationBreadcrumbProcessor.m
Show resolved
Hide resolved
...sts/Integrations/WatchdogTerminations/TestSentryWatchdogTerminationBreadcrumbProcessor.swift
Show resolved
Hide resolved
Sources/Swift/Persistence/SentryScopeContextPersistentStore.swift
Outdated
Show resolved
Hide resolved
…sistentStore - Included SentryNSDictionarySanitize.h in SentryPrivate.h. - Updated SentryScopeContextPersistentStore to sanitize context before serialization. - Added a test to verify that non-literal JSON dictionaries are sanitized and written correctly to disk. - Introduced a new test for validating the Sentry path URL generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks. I know that there are still a few comments to resolve, but I have no strong opinion on these. I don't want to block this PR any longer. Please let me know if you want a nother round of review.
🚨 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:
|
Thanks for getting this in @philprime ! Any idea when a release will be out with this fix? To complicate things we would also need the sentry RN version to pick it up 🙏 |
@metcalfealex I'll check internally regarding any additional work that should be done before the next release and will get back to you when I have an ETA. |
@metcalfealex we released this change in the latest version https://github.com/getsentry/sentry-cocoa/releases/tag/8.52.0 Please notice the warning on the in the release notes, as we are currently working on a hotfix 8.52.1, you might want to wait for. |
Thanks @philipphofmann! we really appreciate the quick turnaound! |
📜 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