Disabled Firebase Performance monitoring in Debug mode #3230
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis change removes outdated dependencies from the Sequence Diagram(s)sequenceDiagram
participant App as CommCareApplication
participant BuildConfig
participant FirebasePerf as FirebasePerformance
App->BuildConfig: Check DEBUG flag
alt If DEBUG is true
App->FirebasePerf: getInstance()
App->FirebasePerf: setPerformanceCollectionEnabled(false)
end
App->App: Continue normal initialization
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/org/commcare/CommCareApplication.java (1)
39-39: Missing Gradle/Manifest guard – import alone won’t prevent early perf initAdding the
FirebasePerformanceimport is harmless, but note that the Perf SDK is still auto-initialized byFirebaseInitProviderbeforeApplication.onCreate(). If the root cause of the crash is that this auto-init runs in debug builds with an invalid API-key, the new runtime flag below may run too late. Consider additionally disabling the Perf component fordebugviadebugImplementation (platform("com.google.firebase:firebase-bom:…")) { exclude group: "com.google.firebase", module: "firebase-perf" }or by putting
<meta-data android:name="firebase_performance_collection_enabled" android:value="false"/>in the
debugmanifest to stop the component from starting at all.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/build.gradle(0 hunks)app/src/org/commcare/CommCareApplication.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/build.gradle
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3070
File: app/res/values/themes.xml:80-80
Timestamp: 2025-05-07T06:48:32.621Z
Learning: The CommCare Android app doesn't require backward compatibility for support library features like the vector drawable compatibility provided by app:srcCompat. It uses android:src directly for drawable references in themes.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/layout/view_country_code_edittext.xml:2-7
Timestamp: 2025-05-09T06:50:50.863Z
Learning: The CommCare Android app uses minSdkVersion 21 (Android 5.0 Lollipop), so API 21+ attributes like paddingVertical and paddingHorizontal can be used without compatibility concerns.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/build.gradle:294-295
Timestamp: 2025-01-27T15:18:25.903Z
Learning: The Connect base URL and API version in build.gradle are intentionally hardcoded as these values do not change with variants.
app/src/org/commcare/CommCareApplication.java (10)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/utils/EncryptionUtils.java:167-167
Timestamp: 2025-01-27T15:14:56.422Z
Learning: MD5 hashing is acceptable for non-security purposes like analytics tracking in FirebaseAnalyticsUtil.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/layout/view_country_code_edittext.xml:2-7
Timestamp: 2025-05-09T06:50:50.863Z
Learning: The CommCare Android app uses minSdkVersion 21 (Android 5.0 Lollipop), so API 21+ attributes like paddingVertical and paddingHorizontal can be used without compatibility concerns.
Learnt from: shubham1g5
PR: dimagi/commcare-android#3116
File: app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java:0-0
Timestamp: 2025-05-19T15:36:34.511Z
Learning: In the CommCare Android codebase, a "fail fast" approach is preferred for internal encoding/decoding operations where both sides are controlled by the application. This helps surface bugs immediately rather than masking them with graceful error handling.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: avazirna
PR: dimagi/commcare-android#3057
File: app/src/org/commcare/views/widgets/AudioRecordingService.java:105-138
Timestamp: 2025-05-01T10:50:29.135Z
Learning: In the CommCare Android app, IllegalStateException from MediaRecorder operations should not be caught to ensure visibility of recording setup issues through app crashes.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
Comment on lines
222
to
224
| if (BuildConfig.DEBUG) { | ||
| FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Runtime flag may still trigger the same crash
FirebasePerformance.getInstance() itself performs substantial setup (including a FIS call) and can throw the same IllegalArgumentException the PR is trying to avoid. Wrapping the call in a try/catch keeps the debug build crash-free even if auto-init already failed, while still disabling collection when it succeeds:
-if (BuildConfig.DEBUG) {
- FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false);
-}
+if (BuildConfig.DEBUG) {
+ try {
+ FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false);
+ } catch (IllegalStateException | IllegalArgumentException e) {
+ Log.w(TAG, "Firebase Perf disabled – init failed in debug build", e);
+ }
+}This keeps fail-fast behaviour in release, yet prevents dev crashes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (BuildConfig.DEBUG) { | |
| FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false); | |
| } | |
| if (BuildConfig.DEBUG) { | |
| try { | |
| FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false); | |
| } catch (IllegalStateException | IllegalArgumentException e) { | |
| Log.w(TAG, "Firebase Perf disabled – init failed in debug build", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/CommCareApplication.java around lines 222 to 224, the
call to FirebasePerformance.getInstance() can throw an IllegalArgumentException
even in debug builds, causing crashes. To fix this, wrap the call to
FirebasePerformance.getInstance().setPerformanceCollectionEnabled(false) in a
try/catch block that catches IllegalArgumentException, so the debug build does
not crash if initialization fails, while still disabling performance collection
when possible.
Contributor
Author
|
@damagatchi retest this please |
avazirna
force-pushed
the
disabled-firebase-perf-monitoring-on-debug-mode
branch
from
245b5e1 to
3890a12
Compare
shubham1g5
reviewed
Jul 8, 2025
| implementation 'com.journeyapps:zxing-android-embedded:3.6.0' | ||
| implementation 'com.google.firebase:firebase-analytics:20.1.2' | ||
| implementation 'com.google.firebase:firebase-messaging:21.1.0' | ||
| implementation 'com.google.firebase:firebase-crashlytics:17.2.1' |
Contributor
There was a problem hiding this comment.
Can we explain this in context of PR ?
shubham1g5
approved these changes
Jul 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Product Description
Developers have reported crashes in debug mode after enabling Firebase Perf monitoring. The stack trace is:
Technical summary
Firebase Perf monitoring is disabled by default and enabled when in
Releasemode. This PR also removes duplicate dependency entries from the Gradle build configuration.Safety Assurance
Safety story
Automated test coverage
Labels and Review