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.

Sign up for GitHub

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

Jump to bottom

[ABW-2263] Crash reporting setup #632

Merged
merged 12 commits into from
Jan 16, 2024
Merged

[ABW-2263] Crash reporting setup #632

merged 12 commits into from
Jan 16, 2024

Conversation

jakub-rdx
Copy link
Contributor

@jakub-rdx jakub-rdx commented Nov 7, 2023 •
edited
Loading

Description

Do not merge until we get green light to proceed.
Crash reporting setup is available only in debugAlpha and release build types. By default user is opted-out, it can be enabled in App Settings.
Notes:

  • to build that branch you need to download up to date google-services.json from wallet firebase console and place it in app folder
  • I've added logNonFatal method and called in in few places in dapp login and transaction submit flows where we catch and handle exceptions. I verified that those non fatal exceptions are not logged when crash reporting is disabled, but we can also think about wrapping that method call into a use case which will also check local data store flag KEY_CRASH_REPORTING_ENABLED

What is shared with Crashlytics other then crash stacktrace

According to terms and conditions:

  • device state information - I suspect this mean make, model, specs etc (all that is visible in crash report page)
  • device UID - self explanatory,
  • device location data
  • information about how the Application was used - I suspect what screen was opened, for how long etc.
    As you can see some shared data categories are clear, and some are not very precise describing what exatly

How to test

To test it you would need to build debugAlpha/release or switch build.gradle crash reporting flag for debug build to true, place code that will crash the app somewhere and try that with crash reporting enabled/disabled, to verify if crashes are sent (or not sent) to specific project in firebase console. Propagation time that I observed was fast, less then a minute.

Screenshot

Example image of crash report
Zrzut ekranu 2023-11-8 o 10 08 50

Video

Short video with verbal explanation how this is supposed to work

Nagranie.z.ekranu.2023-11-8.o.09.57.43.mov

PR submission checklist

  • I have tested that crash reporting is only available in specific builds, and that no crash reports are being sent when reporting is disabled

@jakub-rdx jakub-rdx changed the title [ABW-2263] ]Crash reporting setup [ABW-2263] Crash reporting setup Nov 7, 2023
raf-rdx
raf-rdx reviewed Nov 7, 2023
View reviewed changes
app/build.gradle Outdated Show resolved Hide resolved
micbakos-rdx
micbakos-rdx reviewed Nov 8, 2023
View reviewed changes
...in/java/com/babylon/wallet/android/presentation/settings/appsettings/AppSettingsViewModel.kt Outdated
if (enabled) {
Firebase.crashlytics.deleteUnsentReports()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this a bit? Should we also delete unsent reports when disabling crash reporting or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even when you have disabled sending reports to crashlytics, they are stored in local database until sharing crashes is enabled. And we don't want to send anything from the period when user had crash reporting turned off. So we delete all the reports just before enabling sending them, so that only crashes that happen when you have reports on will be sent

micbakos-rdx
micbakos-rdx approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@micbakos-rdx micbakos-rdx left a comment

Choose a reason for hiding this comment

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

Nice job!

I would suggest adding google-services.json to .gitignore as we do with release keystore since we will not save it into git, so as not to accidentally commit it.

micbakos-rdx
micbakos-rdx reviewed Nov 8, 2023
View reviewed changes
app/src/main/res/values-en/strings.xml Outdated
@@ -880,4 +880,5 @@ You can always change the guarantee from this default in each transaction.</stri
<string name="factorSources_kind_offDeviceMnemonic">Seed phrase</string>
<string name="factorSources_kind_trustedContact">Third-party</string>
<string name="factorSources_kind_securityQuestions">Security Questions</string>
<string name="appSettings_crashReporting_title">Crash Reporting</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add it in Crowdin

@jakub-rdx
Copy link
Contributor Author

Nice job!

I would suggest adding google-services.json to .gitignore as we do with release keystore since we will not save it into git, so as not to accidentally commit it.

It is already there, and google-services.json is added to 1password and to build setup already

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@micbakos-rdx
Copy link
Contributor

Nice job!
I would suggest adding google-services.json to .gitignore as we do with release keystore since we will not save it into git, so as not to accidentally commit it.

It is already there, and google-services.json is added to 1password and to build setup already

Ahh hadn't notice that. Perfect 👌

@micbakos-rdx
Copy link
Contributor

Just adding a comment here as a reminder to log the IOException in EncryptedPreferencesManager so as to find out what devices cause the manager to fail upon initial request.

@jakub-rdx jakub-rdx force-pushed the feature/ABW-2263-crash-reporting branch from 9ed38e5 to ae68529 Compare December 14, 2023 12:40
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 14, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

@jakub-rdx jakub-rdx force-pushed the feature/ABW-2263-crash-reporting branch 2 times, most recently from 28400fc to 8ff29bc Compare January 10, 2024 18:45
@jakub-rdx jakub-rdx force-pushed the feature/ABW-2263-crash-reporting branch from b27f5b0 to 2ee678a Compare January 15, 2024 19:36
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 15, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

2.1% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

giannis-rdx
giannis-rdx approved these changes Jan 16, 2024
View reviewed changes
app/build.gradle
@@ -56,6 +58,7 @@ android {
versionCode 27
versionName "1.3.0"
buildConfigField "boolean", "DEBUG_MODE", "true"
buildConfigField "boolean", "CRASH_REPORTING_AVAILABLE", "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

"enabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had discussion about this. This controls if switch to turn it on is visible in settings, not if it is enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

oh ok thanks!

@jakub-rdx jakub-rdx merged commit 7e5da95 into main Jan 16, 2024
8 of 9 checks passed
@jakub-rdx jakub-rdx deleted the feature/ABW-2263-crash-reporting branch January 16, 2024 10:18
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giannis-rdx giannis-rdx giannis-rdx approved these changes

@micbakos-rdx micbakos-rdx micbakos-rdx approved these changes

@raf-rdx raf-rdx Awaiting requested review from raf-rdx

Assignees
No one assigned
Labels
Do not merge
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakub-rdx @micbakos-rdx @raf-rdx @giannis-rdx @duje-begonja-rdx