Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

kkmurerwa · 2023-11-22T06:32:50Z

Explanation

When merged, this PR will:

Create a FeatureFlagLogger.kt file that aggregates and logs all feature flags.
Add a FeatureFlagLoggerTest.kt to test the FeatureFlagLogger.
Add logging logic to the ApplicationLifecycleObserver.kt file so that it is logged when the app is in foreground.

Screenshot of the FeatureFlagContext log in the Event Logs page

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

… persistent cache store

…ules

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>

…rameter file

…o a common constants file

…a/oppia-android into technical-analytics-milestone-1

…implify sync monitoring

…a/oppia-android into technical-analytics-milestone-1

adhiamboperes · 2024-04-02T11:26:14Z

@kkmurerwa, checking if you meant to assign this to @BenHenning.

kkmurerwa · 2024-04-02T11:32:41Z

This is not done yet. I meant to assign M3 to you and ended up assigning M2 instead.

oppiabot · 2024-04-09T12:01:01Z

Hi @kkmurerwa, 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.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

kkmurerwa · 2024-04-14T14:41:26Z

@BenHenning this PR is now ready for another review. PTAL

BenHenning

Thanks @kkmurerwa! Just small things left I think--PTAL.

BenHenning · 2024-04-16T22:47:34Z

...src/main/java/org/oppia/android/domain/oppialogger/analytics/ApplicationLifecycleObserver.kt

