feat(firebase_crashlytics): rework #3420

Salakar · 2020-09-01T08:55:56Z

Co-authored-by: @MaikuB
Co-authored-by: @Moncader
Co-authored-by: @axel-op
Co-authored-by: @Ehesp
Co-authored-by: @Salakar
Co-authored-by: @greghesp
Co-authored-by: @kirstywilliams
Co-authored-by: @helenaford

Description

For testing purposes, firebase_crashlytics version 0.2.0-dev.4 is published with these changes.

Changes

BREAKING: Removal of Fabric SDKs and migration to the new Firebase Crashlytics SDK.
BREAKING: The following methods have been removed as they are no longer available on the Firebase Crashlytics SDK:
- setUserEmail
- setUserName
- getVersion
- isDebuggable
BREAKING: log now returns a Future. Calling log now sends logs immediately to the underlying Crashlytics SDK instead of pooling them in Dart.
BREAKING: the methods setInt, setDouble, setString and setBool have been replaced by setCustomKey.
- setCustomKey returns a Future. Calling setCustomKey now sends custom keys immediately to the underlying Crashlytics SDK instead of pooling them in Dart.
DEPRECATED: enableInDevMode has been deprecated, use isCrashlyticsCollectionEnabled and setCrashlyticsCollectionEnabled instead.
DEPRECATED: Crashlytics has been deprecated, use FirebaseCrashlytics instead.
NEW: Custom keys that are automatically added by FlutterFire when calling reportError are now prefixed with flutter_error_.
NEW: Calling .crash() on Android & iOS/macOS now reports a custom named exception to the Firebase Console. This allows you to easily locate test crashes.
- Name: FirebaseCrashlyticsTestCrash.
- Message: This is a test crash caused by calling .crash() in Dart..
NEW: recordError now uses a named native exception when reporting to the Firebase Console. This allows you to easily locate errors originating from Flutter.
- Name: FlutterError.
NEW: Added support for checkForUnsentReports.
- Checks a device for any fatal or non-fatal crash reports that haven't yet been sent to Crashlytics.
- See reference API docs for more information.
NEW: Added support for deleteUnsentReports.
- If automatic data collection is disabled, this method queues up all the reports on a device for deletion.
- See reference API docs for more information.
NEW: Added support for didCrashOnPreviousExecution.
- Checks whether the app crashed on its previous run.
- See reference API docs for more information.
NEW: Added support for sendUnsentReports.
- If automatic data collection is disabled, this method queues up all the reports on a device to send to Crashlytics.
- See reference API docs for more information.
NEW: Added support for setCrashlyticsCollectionEnabled.
- Enables/disables automatic data collection by Crashlytics.
- See reference API docs for more information.
NEW: Added support for isCrashlyticsCollectionEnabled.
- Whether the current Crashlytics instance is collecting reports. If false, then no crash reporting data is sent to Firebase.
- See reference API docs for more information.
FIX: Fixed a bug that prevented keys from being set on iOS devices.

Related Issues

Supersedes #2288

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?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

Co-authored-by: Michael Bui <[email protected]> Co-authored-by: Jason Parrott <[email protected]> Co-authored-by: Axel Ogereau-Peltier <[email protected]> Co-authored-by: Salakar <[email protected]> Co-authored-by: Greg Hesp <[email protected]> Co-authored-by: Kirsty Williams <[email protected]> Co-authored-by: Helena Ford <[email protected]>

Salakar · 2020-09-01T08:58:08Z

Tagging in everyone from the previous PR for a final review pass:

Salakar · 2020-09-01T08:58:44Z

If it helps testing this out before we merge, I can publish a dev release. Let me know.

HofmannZ · 2020-09-01T09:04:33Z

@Salakar a dev release would help ✌🏼

Salakar · 2020-09-01T09:30:22Z

@HofmannZ 0.2.0-dev.1 is up

