Skip to content
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

FR: Expose record error with additional user info #5496

Closed
reggian opened this issue Apr 28, 2020 · 7 comments
Closed

FR: Expose record error with additional user info #5496

reggian opened this issue Apr 28, 2020 · 7 comments

Comments

@reggian
Copy link

reggian commented Apr 28, 2020

Feature proposal

  • Firebase Component: Crashlytics

Currently there is only the -recordError: method exposed which calls the appropriate underlying implementation with additionalUserInfo dictionary set to nil.

- (void)recordError:(NSError *)error {
  FIRCLSUserLoggingRecordError(error, nil);
}

Is there a reason that there is no second method which passes the additionalUserInfo through? I.e.:

- (void)recordError:(NSError *)error withAdditionalUserInfo:(NSDictionary<NSString *, id> *)additionalUserInfo {
  FIRCLSUserLoggingRecordError(error, additionalUserInfo);
}
@liamnichols
Copy link

liamnichols commented Jul 6, 2020

I'm going through the migration guide and there is no explanation as to why it was removed. Is it welcomed if I submit a PR? (I noticed that the PR template says that you don't accept public API changes)

@liamnichols
Copy link

Just a note on this for the maintainers... The email we received around a month ago mentions that crash reporting will stop working using the deprecated SDK in November so we need to migrate to Firebase Crashlytics ASAP but this is hard if there is missing functionality from the replacement.

I raised a support ticket for this hoping that it gains a higher priority but please could you let us know if this will be addressed soon? I see it more as missing functionality than a feature request since we need this feature for tracking issues and we need to be able to provide support for older releases of our app and the longer that I hold off switching to FirebaseCrashlytics, the more more builds that will be out there that will stop reporting crashes in November.

I'll hold out a little longer but it would be great to get a documented response so that I can start on an alternative approach if this doesn't score a high enough priority to be tackled soon, cheers. 🙏

@mcontin
Copy link

mcontin commented Jul 10, 2020

I can't test this at the moment, but could this give the same results in the meanwhile?

let tempError = myError as NSError
let recordableError = NSError(domain: tempError.domain, code: tempError.code, userInfo: additionalUserInfo)
Crashlytics.crashlytics().record(error: recordableError)

@zachcristol-heb
Copy link

I can't test this at the moment, but could this give the same results in the meanwhile?

let tempError = myError as NSError
let recordableError = NSError(domain: tempError.domain, code: tempError.code, userInfo: additionalUserInfo)
Crashlytics.crashlytics().record(error: recordableError)

I am running into the same migration problem. Wouldn't you lose data about the original error myError, because you only record the domain, code, and additionalUserInfo, not myError itself

@mcontin
Copy link

mcontin commented Jul 11, 2020

I am running into the same migration problem. Wouldn't you lose data about the original error myError, because you only record the domain, code, and additionalUserInfo, not myError itself

The data you could lose is in tempError.userInfo, so you could merge that with additionalUserInfo.
Here it says we should append additional info in NSError.userInfo like so:

let userInfo = [
  NSLocalizedDescriptionKey: NSLocalizedString("The request failed.", comment: ""),
  NSLocalizedFailureReasonErrorKey: NSLocalizedString("The response returned a 404.", comment: ""),
  NSLocalizedRecoverySuggestionErrorKey: NSLocalizedString("Does this page exist?", comment: ""),
  "ProductID": "123456",
  "View": "MainView"
]

let error = NSError.init(domain: NSCocoaErrorDomain,
                         code: -1001,
                         userInfo: userInfo)

@liamnichols
Copy link

Thanks for pointing that out @mcontin & @zachcristol-heb, I think this would work in our use case so I just need to verify it but can't right this second due to the ongoing incident.

If this does prove to be successful, then maybe a good action point from this ticket for the Firebase team would be to include such instructions in the migration guide 🙏

@liamnichols
Copy link

I can confirm that the approach above works both for Fabric and Firebase SDKs, thanks!

@firebase firebase locked and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants