[firebase_crashlytics]Firebase Crashlytics plugin #1080

kroikie · 2019-01-15T19:23:39Z

Add plugin to FlutterFire that will report Dart side errors as non-fatal errors to the Firebase console.

Taormina · 2019-01-25T17:21:25Z

packages/firebase_crashlytics/README.md

+
+### iOS Integration
+
+Add the Crachlytics run scripts


Crashlytics

Thanks fixed.

...a/io/flutter/plugins/firebase/crashlytics/firebasecrashlytics/FirebaseCrashlyticsPlugin.java

collinjackson

Thanks for doing this!

collinjackson · 2019-02-21T13:55:06Z

packages/firebase_crashlytics/README.md

+Setup `Crashlytics`:
+```dart
+void main() {
+  // Set reportInDevMode to true to see reports while in debug mode


I would put backticks around reportInDevMode

collinjackson · 2019-02-21T13:55:58Z

packages/firebase_crashlytics/README.md

+void main() {
+  // Set reportInDevMode to true to see reports while in debug mode
+  // This is only to be used for confirming that reports are being
+  // submitted as expected. It is not intended to be used in production.


I would change "It is not intended to be used in production" to "It is not intended to be used for everyday development."

collinjackson · 2019-02-21T13:57:51Z

packages/firebase_crashlytics/example/lib/main.dart

+void main() {
+  // Set reportInDevMode to true to see reports while in debug mode
+  // This is only to be used for confirming that reports are being
+  // submitted as expected. It should not intended to be used in production.


Same feedback here, the "not intended to be used in production" comment confused me

collinjackson · 2019-02-21T13:57:59Z

packages/firebase_crashlytics/example/lib/main.dart

+import 'package:firebase_crashlytics/firebase_crashlytics.dart';
+
+void main() {
+  // Set reportInDevMode to true to see reports while in debug mode


packages/firebase_crashlytics/example/lib/main.dart

collinjackson · 2019-02-21T14:49:28Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+class Crashlytics {
+  static final Crashlytics instance = Crashlytics();
+
+  /// Set to true to have Errors sent to Crashlytics while in debug mode.


Don't capitalize errors. I would clarify that you mean Flutter debug mode. You might also say that it defaults to false.

collinjackson · 2019-02-21T14:49:49Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+  static final Crashlytics instance = Crashlytics();
+
+  /// Set to true to have Errors sent to Crashlytics while in debug mode.
+  bool reportInDevMode = false;


I would call it enableInDebugMode

collinjackson · 2019-02-21T14:51:36Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    throw StateError('Error thrown by Crashlytics plugin');
+  }
+
+  Future<bool> isDebuggable() async {


Needs dartdoc. Clarify that this means debuggability of the OEM parts of the app, not the Flutter app.

Used doc from Android, but will have to dig a bit deeper to get more info on what this means in both a Android and iOS context.

collinjackson · 2019-02-21T14:52:46Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+  /// Logs to be included with report.
+  @visibleForTesting
+  final ListQueue<String> logs = ListQueue<String>(15);
+  int logSize = 0;


This is private and should have an underscore

collinjackson · 2019-02-21T14:52:49Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+
+  /// Logs to be included with report.
+  @visibleForTesting
+  final ListQueue<String> logs = ListQueue<String>(15);


This is private and should have an underscore

If this has an underscore how will I use it in tests?

If it's not part of the public API, it's an implementation detail and shouldn't be used in tests.

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

packages/firebase_crashlytics/example/pubspec.yaml

hpoul · 2019-03-04T10:47:25Z

I got a bit eager waiting for this to get merged, so I migrating a project from flutter_crashlytics to this plugin by including the git branch, and stumbled on a few things.. hope you don't mind a bit very early feedback (feel free to ignore):

Imho there should be a way to cause a "real" crash, ie. calling [Crashlytics crash] because if I understand it correctly, crashlytics only ever sends crashes/logs/etc. when the app actually crashes and is subsequently restarted? (or even provide a isFatal flag in the onError method)
It would be nice if logs are also sent periodically, because otherwise if a native part of my application crashes, all "context" logging from the dart side is lost (although i guess i could simply do this manually as a user of the plugin)
api nitpicking .. I imagine user identifier, email, name would be usually set from the same location. how about providing one method with named parameters and only one native roundtrip :)

danwulff · 2019-03-07T18:37:03Z

Consider updating FlutterFire.md while we're at it? https://github.com/flutter/plugins/blob/master/FlutterFire.md

Salakar · 2019-03-13T14:23:16Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    }
+  }
+
+  void setBool(String key, bool value) {


setInt, setBool & setDouble are purely convenience methods on native SDKs (iOS/Android) and internally the SDK's convert these to a string value.

Perhaps it would be better to simplify this and have a singular method in Dart that accepts a bool, double, int or String value and this method converts to a String in dart and calls only one native method (setString)

@Salakar thanks for the feedback. We are trying to keep the plugins interface as close to the native ones as possible. So we will stick with the separate methods for now.

Salakar · 2019-03-13T14:23:26Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    setKey(key, value);
+  }
+
+  void setDouble(String key, double value) {


setInt, setBool & setDouble are purely convenience methods on native SDKs (iOS/Android) and internally the SDK's convert these to a string value.

Perhaps it would be better to simplify this and have a singular method in Dart that accepts a bool, double, int or String value and this method converts to a String in dart and calls only one native method (setString)

@Salakar thanks for the feedback. We are trying to keep the plugins interface as close to the native ones as possible. So we will stick with the separate methods for now.

Salakar · 2019-03-13T14:23:37Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    setKey(key, value);
+  }
+
+  void setInt(String key, int value) {


setInt, setBool & setDouble are purely convenience methods on native SDKs (iOS/Android) and internally the SDK's convert these to a string value.

Perhaps it would be better to simplify this and have a singular method in Dart that accepts a bool, double, int or String value and this method converts to a String in dart and calls only one native method (setString)

@Salakar thanks for the feedback. We are trying to keep the plugins interface as close to the native ones as possible. So we will stick with the separate methods for now.

Salakar · 2019-03-13T14:29:19Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+  }
+
+  void crash() {
+    throw StateError('Error thrown by Crashlytics plugin');


Should this not delegate to the native SDK methods to ensure a full fatal crash (app terminates) as Crashlytics only submits reports after a termination and subsequent app startup - wouldn't the Dart/Flutter error handler capture this in debug/dev and thus preventing this termination behaviour?

iOS: [[Crashlytics sharedInstance] crash];
Android: (new CrashTest()).crashAsyncTask(50); (crash off main thread to avoid being captured by error handlers in dev/debug)

@Salakar This was considered, however most Flutter level crashes don't result in the underlying native application crashing, so throwing an error here was an attempt to represent the types of crashes likely to happen an a Flutter app and how the report will ultimately be submitted to Crashlytics.

Salakar · 2019-03-13T14:39:10Z

packages/firebase_crashlytics/android/build.gradle

+
+dependencies {
+    implementation 'com.google.firebase:firebase-core:16.0.6'
+    implementation 'com.crashlytics.sdk.android:crashlytics:2.9.8'


No Crashlytics NDK support to capture low-level errors such as in Flutter Engine?

Dependency: implementation 'com.crashlytics.sdk.android:crashlytics-ndk:2.0.5'

Users build.gradle (documentation step only):

crashlytics { enableNdk true }

See: https://docs.fabric.io/android/crashlytics/ndk.html

I feel like that should be separate dependency

@Salakar thanks for the suggestion, we can definitely add this in a future version.

kroikie · 2019-03-13T22:46:33Z

@hpoul thanks for the feedback:

Non fatal crashes are the best we can do without changes to the underlying SDKs. We considered causing a fatal crash but decided that non fatals would be better till we have better native SDK support.
Good point, thanks for the suggestion.
It would be nice, but our current approach is to stay as close to the native SDK interfaces as possible. Still a good point and something we can consider later on.

kroikie · 2019-03-14T17:37:22Z

Cirrus is all green, PR has stale CI status. See finished builds here:
https://cirrus-ci.com/build/5687774492491776

collinjackson · 2019-03-15T22:27:11Z

packages/firebase_crashlytics/README.md

+Make sure to call FirebaseApp.initializeApp(Context) first.
+```
+
+*Note:* When you are debugging on android, use a device or AVD with Google Play services.