HofmannZ · 2020-09-01T09:56:55Z

@Salakar it seems like enableInDevMode on is not exposed on FirebaseCrashlytics.instance. Therfore FirebaseCrashlytics.instance.enableInDevMode = true; from the useage docs does not work.

HofmannZ · 2020-09-01T10:39:24Z

Android works fine, having some issues with iOS pods. Let me check if that's me or the package.

HofmannZ · 2020-09-01T10:54:07Z

Ok, resolved the pods issue by cleaning the pods: rm -rf ios/Pods && rm ios/Podfile && rm ios/Podfile.lock.

But running into the following issue during iOS the build:

duplicate symbol '_kArgumentCode' in:
    /Users/zinohofmann/Library/Developer/Xcode/DerivedData/Runner-aefewpetgecaciadgvxxfimlevof/Build/Products/Debug-dev-iphonesimulator/firebase_auth/firebase_auth.framework/firebase_auth(FLTFirebaseAuthPlugin.o)
    /Users/zinohofmann/Library/Developer/Xcode/DerivedData/Runner-aefewpetgecaciadgvxxfimlevof/Build/Products/Debug-dev-iphonesimulator/firebase_crashlytics/firebase_crashlytics.framework/firebase_crashlytics(FLTFirebaseCrashlyticsPlugin.o)
ld: 1 duplicate symbol for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Seems like both firebase_auth and firebase_crashlytics define _kArgumentCode.

Salakar · 2020-09-01T10:54:57Z

Interesting, I know what that is, will push a fix shortly

…rk as deprecated)

HofmannZ · 2020-09-01T19:09:54Z

Probably worth checking these logs on your Mac to see if it had any errors uploading. We could document this also. But ye if your build container exits early then we should probably document to use upload-symbols instead, we could do this based on if CI env exists then use upload-symbols, otherwise run as normal?

@Salakar I think that would indeed be the long term solution. For now, we should al least mention that if your building with a CI, you should use ${PODS_ROOT}/FirebaseCrashlytics/upload-symbols --build-phase instead of ${PODS_ROOT}/FirebaseCrashlytics/run.

krokyze · 2020-09-01T21:38:23Z

Tried using ${PODS_ROOT}/FirebaseCrashlytics/upload-symbols --build-phase and it breaks debug builds for me.

Build prints out bunch of warnings about pods and ends with:

upload-symbols[40295:224829] Unable to get file attributes for dSYM file at path
"/project/build/ios/Debug-iphoneos/Runner.app.dSYM/Contents/Resources/DWARF/Runner"
Running upload-symbols in Build Phase mode
Validating build environment for Crashlytics...
warning: DEBUG_INFORMATION_FORMAT should be set to dwarf-with-dsym for all configurations. This could also be a timing issue, make sure the Fabric run script build phase is the last build phase and no other scripts have moved the dSYM from the location Xcode generated it. Unable to process Runner.app.dSYM at path /project/build/ios/Debug-iphoneos/Runner.app.dSYM
Make sure your project build settings are generating a dSYM file.

Fetching upload-symbols settings...
Sending Crashlytics build event...
Processing dSYMs...
Command PhaseScriptExecution failed with a nonzero exit code
note: Using new build system
note: Building targets in parallel
note: Planning build
note: Constructing build description

Could not build the precompiled application for the device.

Salakar · 2020-09-02T10:51:53Z

Hey all, 0.2.0-dev.3 is up with the following fixes:

Fixed an issue with iOS logs appearing as null on the Firebase Console.
Ensure cosmetic Android exception classes keep their name when app obfuscated.
Fixed an error when hot restarting on Android (🐛 [firebase_crashlytics: 0.2.0-dev.2] Hot restart fails #3432).

HofmannZ · 2020-09-02T11:25:31Z

@krokyze I've been digging into the issue a little bit. And it seems that the output for the background execution in Console.app actually always fails with that error. But since it's running in the background, you don't see the error.

The root of the problem lies in the fact that Flutter, in debug mode, does not generate dSYM files. If we move that out of the background the debug builds fail.

One solution would be to find a way to check for debug mode in the build phase. As for now, I have no idea how to do that. Any suggestions are welcome :)

