-
Notifications
You must be signed in to change notification settings - Fork 528
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
Fix #4389, #4407: Future-proof event analytics logging infrastructure #4421
Conversation
This simplifies application component management significantly and allows individual build flavors to have their own unique module lists.
This also introduces dedicated beta & GA build flavors which is a necessary prerequisite. It also introduces an extra beta, alpha, and dev mode labels for the splash screen (the latter 2 were extra) with 2 second minimum wait timers for beta and alpha to ensure they are seen. A 5-second safety timer was added to ensure the splash screen can always be passed even if something goes wrong at the domain level (since there are now quite a few moving pieces to determine the user's current onboarding state).
This new build flavor automatically enables the study-specific learner study feature flag.
This is mainly done by: 1. Logging an immutable event type for each event and de-coupling the event names so that the latter can be changed over time to improve event management quality-of-life. 2. Logging the Android SDK version, app version name, and app version code with each event. 3. Ensuring the existing Kenya-specific alpha versions of the app continue to use their existing event names so that historical data can be matched (since those queries rely on event names rather than this new event type).
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
…oduce-beta-ga-notices
Tests broken due to changes to the app startup experience haven't yet been fixed.
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
There's a bunch left to do here, this is mainly needed so that I can transfer changes to a different machine.
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Correct typos.
Conflicts: build_flavors.bzl scripts/assets/test_file_exemptions.textproto
…oduce-beta-ga-notices Conflicts: .github/workflows/build_tests.yml build_flavors.bzl scripts/assets/test_file_exemptions.textproto version.bzl
Remaining TODOs before merge:
|
Conflicts: app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
…frastructure Conflicts: utility/src/main/java/org/oppia/android/util/logging/EventBundleCreator.kt utility/src/test/java/org/oppia/android/util/logging/EventBundleCreatorTest.kt
I've found nothing of concern during the self-review. This PR should be mergeable once it's brought up-to-date with develop and its base PR has been merged. |
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.
LGTM for .github/CODEOWNERS.
Thanks @seanlip! |
Assigning @rt4914 for code owner reviews. Thanks! |
…frastructure Conflicts: utility/src/main/java/org/oppia/android/util/logging/EventBundleCreator.kt utility/src/test/java/org/oppia/android/util/logging/EventBundleCreatorTest.kt
…a/oppia-android into future-proof-metrics-infrastructure
Since this PR is part of broader urgent work for the Beta MR1 release, I'm force-merging this without review. I've self-reviewed it and found no major issues. Furthermore, #4567 is tracking ensuring that this does get reviewed by someone else later after it's been merged. |
Explanation
Fixes #4389
Fixes #4407
This PR "future-proofs" the learner analytics logging infrastructure by:
event_type
parameter that contains the actual proto integer of the event rather than just logging a string representation (so that we can rename both the events in code, and the corresponding strings logged to Firebase). The event names, at this point, act as a nice point-of-reference while analyzing events but are no longer meant to be the primary identifying keys for events (event_type
should be used for this purpose, instead).Specifics about tests and exemptions:
EventTypeToHumanReadableNameConverter
is exempted from tests since it's an interface.KenyaAlphaEventTypeToHumanReadableNameConverterImpl
is exempted from tests since it's temporary, and its values are at least indirectly tested throughKenyaAlphaEventBundleCreatorTest
.KenyaAlphaEventBundleCreatorTest
was added as a fork ofEventBundleCreatorTest
to ensure the existing Kenya-specific event names are properly logged, but it will be removed in the future (as tracked by Remove Kenya user study specific constructs once study is over #4419). Note that this suite wasn't updated to verify the new fields likeEventBundleCreatorTest
was.Essential Checklist
For UI-specific PRs only
N/A -- This is a metrics infrastructure change that will have zero effect on the UI (except maybe how events are displayed in the developer options menu, but that's not important to include here since it isn't end user-facing).