@@ -176,6 +176,7 @@ class ApplicationLifecycleObserver @Inject constructor(

  private fun logAllFeatureFlags() {
    CoroutineScope(backgroundDispatcher).launch {
+      // TODO(#5240): Replace appSessionId generation to the modified Twitter snowflake algorithm.


This should point to an issue, not a PR. Should it be pointing to #5341? Though, I think that issue also needs to be updated to explain the additional work of using the modified snowflake algorithm (per the TDD).

I have modified the reference on the code and updated the issue to include the detail about the Twitter snowflake algorithm. Let me know if the issue description is detailed enough.

I see the TODO was removed from code--shouldn't it stay here and point to #5341?

I also think #5341 is a bit confusing since, after this PR is merged, it will seem as if an ID is being provided. The issue should probably instead explain what needs to be changed from the solution introduced in this PR to get analytics to where we want them long-term.

I have updated the issue description and reverted the deletion that removed the TODO from the codebase.

BenHenning · 2024-04-16T22:49:42Z

domain/src/test/java/org/oppia/android/domain/oppialogger/LoggingIdentifierControllerTest.kt

@@ -251,6 +251,44 @@ class LoggingIdentifierControllerTest {
    assertThat(sessionIdFlow.value).isEqualTo("59aea8d4-af4b-3249-b889-dfeba06d0495")
  }

+  @Test
+  fun testGetAppSessionId_initialState_returnsRandomId() {


Please add a test to verify a new app instance to demonstrate a different ID is generated, similar to this test:

oppia-android/domain/src/test/java/org/oppia/android/domain/oppialogger/LoggingIdentifierControllerTest.kt

Line 100 in 4f03d9a

fun testGetInstallationId_secondAppOpen_providerReturnsSameInstallationIdValue() {

I have added a test that kind of checks for this but I will need some validation on whether it does that correctly.

As discussed in the meeting today, we should probably be doing something like the following to ensure correct behavior:

Arrange

Set fixed application random seed to value (explicitly within the test). Note this should be done in the test that doesn't verify app restart, too, for parity.

Set up rest of test as normal

Act

Update the application random seed to be something different.

Recreate the application component & reinit the test suite fields, where needed.

Assert: assert as normal (that the ID is different than in the test which doesn't 'restart' the app).

I am unable to create a test that can objectively test that the app is generating different app session IDs on restart based on how the test file is structured. I have created an issue to track this #5399. Once fixed, all tests on the file should now pass (or fail) for the correct reason.

domain/src/test/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLoggerTest.kt

kkmurerwa · 2024-05-06T12:25:28Z

@BenHenning this PR should be ready for another review. PTAL

BenHenning

Thanks @kkmurerwa! Just a few follow-ups--PTAL.

BenHenning · 2024-05-07T22:23:22Z

domain/src/main/java/org/oppia/android/domain/oppialogger/LoggingIdentifierController.kt

+   *  1. The app is opened.
+   *  2. The app is destroyed by the Android system.
+   */
+  fun updateAppSessionId() {


Why is this needed? It seems like we shouldn't ever need to explicitly updated the session ID since it autoinitializes upon app open.

I realize that now. I will remove this.

BenHenning · 2024-05-07T22:25:35Z

...src/main/java/org/oppia/android/domain/oppialogger/analytics/ApplicationLifecycleObserver.kt

@@ -176,6 +176,7 @@ class ApplicationLifecycleObserver @Inject constructor(

  private fun logAllFeatureFlags() {
    CoroutineScope(backgroundDispatcher).launch {
+      // TODO(#5240): Replace appSessionId generation to the modified Twitter snowflake algorithm.


I see the TODO was removed from code--shouldn't it stay here and point to #5341?

I also think #5341 is a bit confusing since, after this PR is merged, it will seem as if an ID is being provided. The issue should probably instead explain what needs to be changed from the solution introduced in this PR to get analytics to where we want them long-term.

BenHenning · 2024-05-07T22:28:59Z

domain/src/test/java/org/oppia/android/domain/oppialogger/LoggingIdentifierControllerTest.kt

@@ -251,6 +251,44 @@ class LoggingIdentifierControllerTest {
    assertThat(sessionIdFlow.value).isEqualTo("59aea8d4-af4b-3249-b889-dfeba06d0495")
  }

+  @Test
+  fun testGetAppSessionId_initialState_returnsRandomId() {


As discussed in the meeting today, we should probably be doing something like the following to ensure correct behavior:

Arrange

Set fixed application random seed to value (explicitly within the test). Note this should be done in the test that doesn't verify app restart, too, for parity.

Set up rest of test as normal

Act

Update the application random seed to be something different.

Recreate the application component & reinit the test suite fields, where needed.

Assert: assert as normal (that the ID is different than in the test which doesn't 'restart' the app).

kkmurerwa and others added 30 commits October 18, 2023 10:01

feat: Add a FeatureFlagConstants file to host feature flags

3fee528

feat: Add constant names for easy retrieval of the feature flags from…

9c09fbf

… persistent cache store

feat: Add newly status flags to the Platform Parameter dependency mod…

b0d1572

…ules

feat: Add ability to insert sync statuses to the cache store

7e13530

feat: Add ability to save flag statuses for every feature flag

6c06ea5

Merge branch 'oppia:develop' into technical-analytics-milestone-1

5e26d8e

fix: Fix linting and styling checks

1f0fa64

Merge branch 'develop' into technical-analytics-milestone-1

f4f5ff2

fix: Fix comments made on the previous review

795e90c

Merge branch 'develop' into technical-analytics-milestone-1

dd5c5b2

Merge branch 'develop' into technical-analytics-milestone-1

da852b4

fix: Fix nit

9da1607

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>

Merge branch 'develop' into technical-analytics-milestone-1

0c0cd75

fix: Fix lint issues causing build failure

4dae83c

fix: Move test sync status flag booleans to the TestBooleanPlatformPa…

81d968b

…rameter file

chore: Move all test constants from individual type-organized files t…

d8f14bd

…o a common constants file

Merge branch 'develop' into technical-analytics-milestone-1

7918da5

fix: Fix failing text file checks

7bcbdcf

Merge branch 'technical-analytics-milestone-1' of github.com:kkmurerw…

ed2b7ac

…a/oppia-android into technical-analytics-milestone-1

Merge branch 'develop' into technical-analytics-milestone-1

5638b31

Merge branch 'develop' into technical-analytics-milestone-1

a1846ce

feat: Add a isSynced variable in the platform parameter variable to s…

b49df60

…implify sync monitoring

fix: Fix failing EventBundleCreator tests

7ee1fd8

feat: Make Sync status an enum

b9f28ba

Merge branch 'develop' into technical-analytics-milestone-1

c0338b0

feat: Add feature flag names to all flags lacking one

501ae40

Merge branch 'technical-analytics-milestone-1' of github.com:kkmurerw…

91c526d

…a/oppia-android into technical-analytics-milestone-1

fix: Fix failing check and linting issue

551fd4a

Merge branch 'develop' into technical-analytics-milestone-1

e6d748f

merge: Merge from develop and fix conflicts

3b19e3f

feat: Add tests for the appSessionId and the user UUID

e8503b7

kkmurerwa requested a review from BenHenning April 2, 2024 11:33

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 9, 2024

merge from develop

6d20011

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 14, 2024

kkmurerwa added 2 commits April 14, 2024 09:47

test: Add parameterized test for FeatureFlagsLoggerTest

46d5484

fix: Fix failing parameterized tests and checks

4f03d9a

kkmurerwa assigned BenHenning and unassigned kkmurerwa Apr 15, 2024

BenHenning requested changes Apr 16, 2024

View reviewed changes

BenHenning assigned kkmurerwa and unassigned BenHenning Apr 16, 2024

kkmurerwa added 5 commits April 23, 2024 14:58

Merge branch 'develop' into technical-analytics-milestone-2

97dcc02

chore: Make changes made in last review.

5f1af8d

chore: Add test to ensure app session id changes on each request.

3fa23aa

fix: Fix failing todos check.

1a7cb3d

fix: Fix failing todos check.

c140838

kkmurerwa assigned BenHenning and unassigned kkmurerwa May 6, 2024

BenHenning reviewed May 7, 2024

View reviewed changes

BenHenning assigned kkmurerwa and unassigned BenHenning May 7, 2024

kkmurerwa added 3 commits May 14, 2024 20:39

feat: Make some of the changes.

e4d6b57

Merge branch 'develop' into technical-analytics-milestone-2

bd3f2de

fix: Remove unnecessary test.

c7184c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

kkmurerwa commented Nov 22, 2023 •

edited

adhiamboperes commented Apr 2, 2024

kkmurerwa commented Apr 2, 2024

oppiabot bot commented Apr 9, 2024

kkmurerwa commented Apr 14, 2024 •

edited

BenHenning left a comment

BenHenning Apr 16, 2024

kkmurerwa Apr 25, 2024

BenHenning May 7, 2024

kkmurerwa May 18, 2024

BenHenning Apr 16, 2024

kkmurerwa May 6, 2024

BenHenning May 7, 2024

kkmurerwa May 18, 2024 •

edited

kkmurerwa commented May 6, 2024

BenHenning left a comment

BenHenning May 7, 2024

kkmurerwa May 8, 2024

BenHenning May 7, 2024

BenHenning May 7, 2024

Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

Are you sure you want to change the base?

Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

Conversation

kkmurerwa commented Nov 22, 2023 • edited

Explanation

Screenshot of the FeatureFlagContext log in the Event Logs page

Essential Checklist

For UI-specific PRs only

adhiamboperes commented Apr 2, 2024

kkmurerwa commented Apr 2, 2024

oppiabot bot commented Apr 9, 2024

kkmurerwa commented Apr 14, 2024 • edited

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkmurerwa May 18, 2024 • edited

Choose a reason for hiding this comment

kkmurerwa commented May 6, 2024

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkmurerwa commented Nov 22, 2023 •

edited

kkmurerwa commented Apr 14, 2024 •

edited

kkmurerwa May 18, 2024 •

edited