krokyze · 2020-09-02T11:37:53Z

@HofmannZ I was thinking the same about checking if it's debug mode, not sure if it's possible. The more I searched for solution, the more I started to think this is not really issue related with this package, but CI and FirebaseCrashlytics issue fastlane/fastlane#13096

Salakar · 2020-09-02T11:38:03Z

@krokyze I've been digging into the issue a little bit. And it seems that the output for the background execution in Console.app actually always fails with that error. But since it's running in the background, you don't see the error.

The root of the problem lies in the fact that Flutter, in debug mode, does not generate dSYM files. If we move that out of the background the debug builds fail.

One solution would be to find a way to check for debug mode in the build phase. As for now, I have no idea how to do that. Any suggestions are welcome :)

You could do something like the following to limit to release builds only:

if [ "${CONFIGURATION}" = "Release" ]; then
  ${PODS_ROOT}/FirebaseCrashlytics/upload-symbols --build-phase
fi

HofmannZ · 2020-09-02T11:50:07Z

@HofmannZ I was thinking the same about checking if it's debug mode, not sure if it's possible. The more I searched for solution, the more I started to think this is not really issue related with this package, but CI and FirebaseCrashlytics issue fastlane/fastlane#13096

Well, with a native iOS app, you would use dwarf-with-dsym for all configurations. And therefore the upload would be all fine. Our specific issue comes from the fact that Flutter does not enable dwarf-with-dsym for the Debug configuration by default. One option would be to enable it for debug mode ourselves. But I think the solution pointed out by @Salakar might actually to the job as well and only requires two additional lines. In addition, I have no idea what the effect of enabling dwarf-with-dsym for the Debug would have on the live-reload and other Flutter internals.

Going to test it 🧪

Salakar · 2020-09-02T11:52:45Z

Final script I've come up locally with, that will check if CI (should support all CIs) and whether is release mode:

IS_CI="${CI}${CONTINUOUS_INTEGRATION}${BUILD_NUMBER}${RUN_ID}"
if [[ ("${CONFIGURATION}" == "Release" && (-n "${IS_CI}")) ]]; then
  ${PODS_ROOT}/FirebaseCrashlytics/upload-symbols --build-phase
else
  ${PODS_ROOT}/FirebaseCrashlytics/run
fi

HofmannZ · 2020-09-02T12:12:04Z

Final script I've come up locally with, that will check if CI (should support all CIs) and whether is release mode:

IS_CI="${CI}${CONTINUOUS_INTEGRATION}${BUILD_NUMBER}${RUN_ID}"
if [[ ("${CONFIGURATION}" == "Release" && (-n "${IS_CI}")) ]]; then
  ${PODS_ROOT}/FirebaseCrashlytics/upload-symbols --build-phase
else
  ${PODS_ROOT}/FirebaseCrashlytics/run
fi

Don't we only want to run any of the scripts if in release mode, and then pick the script based on if you are in CI?

Salakar · 2020-09-02T15:01:22Z

I think everything is generally good for a merge here, so I'll go ahead and do one final dev release off this branch and then merge.

We should do some follow up PRs for things like docs (e.g. the run script in CI notes from above) - would welcome contributions for this if anyone is willing.

We'll keep it on dev release for a couple of weeks.

Ehesp and others added 7 commits July 30, 2020 13:38

Merge pull request #47 from FirebaseExtended/master

aff7197

Merge pull request #48 from FirebaseExtended/master

0e98060

Merge pull request #54 from FirebaseExtended/master

58c19bf

Merge branch 'master' of https://github.com/FirebaseExtended/flutterfire

2f06ff1

