search

firebase / flutterfire

🔥 A collection of Firebase plugins for Flutter apps.
https://firebase.google.com/docs/flutter/setup
BSD 3-Clause "New" or "Revised" License
8.46k stars 3.91k forks source link

[firebase_crashlytics] Migrate to new Crashlytics SDK #2288

Closed axel-op closed 3 years ago

axel-op commented 4 years ago

Description

This PR updates the Crashlytics implementation to use the new SDK, which doesn't use the Fabric SDK anymore: https://firebase.google.com/docs/crashlytics/get-started-new-sdk.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

MaikuB commented 4 years ago

@axel-op I believe the title of the PR needs amending to start with [firebase_crashlytics] instead of [crashlytics] as it's the name of the plugin that should be used

axel-op commented 4 years ago

@MaikuB Thanks, it's done. What do you think of the PR?

MaikuB commented 4 years ago

@axel-op I'm not familiar with the APIs to comment. Came across your PR as another member in the community mentioned it :)

axel-op commented 4 years ago

I'll definitely need help for the iOs part.

workerbee22 commented 4 years ago

What do we need to do to get this moving ... it's already very urgent.

axel-op commented 4 years ago

I have to update the README file

workerbee22 commented 4 years ago

Wow ... @axel-op great progress 🎉🎉🎉

axel-op commented 4 years ago

Looks like they have moved to beta4 as per the official doco here

Thanks @workerbee22 for pointing that out :)

axel-op commented 4 years ago

@workerbee22 and @creativecreatorormaybenot is everything OK for you?

creativecreatorormaybenot commented 4 years ago

It seems as though my feedback has been addressed 👍🏼
Need some testing now to confirm that everything works as expected and a maintainer to take a look at this.

axel-op commented 4 years ago

Well, I was just asking for your opinion, since I knew you would answer much faster ;)

axel-op commented 4 years ago

The new SDK is out of beta

googlebot commented 4 years ago

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

axel-op commented 4 years ago

@MaikuB I added you as a co-author, are you OK with this?

MaikuB commented 4 years ago

@axel-op wasn't necessary but sure :)

MaikuB commented 4 years ago

@googlebot I consent.

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

MaikuB commented 4 years ago

Fyi I've mentioned this PR the official discord but not sure if that'll help speed things along in getting this looked at as I didn't get a response

workerbee22 commented 4 years ago

@axel-op Looks like everything is done on this PR, besides those failing checks. Do you need help with anything else to get this to 'done'? Btw. Where is the process for contributing changes to the code base documented? Or is it just the contributing guide here?

axel-op commented 4 years ago

@workerbee22 We now have to wait for the review of @collinjackson or @kroikie

jimexist commented 4 years ago

@axel-op Looks like everything is done on this PR, besides those failing checks. Do you need help with anything else to get this to 'done'? Btw. Where is the process for contributing changes to the code base documented? Or is it just the contributing guide here?

These failing tests should be resolved once rebased onto latest master i think?

axel-op commented 4 years ago

@Jimexist Thanks.

Does anyone know how to fix this last failing check?

jimexist commented 4 years ago

@Jimexist Thanks.

Does anyone know how to fix this last failing check?

seems https://github.com/FirebaseExtended/flutterfire/pull/2288/checks?check_run_id=667329313 is a TLS error not related to your change. I'd re-base again and retry the CI and see if it goes away.

workerbee22 commented 4 years ago

Awesome guys 👍So we're back to waiting for the review of @collinjackson or @kroikie ?

@axel-op is it worth doing another quick test with the new v1.17.1 ?

axel-op commented 4 years ago

@workerbee22 What kind of test are you talking about? Correct me if I'm wrong, but this new version of Flutter doesn't seem to affect any API used by this plugin.

workerbee22 commented 4 years ago

You could be right .... but I don't know. Thus a quick integration test on v1.17.1 is prudent? For example firebase_analytics seemed fine on v1.17 ... until someone tested it. Thus we got v1.17.1 and sure is wasn't a firebase_analytcis issues (it was a Flutter framework issue) but would be nice to know.

axel-op commented 4 years ago

I've tested it with Flutter 1.17.1 on Android, everything works fine and is reported to the console (stacktrace, logs, keys, ids...). I haven't tested it on iOs yet with this version of Flutter, I'll try to do it soon.

EDIT: It's done, I'm just waiting for the data to appear in the console.

axel-op commented 4 years ago

@Moncader Do you know why this decision has been made in the first place to not send logs as soon as they're declared?

Moncader commented 4 years ago

@axel-op Looks like it was suggested in the original PR for the first version of this plugin.

https://github.com/flutter/plugins/pull/1080#issuecomment-469207430

Which was taken in to consideration https://github.com/flutter/plugins/pull/1080#issuecomment-472636009

Which was implemented... https://github.com/flutter/plugins/pull/1080/commits/701f2d1cc214a5119a9d79f441681210fef73eba#diff-a108e5652361ec613549c31238b30cf1

And subsequently wiped out https://github.com/flutter/plugins/pull/1080/commits/e3f820985ab4004ce38bf1bdc655839c977f7a9d

I don't see any community discussion regarding that and I think it was just forgotten.

@kroikie Since you authored this originally do you have any input on this?

axel-op commented 4 years ago

I also have a suggestion, about key/value methods.

With the new SDK, "setBool, setDouble, setFloat, setInt, setLong, and setString are aggregated into setCustomKey". For now I just replaced these methods calls by setCustomKey on the native side.

