Repository navigation
[#16044] Crashlytics Error Identity for Non-Fatals - #16045
yakovmanshin wants to merge 20 commits into
Conversation
* Introduced the `FIRCLSErrorInspector` utility class to describe the identity of non-fatal errors reported to Crashlytics * Updated `-[FIRCrashlytics recordError:userInfo:]` to report such descriptions with the additional user info * Moved `FIRCLSwiftFileUtility` into the new `SwiftUtilities` folder inside `Crashlytics`
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
morganchen12
left a comment
There was a problem hiding this comment.
Thanks for the reference implementation. I commented on one potential concern, otherwise this proposal and implementation (broadly) LGTM.
| } | ||
|
|
||
| let nsError = error as NSError | ||
| guard NSStringFromClass(type(of: nsError)).contains("__SwiftNativeNSError") else { |
There was a problem hiding this comment.
FYI, this may get flagged by Apple's private symbol scanner either now or in a future version of their scanner. We had a similar check for __SwiftValue in Analytics that was fine for several years and then caused App Store submission issues suddenly when Apple updated their scanner. Is there a way to implement this that doesn't require checking for the presence of a private class?
There was a problem hiding this comment.
My original implementation relied on a combination of isKind(of:) and isMember(of:), but it couldn’t distinguish between bridged Swift errors and subclasses of the real NSError (e.g. @interface MyError : NSError) because both are “kind of” but not “member of” NSError.
The best way I can think of to avoid issues with App Store’s static analysis is to reduce granularity in the check: e.g. something more generic, like SwiftNative, seems unlikely to be on the watch list of private APIs.
* Changed `__SwiftNativeNSError` to `SwiftNative` in `ErrorInspector.identityDescription(for:)` to avoid potential issues with static analysis * Introduced a test for the known edge case
|
The error in podspec validation should be fixed by ce52129, but I’m still not sure what’s causing Catalyst tests to fail: |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an ErrorInspector utility to provide standardized error identity descriptions and integrates it into FIRCrashlytics. Feedback identifies a critical missing nil check that could cause a runtime crash and suggests using more idiomatic Objective-C syntax for dictionary manipulation.
|
Podspec validation now passes, and the Catalyst job failed for some unrelated reason: Just in case, I merged the latest commits from @morganchen12 Could you please start the CI workflow? Thanks! |
|
I restarted CI. The catalyst job can be flaky. |
|
Seems that all checks are fine now 🤞🏻 |
morganchen12
left a comment
There was a problem hiding this comment.
LGTM with one nit. Adding someone from Crashlytics to approve as well.
* Tuple as an enum case’s associated value * `NSError` conforming to `CustomNSError`
|
@yakovmanshin Thanks for the FR and proposed implementation. Based on looking at it, I'm not sure adding an additional key Some potential changes:
However that's a larger effort, and would need more approvals to merge it into |
|
@tejasd Thanks for the review! My proposed implementation is a conservative one. It avoids modifying any existing data—but at the cost of reporting an extra key with every error. While it doesn’t fix error grouping automatically, it makes it possible to eventually update the backend to use the “error identity” as the new basis for grouping errors (once this concept is proven stable and consistent, as you mention in your second point). Limiting the scope to only Swift errors (maybe even only Swift enums) is an easy tweak in the implementation. But to make the update helpful at scale, it would require branching in the backend logic: enums should be grouped based on one key, and all other errors, based on the old Alternative approach (which I don’t like)It’s a way larger project than even the proposed implementation. It streamlines the logic and reported errors’ format, avoids sending redundant data, and doesn’t require complex changes on the backend. But it basically comes down to switching away from Conceptually, it would work like this:
It’s a much bigger and riskier change, and frankly, I don’t like it. That’s why I proposed this conservative approach. |
Makes sense, thanks. If you can update this PR to add the For the clustering changes, like you mentioned this would be a larger effort - and we'll keep that in mind if we receive more feedback on the feature request. |
* Updated `ErrorInspector` to return `nil` for true `NSError`s since we’re only interested in the identity of errors declared as Swift types * Renamed the `userInfo` key to `swift-error` * Updated tests * Updated documentation
|
@tejasd Could you have another look at this PR? Thanks! |


FIRCLSErrorInspectorutility class to describe the identity of non-fatal errors reported to Crashlytics-[FIRCrashlytics recordError:userInfo:]to report such descriptions with the additional user infoFIRCLSwiftFileUtilityinto the newSwiftUtilitiesfolder insideCrashlyticsSee the detailed proposal in #16044.