-
-
Notifications
You must be signed in to change notification settings - Fork 376
Convert SentryCrashIntegration to Swift #7187
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?
Convert SentryCrashIntegration to Swift #7187
Conversation
…tionTracker to Swift Migrate the Logic and Tracker classes from Objective-C to Swift while keeping the Integration in Objective-C for now. The Swift classes are marked with @objc and @_spi(Private) to maintain compatibility with the existing Objective-C Integration. Changes: - Add SentryWatchdogTerminationLogic.swift with @objc support - Add SentryWatchdogTerminationTracker.swift with @objc support - Remove old Objective-C implementations - Update imports in Integration and other files to use Swift versions - Update test data to reference Swift class properties - Update bridging header to remove obsolete imports
Migrate the Integration class from Objective-C to Swift, building on top of the previously migrated Logic and Tracker Swift classes. The Swift Integration uses a protocol-based dependency injection pattern for better testability. Changes: - Add SentryWatchdogTerminationTrackingIntegration.swift with generics - Remove old Objective-C Integration implementation - Update Integrations.swift to register the Swift integration - Update SentrySDKInternal.m to remove ObjC registration - Update Integration tests to work with new Swift implementation
Update the scope observer API to be more Swift-friendly. Rename addObserver to addScopeObserver and make it accept Any type with runtime validation, allowing Swift-defined protocols to work properly with Objective-C. Changes: - Rename addObserver: to addScopeObserver: in SentryScope - Update addScopeObserver to accept Any with runtime protocol checking - Remove addObserver from SentryScope+Private.h header - Add addScopeObserver to SentryScope+PrivateSwift.h - Update SentryCrashIntegration to use new API - Update tests to cast observer to Any when needed - Update Xcode project to include Integration Swift file
…Tests, SentryMetricKitEventTests, and TestData. This simplifies the test files by eliminating unnecessary preprocessor directives.
- Create SentryCrashIntegration.swift with SwiftIntegration protocol - Implement dependency injection via CrashIntegrationProvider - Handle C callback for crash-time transaction saving - Remove old Objective-C files (SentryCrashIntegration.m/h) - Update SentrySDKInternal.m to remove ObjC integration reference - Register integration in Swift integration installer - Expose required ObjC headers in SentryPrivate.h - Update all tests to use new Swift initialization API - Add MIGRATION_NOTES.md documenting remaining work Note: Requires SentryWatchdogTerminationLogic, SentryCrashIntegrationSessionHandler, and SentryCrashScopeObserver to be properly bridged to Swift or migrated. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
…2-refactor-sentrycrashintegrationm-in-swift # Conflicts: # Sources/Sentry/SentryCrashIntegration.m
…chdog-integration-to-swift
…chdog-integration-to-swift
…atchdogTerminationTrackingIntegration.swift
…refactor-sentrycrashintegrationm-in-swift
- Remove SentryCrashIntegration.h import from SentryClient.m - Remove SentryCrashIntegration headers from SentryTests-Bridging-Header.h - Remove SentryWatchdogTerminationLogic.h from SentryPrivate.h (now in Swift) - Fix Scope casting to access private ObjC methods Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
The protocol is already defined in SentryDependencyContainer.swift from the watchdog termination branch merge. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
…tor-sentrycrashintegrationm-in-swift
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| dbf22c2 | 1214.19 ms | 1241.67 ms | 27.48 ms |
| 17b7e88 | 1223.04 ms | 1253.48 ms | 30.43 ms |
| 24ac927 | 1218.04 ms | 1242.02 ms | 23.98 ms |
| dbfeb41 | 1215.17 ms | 1237.41 ms | 22.23 ms |
| 778dadf | 1207.69 ms | 1246.09 ms | 38.40 ms |
| db9e223 | 1193.69 ms | 1213.56 ms | 19.87 ms |
| bb418da | 1227.60 ms | 1265.90 ms | 38.30 ms |
| f84c826 | 1216.38 ms | 1241.98 ms | 25.60 ms |
| 93ef486 | 1220.22 ms | 1244.96 ms | 24.74 ms |
| 3bf0d3f | 1202.12 ms | 1237.23 ms | 35.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| dbf22c2 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 17b7e88 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 24ac927 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| dbfeb41 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 778dadf | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| db9e223 | 24.14 KiB | 1.06 MiB | 1.03 MiB |
| bb418da | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| f84c826 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 93ef486 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 3bf0d3f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
Previous results on branch: itay/cocoa-992-refactor-sentrycrashintegrationm-in-swift
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 480baa9 | 1217.82 ms | 1236.80 ms | 18.98 ms |
| 8898ef5 | 1214.73 ms | 1254.80 ms | 40.07 ms |
| 84c0478 | 1224.17 ms | 1244.96 ms | 20.79 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 480baa9 | 24.14 KiB | 1.07 MiB | 1.04 MiB |
| 8898ef5 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 84c0478 | 24.14 KiB | 1.07 MiB | 1.04 MiB |
…ety and improve installation token handling. Update crash handler initialization logic and enhance comments for clarity.
philprime
left a comment
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.
Almost LGTM
| @objc public class func install(with options: Options) { | ||
| let dependencies = SentryDependencyContainer.sharedInstance() | ||
|
|
||
| // The order of integrations here is important. |
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.
l: We might want to organize this in a tree structure at some point. This would also make sure we do not have circular dependencies. I would not do this in this PR but maybe a follow-up issue makes sense?
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.
Created: #7202
Sources/Swift/Integrations/SentryCrash/SentryCrashIntegration.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SentryCrash/SentryCrashIntegration.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SentryCrash/SentryCrashIntegration.swift
Outdated
Show resolved
Hide resolved
…zation in SentryCrashIntegration
…ndency and simplify installation handling. Update tests to reflect changes in mock dependencies.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
…coa-992-refactor-sentrycrashintegrationm-in-swift
📜 Description
Converts
SentryCrashIntegrationto Swift.No user change: #skip-changelog
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.Closes #7195