CCCT-1952 New Firebase Analytics Property #3468
Conversation
Added the connect server to the user properties for all firebase analytics events.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThe PR introduces a new Firebase analytics user property Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes The changes are straightforward: a single constant addition and a single property assignment across two files with no complex logic or behavioral modifications. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
128-129: Set CONNECT_SERVER property using BuildConfig.CCC_HOST.The code correctly sets the user property with the compile-time constant. BuildConfig.CCC_HOST is defined in build.gradle and will always have a value, so a null/empty check is unnecessary. Unlike runtime values like domain and serverName which legitimately require TextUtils.isEmpty() guards, BuildConfig fields are never null.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
🧬 Code graph analysis (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
51-51: LGTM!The constant declaration follows the established pattern and naming conventions in this file.
|
|
||
| flagPersonalIDDemoUser(ReportingUtils.getIsPersonalIDDemoUser()); | ||
|
|
||
| analyticsInstance.setUserProperty(CCAnalyticsParam.CONNECT_SERVER, BuildConfig.CCC_HOST); |
There was a problem hiding this comment.
A question mostly for @shubham1g5: Do we only want to set this when the PersonalID user is logged in? Not sure if there's any harm in always logging it, other than maybe some extra storage costs in BigQuery.
There was a problem hiding this comment.
I think it should be fine, probably the most damage this user property is causing is it's consuming our quota of 25 user properties, I am curious if we don't already get the app flavor info in big query with events or in some other way ?
There was a problem hiding this comment.
Okay, that sounds good to me. Unfortunately I don't think the app flavor gets logged with analytics. I just double-checked the definitions for the analytics tables, and did a quick query on the app_info subsection specifically to verify it isn't included there
There was a problem hiding this comment.
Hmm ok, I think we are better off in that case is to log a flavor here instead -
FirebaseAnalytics.getInstance(context)
.setUserProperty("app_flavor", BuildConfig.FLAVOR)
We can use this to filter our connect staging but also won't need another user property in case we need to send flavor for some other purpose in future.
There was a problem hiding this comment.
Yeah, I like that idea
There was a problem hiding this comment.
yeah that's a good idea too, I think we would want to still see the crashes but agree that for analytics it feels best to skip these events on staging.
There was a problem hiding this comment.
I have a slight preference to keeping it for the following reasons:
- Now that we're cleaning up analytics older than 90 days I don't think storage costs are a big issue
- Leaving these events for staging would still be helpful for us to test them out (like testing that a new event is being logged correctly)
That being said, I'm also okay with skipping them. Do we know if QA still uses the cccStaging flavor when testing? They used to but in recent releases I think we've only been giving them prod builds...
There was a problem hiding this comment.
I'm ok with going either way too - I can see the pros of taking either route
There was a problem hiding this comment.
lets go ahead with the current solution then.
There was a problem hiding this comment.
+1 for testing new events for stagging @OrangeAndGreen.
…nto bug/CCCT-1952-new-firebase-analytics-property
Logged app flavor instead of connect server.
conroy-ricketts
deleted the
bug/CCCT-1952-new-firebase-analytics-property
branch
CCCT-1952
Technical Summary
I added the connect server to the user properties for all firebase analytics events. Here are the name-value pairs that will now appear for all events:
connect_server->connect.dimagi.comconnect_server->connect-staging.dimagi.comEdit: I updated the name-value pairs to the following:
app_flavor->commcareapp_flavor->cccStagingSafety Assurance
Safety story
I went into BigQuery and ran a query to verify that this new user property does show for my test device, and I tested with both a staging and a prod build.