Skip to content

feat: Update Persistent store and processor to handle multiple data types #5558

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 7, 2025
Merged

feat: Update Persistent store and processor to handle multiple data types #5558

merged 10 commits into from
Jul 7, 2025

Conversation

itaybre
Copy link
Contributor

@itaybre itaybre commented Jul 2, 2025

Update Persistent store and processor to handle multiple data types

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 2, 2025 •
edited
Loading

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against cf10371

@codecov Codecov
Copy link

codecov bot commented Jul 2, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.98990% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.341%. Comparing base (ea5a59b) to head (cf10371).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...Swift/Persistence/SentryScopePersistentStore.swift 98.591% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #5558       +/-   ##
=============================================
+ Coverage   86.175%   86.341%   +0.165%     
=============================================
  Files          407       407               
  Lines        35091     35165       +74     
  Branches     15025     15271      +246     
=============================================
+ Hits         30240     30362      +122     
+ Misses        4808      4756       -52     
- Partials        43        47        +4
Files with missing lines Coverage Δ
Sources/Sentry/SentryDependencyContainer.m 88.372% <100.000%> (ø)
Sources/Sentry/SentrySDK.m 88.095% <100.000%> (ø)
...es/Sentry/SentryWatchdogTerminationScopeObserver.m 62.162% <100.000%> (+6.606%) ⬆️
Sources/Sentry/SentryWatchdogTerminationTracker.m 100.000% <100.000%> (ø)
...try/SentryWatchdogTerminationTrackingIntegration.m 84.507% <100.000%> (+0.221%) ⬆️
...SentryWatchdogTerminationAttributesProcessor.swift 100.000% <100.000%> (ø)
...Swift/Persistence/SentryScopePersistentStore.swift 98.591% <98.591%> (ø)

... and 20 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 ea5a59b...cf10371. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 2, 2025 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1240.29 ms 1261.47 ms 21.18 ms
Size 23.75 KiB 877.14 KiB 853.39 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4d264fa 1223.48 ms 1246.91 ms 23.44 ms
d637379 1226.43 ms 1250.77 ms 24.34 ms
61414e8 1225.49 ms 1254.28 ms 28.79 ms
63ac649 1192.10 ms 1216.78 ms 24.68 ms
f92cfa9 1217.94 ms 1240.06 ms 22.12 ms
65f8d2e 1221.15 ms 1243.96 ms 22.81 ms
fd5961e 1210.59 ms 1235.57 ms 24.98 ms
701b301 1226.10 ms 1245.57 ms 19.47 ms
f92cfa9 1228.45 ms 1251.33 ms 22.88 ms
aa96485 1215.37 ms 1234.04 ms 18.67 ms

App size

Revision Plain With Sentry Diff
4d264fa 23.74 KiB 874.07 KiB 850.33 KiB
d637379 23.75 KiB 855.38 KiB 831.63 KiB
61414e8 23.75 KiB 867.69 KiB 843.94 KiB
63ac649 23.75 KiB 855.38 KiB 831.63 KiB
f92cfa9 23.75 KiB 855.38 KiB 831.63 KiB
65f8d2e 23.74 KiB 872.67 KiB 848.93 KiB
fd5961e 23.74 KiB 874.07 KiB 850.32 KiB
701b301 23.75 KiB 867.16 KiB 843.41 KiB
f92cfa9 23.75 KiB 855.38 KiB 831.62 KiB
aa96485 23.75 KiB 874.46 KiB 850.71 KiB

Previous results on branch: itay/cocoa-412-add-missing-information-to-watchdog-termination-events-unified

Startup times

Revision Plain With Sentry Diff
399e976 1227.44 ms 1246.69 ms 19.26 ms
45b6f56 1226.98 ms 1252.40 ms 25.42 ms
ec06b2b 1230.94 ms 1250.49 ms 19.55 ms
bab2c7f 1219.65 ms 1242.41 ms 22.76 ms
54477e0 1221.39 ms 1236.63 ms 15.24 ms
b06498a 1228.43 ms 1246.25 ms 17.82 ms

App size

Revision Plain With Sentry Diff
399e976 23.74 KiB 877.02 KiB 853.28 KiB
45b6f56 23.75 KiB 877.13 KiB 853.38 KiB
ec06b2b 23.75 KiB 877.10 KiB 853.36 KiB
bab2c7f 23.75 KiB 877.17 KiB 853.42 KiB
54477e0 23.74 KiB 877.01 KiB 853.27 KiB
b06498a 23.75 KiB 877.17 KiB 853.42 KiB

@itaybre itaybre marked this pull request as ready for review July 3, 2025 11:41
philipphofmann
philipphofmann reviewed Jul 4, 2025
View reviewed changes
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.

Almost, LGTM. @itaybre can you please also test this with a build from TestFlight or on a real device and double-check if the user now appears in watchdog termination events. The iOS-Swift sample app has a button called OOM crash, that you can use.

philprime
philprime reviewed Jul 4, 2025
View reviewed changes
Copy link
Member

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

Good progress! Left a couple of comments.

@itaybre
Copy link
Contributor Author

itaybre commented Jul 4, 2025

Almost, LGTM. @itaybre can you please also test this with a build from TestFlight or on a real device and double-check if the user now appears in watchdog termination events. The iOS-Swift sample app has a button called OOM crash, that you can use.

Confirmed working
image

philipphofmann
philipphofmann approved these changes Jul 7, 2025
View reviewed changes
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.

LGTM, but maybe it makes sense to also wait for the approval of @philprime, cause he came up with the initial new logic for the scope observer.

philprime
philprime approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

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

Left my feedback in the comments, will fix the workaround in a follow-up PR. Proceed at your discretion.

@itaybre itaybre merged commit ccf1278 into main Jul 7, 2025
125 of 126 checks passed
@itaybre itaybre deleted the itay/cocoa-412-add-missing-information-to-watchdog-termination-events-unified branch July 7, 2025 16:14
This was referenced Jul 7, 2025
Closed
Closed
Closed
This was referenced Jul 7, 2025
Closed
Closed
Closed
Closed
This was referenced Jul 9, 2025
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipphofmann philipphofmann philipphofmann approved these changes

@philprime philprime philprime approved these changes

@armcknight armcknight Awaiting requested review from armcknight

@noahsmartin noahsmartin Awaiting requested review from noahsmartin noahsmartin is a code owner

@trevor-e trevor-e Awaiting requested review from trevor-e

@NicoHinderling NicoHinderling Awaiting requested review from NicoHinderling

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@itaybre @philipphofmann @philprime