nit: capitalize Android

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

collinjackson · 2019-03-15T22:33:39Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    }
+  }
+
+  void setKey(String key, dynamic value) {


still needs dartdocs

collinjackson · 2019-03-15T22:43:46Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+
+  /// Logs to be included with report.
+  @visibleForTesting
+  final ListQueue<String> logs = ListQueue<String>(15);


If it's not part of the public API, it's an implementation detail and shouldn't be used in tests.

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

collinjackson · 2019-03-15T22:47:10Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+      // TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
+      // https://github.com/flutter/flutter/issues/26431
+      // ignore: strong_mode_implicit_dynamic_method
+      await channel.invokeMethod('Crashlytics#log', <String, dynamic>{


This seems like a lot of unnecessary async waiting, especially if you have big logs. Can we send everything to the native side as an array argument to onError and log it there?

Removed since logs are sent as part of error reporting.

collinjackson · 2019-03-15T22:48:56Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+        // TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
+        // https://github.com/flutter/flutter/issues/26431
+        // ignore: strong_mode_implicit_dynamic_method
+        await channel.invokeMethod('Crashlytics#setInt', crashlyticsKey);


I think it would be a bit more concise to send over the params map and switch on the type of the data on the native side, so that you're defining fewer MethodCall invocations.

collinjackson

minor style nit, lgtm otherwise

collinjackson · 2019-03-19T21:58:05Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+    return result;
+  }
+
+  /// Returns the Kit Version.


I think it would be clearer to say Crashlytics SDK version instead of Kit version

collinjackson · 2019-03-19T22:48:38Z

packages/firebase_crashlytics/test/firebase_crashlytics_test.dart

+      crashlytics.log('foo');
+      await crashlytics.onError(details);
+      expect(log[0].method, 'Crashlytics#onError');
+      expect(log[0].arguments['exception'], 'foo exception');


I think it would be more readable and consistent with other tests to write this as a single expect comparing log with an array of maps.

It sounds like StackTrace.current might make it hard to use the isMethodCall matcher, in which case this is fine.

After chat with @collinjackson we will leave this as is, since Stacktrace.current will likely not be the same on each execution.

* Add Crashlytics support

googlebot added the cla: yes label Jan 15, 2019

Taormina reviewed Jan 25, 2019

View reviewed changes

kroikie changed the title ~~[WIP] Firebase Crashlytics plugin~~ Firebase Crashlytics plugin Feb 14, 2019

kroikie assigned collinjackson Feb 14, 2019

slightfoot reviewed Feb 15, 2019

View reviewed changes

...a/io/flutter/plugins/firebase/crashlytics/firebasecrashlytics/FirebaseCrashlyticsPlugin.java Outdated Show resolved Hide resolved

collinjackson reviewed Feb 21, 2019

View reviewed changes

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart Show resolved Hide resolved

danwulff reviewed Feb 22, 2019

View reviewed changes

packages/firebase_crashlytics/example/pubspec.yaml Show resolved Hide resolved

cyanglaz changed the title ~~Firebase Crashlytics plugin~~ [firebase_crashlytics]Firebase Crashlytics plugin Feb 23, 2019

Salakar reviewed Mar 13, 2019

View reviewed changes

collinjackson reviewed Mar 15, 2019

View reviewed changes

kroikie added 11 commits March 19, 2019 08:52

Add Crashlytics support

5158a2f

Update sample to set reportInDevMode

b8ad281

best effort to add dart errors to console

13f2ec5

Cast code to int on iOS

91a68e2

Update readme

c96755d

fix typo

2ef93b0

Implement Crashlytics methods on Dart side

31458d3

add setString

7a9de7a

Add log and key implementations

701f2d1

fix formatting

5a56879

remove unused import

a83d187

kroikie added 18 commits March 19, 2019 08:52

fix formatting

b123ea9

fix formatting

6ac3fcc

fix example test

3dd19c7

remove example tests

0717138

Update documentation

51b7d3c

fix formatting

b2fecb7

fix formatting again

4300a30

implement isDebuggable and getVersion

189112b

fix formatting

a7d651c

Safer generation of stack trace elements from Dart error

66831ea

fix formatting

94d59ed

Respond to comments

bf3ef62

Submit stack trace for both iOS and Android

f811d78

Use FlutterErrorDetails.context for 'reason' on iOS

bd5eaf7

fix formatting

e96f148

make elements final

efc994e

Remove unnecessary dependencies

4710095

pass keys and docs with report

e3f8209

kroikie force-pushed the firebase_crashlytics branch from 0b1189b to e3f8209 Compare March 19, 2019 15:53

kroikie added 2 commits March 19, 2019 09:03

fix formatting

f4527c4

fix formatting again

2cdf8d6

collinjackson approved these changes Mar 19, 2019

View reviewed changes

Update getVersion docs

a2ec4a4

kroikie force-pushed the firebase_crashlytics branch from 4da42b1 to a2ec4a4 Compare March 20, 2019 16:36

Update example README

248ff13

kroikie merged commit 5c20e7d into flutter:master Mar 20, 2019

matthewlloyd mentioned this pull request May 7, 2019

[firebase_crashlytics] RangeError: Value not in range: -1 flutter/flutter#32178

Closed

kroikie mentioned this pull request Oct 13, 2019

[firebase_crashlytics] RangeError: Value not in range: -1 firebase/flutterfire#383

Closed

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

[firebase_crashlytics]Firebase Crashlytics plugin (flutter#1080)

a76670d

* Add Crashlytics support

Moncader mentioned this pull request May 20, 2020

[firebase_crashlytics] Migrate to new Crashlytics SDK firebase/flutterfire#2288

Closed

13 tasks

[firebase_crashlytics]Firebase Crashlytics plugin #1080

[firebase_crashlytics]Firebase Crashlytics plugin #1080

Uh oh!

Conversation

kroikie commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

collinjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hpoul commented Mar 4, 2019

Uh oh!

danwulff commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Salakar Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroikie commented Jan 15, 2019 •

edited

Loading

Salakar Mar 13, 2019 •

edited

Loading