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

Firebase Crashlytics #1911

Closed
wants to merge 1 commit into from
Closed

Firebase Crashlytics #1911

wants to merge 1 commit into from

Conversation

RaulUrtecho
Copy link
Collaborator

@RaulUrtecho RaulUrtecho commented Jun 26, 2022

πŸ—ƒ Github Issue Or Explanation for this PR. (What is it supposed to do and Why is needed)

#1910

βœ… Checklist

  • Github issue details are up to date for people to QA.
  • I have tested all my changes.

πŸ•΅οΈβ€β™‚οΈ 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

image

image

πŸ‘―β€β™€οΈ Paired with

@github-handle or "nobody" if you did not pair.

@RaulUrtecho RaulUrtecho linked an issue Jun 26, 2022 that may be closed by this pull request
@RaulUrtecho
Copy link
Collaborator Author

I need help here to compile for iOS at least once in order to update Pods.

@RaulUrtecho RaulUrtecho force-pushed the lw-1910 branch 2 times, most recently from adb1262 to e2a57f1 Compare June 26, 2022 23:07
@@ -0,0 +1,72 @@
// File generated by FlutterFire CLI.
Copy link
Collaborator Author

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

@RaulUrtecho RaulUrtecho force-pushed the lw-1910 branch 2 times, most recently from dad876b to d02b7bf Compare June 27, 2022 03:02
@seedsbot
Copy link

Fails
🚫 ❗ PR has more than 500 line changes

Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

Generated by 🚫 dangerJS against 010109e

"android_client_info": {
"package_name": "com.joinseeds.seedswallet"
"package_name": "com.joinseeds.parq"
Copy link
Collaborator

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

Copy link
Collaborator Author

@RaulUrtecho RaulUrtecho Jun 27, 2022

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 ?

Copy link
Collaborator Author

@RaulUrtecho RaulUrtecho Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have 2 android projects?
image

Copy link
Member

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.

Copy link
Member

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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❀️

Copy link
Member

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 {
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@RaulUrtecho RaulUrtecho Jun 27, 2022

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.

Copy link
Member

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": {
Copy link
Member

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}');
Copy link
Member

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

@n13 n13 closed this Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firebase Crashlytics
4 participants