-
Notifications
You must be signed in to change notification settings - Fork 68
feat: add crash reporting #417
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: feat/error-tracking
Are you sure you want to change the base?
Conversation
can we create an issue for this and add to the limitation section in the docs once we have one? |
do we also need to patch PLCrashReporter for this? have other folks figured this out? otherwise same as #417 (comment) |
| s.frameworks = 'Foundation' | ||
|
|
||
| # PLCrashReporter dependency (not available on watchOS) | ||
| # Using ~> 1.8 for minimum compatibility with host apps |
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.
any reason to choose this version specifically?
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.
1.8.0 is the earliest version that supports saving custom data along with the crash report (PLCrashReporter.customData), which we use to persist context at crash time so we can accurately rebuild all the event properties on subsequent app launches. So minimum version that has the features we need
|
|
||
| // Add crash metadata | ||
| if let uuidRef = report.uuidRef { | ||
| properties["$crash_report_id"] = CFUUIDCreateString(nil, uuidRef) as String |
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.
what should we do with this info? should we add this to taxonomy?
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.
This is a uuid that PLCrashReport generates. It's crash metadata, so I figured we wouldn't want to lose it and it won't hurt including it, even though tbh I can't think of any uses of it. Maybe deduplication of reports BE side but that's highly unlikely since we delete the crash report once processed. I'm okay removing this
| "mach": [ | ||
| "exception": machException.type, | ||
| "code": machException.codes.first, | ||
| "subcode": machException.codes.count > 1 ? machException.codes[1] : nil, |
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.
lets skip nil values
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.
nil values are removed with .compactMapValues { $0 } below, for both "meta" and "mach" keys. Not super efficient but the number of keys is really small here so negligible I believe
| // MARK: - Helpers | ||
|
|
||
| /// Format string for zero-padded 64-bit hex addresses (e.g., "0x00007fff12345678") | ||
| static let hexAddressPaddedFormat = "0x%016llx" |
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.
make it private
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.
done
| ] | ||
|
|
||
| private static func machExceptionName(_ type: UInt64) -> String { | ||
| machExceptionNames[type] ?? "EXC_UNKNOWN(\(type))" |
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.
probably better with EXC_UNKNOWN_X instead of EXC_UNKNOWN(X), more readable
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.
agreed, done
|
|
||
| import Foundation | ||
|
|
||
| #if os(iOS) || os(macOS) || os(tvOS) |
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.
is the manual capture also available for the 3 OSs?
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.
No, manual capture is available on watchOS as well.
For this class, we have fallback stub implementation for watchOS, which I think works well. To be consistent on the public API, however, I'll mark this config unavailable for watchOS
| func install(_ postHog: PostHogSDK) throws { | ||
| try PostHogCrashReporterIntegration.integrationInstalledLock.withLock { | ||
| if PostHogCrashReporterIntegration.integrationInstalled { | ||
| throw InternalPostHogError(description: "Crash report integration already installed to another PostHogSDK instance.") |
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.
we can just log and bail out isntead of throwing, this would be more expensive at runtime
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.
This is a pattern we use for other integrations as well. We catch on install step and log in PostHogSDK. We could change the signature of this method to return a truthy for success or failure instead, but I'll skip for this PR to keep the scope down
| /// as the debugger intercepts signals before the crash handler can process them. | ||
| /// | ||
| /// Default: false | ||
| @objc public var enableCrashHandler: Bool = false |
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.
i think we call this just autocapture on flutter? i think we should unify naming if that 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.
flutter uses captureX for different error types, but Android and RN do use autoCapture.
Updated config to autoCapture and integration to PostHogErrorTrackingAutoCaptureIntegration for consistency
| /// - Parameters: | ||
| /// - skipBuildProperties: When true, skips buildProperties call and uses properties as-is. | ||
| /// Used by crash reporting to capture events with pre-built crash-time context. | ||
| func captureInternal( |
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.
this seems to be a public API now, how can we make this internal only for us? we should not expose this to customers
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.
This is an internal method that accepts an additional shouldBuildProperties: Bool parameter (used internally when capturing an exception event). I think the public API remains unchanged
# Conflicts: # PostHog.xcodeproj/project.pbxproj # PostHog/ErrorTracking/Models/PostHogBinaryImageInfo.swift # PostHog/ErrorTracking/PostHogErrorTrackingConfig.swift # PostHog/ErrorTracking/PostHogExceptionProcessor.swift # PostHog/ErrorTracking/Utils/PostHogDebugImageProvider.swift # PostHog/PostHogSDK.swift # PostHogExample/ContentView.swift # PostHogExample/ExceptionHandler.h # PostHogExample/ExceptionHandler.m
|
Build is green locally but fails in CI, looking into it |
💡 Motivation and Context
Stacked on top of #404
#skip-changelog
Adds automatic crash reporting to the PostHog iOS SDK using
PLCrashReporter.Crashes are captured and sent as
$exceptionevents with level "fatal" on the next app launch.Crash Types Handled:
Implementation notes
Limitations
💚 How did you test it?
📝 Checklist