Merge branch 'master' of https://github.com/FirebaseExtended/flutterfire

bf8339c

Merge branch 'master' of https://github.com/FirebaseExtended/flutterfire

90b3f47

googlebot added the cla: yes label Sep 1, 2020

Salakar self-assigned this Sep 1, 2020

Salakar added the plugin: crashlytics label Sep 1, 2020

Salakar requested review from Ehesp, helenaford and kirstywilliams September 1, 2020 08:59

Salakar mentioned this pull request Sep 1, 2020

[firebase_crashlytics] Migrate to new Crashlytics SDK #2288

Closed

13 tasks

This comment has been minimized.

Sign in to view

Salakar added 2 commits September 1, 2020 11:57

fix: add backwards compatible enableInDevMode getter/setter (but ma…

d42f9fc

…rk as deprecated)

docs: remove enabledInDevMode docs

4b3e109

This comment has been minimized.

Sign in to view

MaikuB and others added 5 commits September 2, 2020 10:54

refactor: update example app to use FutureBuilder (#68)

469a680

fix: fix ios logging

ca49ab5

fix(android): keep cosmetic exception class names when obfuscated

50798cd

chore: bump version to dev.3

dd80487

fix(android): fixed an error when hot restarting (fixes #3432)

d4c7dfc

Salakar added 5 commits September 2, 2020 13:30

fix(ios): hot restart issue on ios (#3432)

766406a

chore: bump version to dev.4

714ba2a

refactor(example): update android example to flutterEmbedding v2

26e7ecc

refactor: fix formatting

d3f08d7

docs(crashlytics): finalize changelog and add migration guide

3a7061f

Salakar marked this pull request as ready for review September 2, 2020 14:36

Salakar requested a review from kroikie as a code owner September 2, 2020 14:36

Salakar added 3 commits September 2, 2020 15:40

docs: fix -> FIX

6543922

docs: fix -> FIX

dc921ee

docs: capitalize change type on changelog

dfa7f33

chore: bump version to dev.5

af1ac2f

Salakar changed the title ~~feat(firebase_crashlytics): v1 rework~~ feat(firebase_crashlytics): rework Sep 2, 2020

Salakar merged commit a084641 into firebase:master Sep 2, 2020

Salakar deleted the @invertase/crashlytics_v1 branch September 2, 2020 15:21

firebase locked and limited conversation to collaborators Oct 3, 2020

feat(firebase_crashlytics): rework #3420

feat(firebase_crashlytics): rework #3420

Uh oh!

Conversation

Salakar commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Related Issues

Checklist

Breaking Change

Uh oh!

Salakar commented Sep 1, 2020

Uh oh!

Salakar commented Sep 1, 2020

Uh oh!

HofmannZ commented Sep 1, 2020

Uh oh!

Salakar commented Sep 1, 2020

Uh oh!

This comment has been minimized.

HofmannZ commented Sep 1, 2020

Uh oh!

This comment has been minimized.

HofmannZ commented Sep 1, 2020

Uh oh!

HofmannZ commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Salakar commented Sep 1, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HofmannZ commented Sep 1, 2020

Uh oh!

krokyze commented Sep 1, 2020

Uh oh!

Salakar commented Sep 2, 2020

Uh oh!

HofmannZ commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krokyze commented Sep 2, 2020

Uh oh!

Salakar commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HofmannZ commented Sep 2, 2020

Uh oh!

Salakar commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HofmannZ commented Sep 2, 2020

Uh oh!

Salakar commented Sep 2, 2020

Uh oh!

Uh oh!

Salakar commented Sep 1, 2020 •

edited

Loading

HofmannZ commented Sep 1, 2020 •

edited

Loading

HofmannZ commented Sep 2, 2020 •

edited

Loading

Salakar commented Sep 2, 2020 •

edited

Loading

Salakar commented Sep 2, 2020 •

edited

Loading