-
Notifications
You must be signed in to change notification settings - Fork 23
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
Firebase Crashlytics #1911
Firebase Crashlytics #1911
Conversation
I need help here to compile for iOS at least once in order to update Pods. |
adb1262
to
e2a57f1
Compare
@@ -0,0 +1,72 @@ | |||
// File generated by FlutterFire CLI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The apiKey in this configuration snippet just identifies your Firebase project on the Google servers. It is not a security risk for someone to know it.
See more here:
https:// github.com/ firebase/flutterfire/ discussions/ 7617
dad876b
to
d02b7bf
Compare
"android_client_info": { | ||
"package_name": "com.joinseeds.seedswallet" | ||
"package_name": "com.joinseeds.parq" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think this change is supposed to be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intresting these changes were performed by the FlutterFire ClI tool I only pick the seeds project in available options to config the Crashlytics.
Should I keep that name @n13 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't change that.
This is correct
package_name": "com.joinseeds.seedswallet
Why we have multiple firebase projects - Igor did this, we can delete them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RaulUrtecho is there's strange merge errors, I'd tend to just start over.
if (kDebugMode) { | ||
print('mapEosError: $error'); | ||
} | ||
FirebaseCrashlytics.instance.log('mapEosError: $error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
β€οΈ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this removes EOS errors completely from the device log, then I don't agree with this change.
If crashlytics prints the errors to console as well, then OK.
EOS errors must be in the device logs, anyway.
import 'package:http/http.dart' as http; | ||
import 'package:seeds/datasource/remote/api/http_repo/seeds_scopes.dart'; | ||
import 'package:seeds/datasource/remote/api/http_repo/seeds_tables.dart'; | ||
import 'package:seeds/datasource/remote/firebase/firebase_remote_config.dart'; | ||
import 'package:seeds/datasource/remote/util/response_extension.dart'; | ||
|
||
class _HttpClient extends http.BaseClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we should fire every request into crashalytics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flutterfire docs says for log method:
To avoid slowing down your app, Crashlytics limits logs to 64kB and deletes older log entries when a session's logs go over that limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
64 kB is quite a lot text I don't think be a problem also I did not notice any bottle neck in the app.
However this can be a big hindrance in issue diagnostic since many at times by the time we identify an issue and begin diagnostics, the logs are gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let's not go overboard with Crashlytics.
It's useful but we do not want to depend on it (what if crashlytics is down, do these still work?)
We also don't want to send all http requests through crashlytics.
Only ones that have unexpected crashes.
We also need to filter out expected crashes - example, network error because user's internet was down.
@@ -53,18 +45,18 @@ | |||
}, | |||
{ | |||
"client_info": { | |||
"mobilesdk_app_id": "1:366914936426:android:389b18d13a862a531a3413", | |||
"mobilesdk_app_id": "1:366914936426:android:7d2393ccb8b3bdda1a3413", | |||
"android_client_info": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever the package name is, don't change it?!
if (kDebugMode) { | ||
print('http error: ${response.reasonPhrase}'); | ||
} | ||
FirebaseCrashlytics.instance.log('http error: ${response.statusCode}, ${response.reasonPhrase}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here - still going to device log?
if (kDebugMode) { | ||
print('mapHttpError: $error'); | ||
} | ||
FirebaseCrashlytics.instance.log('mapHttpError: $error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
π Github Issue Or Explanation for this PR. (What is it supposed to do and Why is needed)
#1910
β Checklist
π΅οΈββοΈ Notes for Code Reviewer
This is something that we need to have yes or yes and since for a long time ago.
With this we can manage our attention to bugs reported by users.
π Screenshots
π―ββοΈ Paired with
@github-handle or "nobody" if you did not pair.