Since the SDK converts every value set with this method into a string before sending it, shouldn't we replace the Dart methods setInt, setBool... by a single one (e.g. setKey) that would accept a dynamic value, and do the string conversion by calling toString before sending this string to the native side?

Then we wouldn't have to do all these type checkings, and the Dart implementation would be closer to the native one.

axel-op commented 4 years ago

Guess I'll have to rebase again... Task failing for no apparent reason

workerbee22 commented 4 years ago

@axel-op Keep up the great work 👍Nearly there and lots of us out here are cheering you on 😁

googlebot commented 4 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot commented 4 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

axel-op commented 4 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@Moncader

Moncader commented 4 years ago

@googlebot I fixed it.

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

workerbee22 commented 4 years ago

Given the NEW FlutterFire Roadmap

This fantastic PR by @axel-op which seems very close to being ready, I think should be given some attention sooner rather than later 👍

Ehesp commented 4 years ago

Before we can merge, we need to get the overall tests working together (current in progress). There's some underlying changes we're making to versioning so we'll need to make sure all plugins can work together.

axel-op commented 4 years ago

For the iOS developers: step 2 of the installation page for iOS isn't reported on the README, but I think it should... What do you all think?

MaikuB commented 4 years ago

@axel-op what makes you think so? Isn't that step already handled by the plugin? The Firebase pods should end up being installed as a transitive dependency of sorts by apps that use this plugin since the plugin depends on them. If you inspect the Podfile.lock file for an app, you should see that they've been installed

axel-op commented 4 years ago

Whether this should be fixed now or not is up for debate, but since this is a breaking change PR, I'd like to suggest changing how the custom keys and logs work.

Currently in this plugin, keys and logs are only actually set in native Crashlytics when an error actually occurs.

This has a few problems:

  1. If the app crashes, no logs or custom keys have or can be set by dart code and therefore all logs of keys that should have been set by the user are not and lost.
  2. The timestamp information for logs is 100% incorrect and all logs have the same timestamp as the error itself.

In a production environment the logs are mostly useless without correct timestamp information and keys aren't even set for tracking crashes which make those quite unusable as well.

I'd like to propose we change it so that logging is logged directly to Crashlytics on the spot, and that a custom keys are not passed with the onError method call but rather as a separate and new API like setCustomKey (just mirror the native API)

I'm marking this PR as draft until I make these changes.

aguitto commented 4 years ago

Could be directly connected with TimeoutException issue ? https://github.com/FirebaseExtended/flutterfire/issues/50

Could you please accept this asap ?

axel-op commented 4 years ago

@aguitto What makes you think this PR is related to this issue?

axel-op commented 4 years ago

@axel-op what makes you think so? Isn't that step already handled by the plugin? The Firebase pods should end up being installed as a transitive dependency of sorts by apps that use this plugin since the plugin depends on them. If you inspect the Podfile.lock file for an app, you should see that they've been installed

@MaikuB In #2379, someone wrote:

I just spend few hour trying to make CrashLytics work on iOS. Between the officiel CrashLytics docs and this plugin Readme, both are partially wrong or not enough (at least in my case, which should be any Flutter dev's).

When you add a new iOS App on Firebase, you're asked to :

  1. Download a .plist file and copy inside your project. => In Flutter's case, it should be in the Runner folder
  2. Add some lines to the Podfile. => In my case this would cause error. Ignoring this totally resolve the issue.
  3. Add some lines to the AppDelegate.swift => This was needed.

After, you need to install firebase_crashlytics.

  1. Add line to pubspec.yaml
  2. Follow this plugin's README iOS instructions => This cause error too. Ignoring this totally resolve the issue.
MaikuB commented 4 years ago

@axel-op I'm skeptical about that as all that should already be handled and a lot of plugins that wrap around native SDKs work in a similar way without issues. I would think there's something else going on there that I wouldn't read too much into it unless a complete sample app can be provided to demonstrate it's needed. For what it's worth I work on an app that hasn't needed those steps and would expect it to be a more widespread issue if it was needed

axel-op commented 4 years ago

@MaikuB Also while testing I had to update my AppDelegate file like so. But I'm really not an expert in iOS development, so I'd really like to have a confirmation from an iOS developer.

axel-op commented 4 years ago

@axel-op I'm skeptical about that as all that should already be handled and a lot of plugins that wrap around native SDKs work in a similar way without issues. I would think there's something else going on there that I wouldn't read too much into it unless a complete sample app can be provided to demonstrate it's needed. For what it's worth I work on an app that hasn't needed those steps and would expect it to be a more widespread issue if it was needed

Noted 👍

MaikuB commented 4 years ago

@MaikuB Also while testing I had to update my AppDelegate file like so. But I'm really not an expert in iOS development, so I'd really like to have a confirmation from an iOS developer.

If that's the case then it worth debugging the plugin further as it could mean that this isn't being called.

One alternative is to have the plugin register to receive AppDelegate calls and call configure like you currently do in the example app's AppDelegate.swift file. This would (1) remove the need to have developers do it themselves and (2) more closely mirror the setup required by a native iOS app. An example of how to have the plugin register to receive these calls can be found here. Not sure if there was a reason why it wasn't originally done this way though

axel-op commented 4 years ago

@MaikuB Thanks! I'm not sure I should make these changes right now, as I believe the current rework by the people from Invertase will probably bring some important changes to the native part of all Firebase plugins, as this comment suggests.