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.

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 #4249, part of #4064: Domain components for learner analytics #4267

Merged
merged 86 commits into from
May 5, 2022

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Mar 28, 2022

Explanation

Fix #4249
Fix part of #4064

Introduces the domain utilities necessary for logging learner analytics, but doesn't make them available for actual usage yet (that is being done in #4269 to keep this PR smaller & more focused).

Some notes on the history of this PR:

This PR makes a number of changes over the original design document and implementation, the most noteworthy being:

  • This PR organizes learner analytics logging into its own logger (and makes changes to event bundle creation & the generic OppiaLogger). I think that we should move toward this pattern generally in the future rather than continuing with a generic OppiaLogger as it seems to help keep things much more focused. Existing logging should not be affected.
  • The notion of a device ID has been dropped as there's no reliable way retrieve such an ID (see https://developer.android.com/training/articles/user-data-ids). Instead, we're using a per-device ID (by leveraging PersistentCacheStore), and have confirmed with study partners that this is workable.
  • The logging logic for the new logs was rearranged such that all new analytics logs will be logged for everyone, but the user and installation-tied IDs won't be logged in such cases (since they are more sensitive data). These events are generally useful for the platform, so we shouldn't restrict them as such.
  • Learner ID generation for profiles only occurs if the experiment is enabled, and otherwise stays empty. We may add future cleanup code to ensure it's erased across studies, but this at least lays the initial groundwork to keep such IDs separate when they aren't needed.

For a high-level on the design, please refer to this design document.

Other noteworthy design choices:

  • DebugEventLogger was updated to call through to the real logger (as it makes event verification simpler in developer builds; normally analytics is off so this won't have any effects for the broader team)
  • Both DebugEventLogger and FakeEventLogger were updated to be thread-safe
  • Some extended functionality was added to FakeEventLogger
  • LearnerAnalyticsLogger is designed a bit differently compared to other domain classes in that it actually provides session-specific objects to the application-wide singleton graph (which is needed for logging certain situations, such as the user playing/stopping audio during a play session)
  • LoggingIdentifierController makes use of a lazy retrieval for session ID now (which is fine because it's guaranteed to compute exactly one initial ID)
  • StateFlow is used for easier cross-thread communication, including to expose internal asynchronous state across domain components (the only way we had to do this before was Deferred, and that can be clunky; the new approach is much cleaner)
  • An EventLogSubject was introduced to make testing event logs easier. It's used extensively in tests for this feature, but most existing use cases weren't migrated. Add tests for EventLogSubject #4272 is tracking adding tests for this subject (hence the test file exemption).
  • There were TODOs introduced on Add basic learner analytics support #4064 to provide explicit clarity to reviewers on what needs to be changed in later PRs (as there's some things being introduced before the final PR that aren't actually used yet to help break up the project)
  • Multiple test suites verify behaviors with and without the feature enabled to be very explicit about what behavior occurs when
  • EventBundleCreatorTest in particular has very strict tests to ensure that sensitive IDs are logged exactly when expected (initially, never since they aren't turned on in this PR; this is fixed in a later PR)
  • ExplorationDataController was updated to introduce new play entrypoints, but these aren't "interesting" yet as the underlying ExplorationProgressController changes are coming in a later PR. Further, testing coverage technically removes checking playExploration, but it'll be removed (and it's technically tested through the other functions since they call through).
  • A new ClipboardManager was introduced with the specific design of not allowing the broad app access to clipboard information from other apps. Instead, it provides an interface to confirm whether the app's known clipboard has been kept. A regex content check was added to ensure developers never use the clipboard service directly and instead use this manager.
  • PersistentCacheStore was updated to include a new primeInMemoryAndDiskCacheAsync function which works more predictably for initialization than primeInMemoryCacheAsync (formerly primeCacheAsync). In particular, primeInMemoryCacheAsync is better for ensuring that the cache will quickly be read once it needs to be (and, if it isn't, will default in the same way the cache store normally defaults). However, there are cases when the app wants to change the default values such that: (1) the normal default is never used, (2) the default has to be computed and isn't cheap, and (3) it should never compute that default again once saved on disk. primeInMemoryAndDiskCacheAsync makes these assurances which, in turn, makes the installation ID cache store even possible without potential race conditions or breaking Dagger's cheap-initialization best practice.

Test exemptions: all exemptions are annotations or interfaces except EventBundleCreatorTest (which is explained above).

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

N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected.

@BenHenning
Copy link
Sponsor Member Author

PTAL @anandwana001 at the whole PR (as you'll be the sole reviewer for this change).

I'm also planning to defer @vinitamurthi's review per earlier conversations.

@oppiabot
Copy link

oppiabot bot commented Apr 22, 2022

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.
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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 22, 2022
@oppiabot oppiabot bot closed this Apr 29, 2022
@BenHenning BenHenning reopened this May 2, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 2, 2022
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@oppiabot
Copy link

oppiabot bot commented May 3, 2022

Unassigning @anandwana001 since the review is done.

@oppiabot
Copy link

oppiabot bot commented May 3, 2022

Hi @BenHenning, it looks like some changes were requested on this pull request by @anandwana001. PTAL. Thanks!

@oppiabot
Copy link

oppiabot bot commented May 3, 2022

Assigning @rt4914, @vinitamurthi for code owner reviews. Thanks!

@BenHenning
Copy link
Sponsor Member Author

Deferring @rt4914's review (tracked in #4266) to help get this merged sooner, and since @anandwana001 has reviewed the whole PR.

@BenHenning BenHenning merged commit a7c8e6c into develop May 5, 2022
@BenHenning BenHenning deleted the learner-analytics-pt4-domain-components-rebased branch May 5, 2022 01:54
BenHenning added a commit that referenced this pull request May 5, 2022
## Explanation
Fix part of #4064

This PR integrates the constituent Dagger modules introduced in #4267 broadly in the app so that new logs can be easily added without changing a bunch of files (see #4270).

Some notes on the history of this PR:
- This PR is a rebase of #4257 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history.
- This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601.
- This PR extracts just the 'domain integration' pieces from the above, and makes some fixes

For a high-level on the design, please refer to [this design document](https://docs.google.com/document/d/1c8lpH-IUvoU1t4LUoYNqNilP2e9yCnzGnSSG0yBxBrY/edit).

For the most part, this PR only changes module lists except for:
- Changes in ``AnalyticsController`` (& corresponding tests)
- Changes in ``LogUploadWorker`` (& corresponding tests)
- ``OptionsFragmentTest`` (both of them) since they need to provide custom parameter overrides (the new override needed to be added)

The non-test changes above were done to "partially integrate" the necessary pieces that couldn't be done in #4267 without blowing up the PR, and to ensure #4270 can stay focused on just introducing new events & their verification tests.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation.
- [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected.

Commits:

* strings for learner analytics

* platform parameter impl for learner analytics

* nit

* nit

* event action enum update

* addition of contexts

* nit

* controller level logging and contexts

* nit

* nit fixes.

* nit fixes.

* event bundle modifications

* sync status, logging identifiers, profile update, lifecycle owner

* ui impl: part 1 -- basic

* admin control strings

* strings correction

* strings correction

* device id correction

* exhaustive when fix.

* exhaustive when fix.

* todo formatting

* nits.

* nits.

* collapsed contexts, added spacing, added comments

* event action removal + nits

* tests + dev options event logs fixes post event action removal

* nits

* removal of method for event action formatted string

* nits, null context changes.

* nits

* reserved fixes and help index fix

* bazel imports

* bazel build fixes

* test fixes

* nit

* logging identifier controller, module + uuid wrapper, real impl

* logging identifier controller tests, fake uuid, tests

* sync status manager + fake

* logging methods, test setup

* profile management, tests

* sync status update.

* lifecycle observer

* Post-merge fixes + Bazel support.

* Lots of reorganizing & changes.

New tests and documentation have also been added. More broadly, this
changes the device ID computation, but actually breaks it so more work
will be needed in subsequent commits.

* Lint fixes.

* Post-merge fix (proper merge of maven_install).

* Lint fixes (includes post-merge cleanups).

* Lots of stuff.

Restructured a lot of the UI, addressed most failing static checks
(except KDocs and lint which will be in the follow-up commit), added
tests, fixed copying, and generally finished the UI.

Sync status seems broken, and it's not yet clear whether events are
actually being logged (I need to investigate this). Analytics are
disabled in local testing, so that might also be the reason for logs
being stuck in an uploading state.

* Documentation + lint fixes.

This also changes the contract of ClipboardController.

* Finish remaining planned tests.

* Move over changes from learner-analytics-proto.

* Manually pull in changes from 3d6c716.

Note that this is operating on a different base).

* Post-merge fixes.

These at least ensure that the app can build, but many tests will still
fail (which is fine seeing as much of this code is going to be split up
soon, anyway).

Rebase version: app build is no longer guaranteed.

* Lint fixes.

* Undo all learner analytics changes.

I'll be pulling in specific components in specific PRs to organize the
changes across 4 PRs.

Note that I took this approach to preserve the history from the earlier
commits. Those changes will still be included in this PR chain, just a
bit awkwardly (i.e. it'll look like I introduced them originally, but
that distinction is lost during the squash-and-merge, anyway).

* Manually pull in non-app module changes.

A bunch of work is still needed to finish these, and I'm still trying to
figure out whether I can de-couple the module changes to make reviewing
a bit nicer.

* Post-merge fixes.

All tests verified as building & passing.

* Add sync status for no connectivity case.

* Remove unnecessary sync manager.

* Copy over changes from #4263.

These are the domain changes needed for finishing learner analytics
support. Cleanup, documentation, and testing all still need to be
completed.

* Add domain changes for AudioPlayerController.

These originate from #4263.

* Add missing Javadoc from #4263.

* Add module deps for learner analytics components.

This ensures that the new domain components introduced in the previous
branch can be used throughout the app in both production and test
components.

* Post-merge fixes from different branch.

* Remove all UserIdProdModule references.

The module was removed during development in favor of a more integrated
approach to managing the logging-related IDs.

* Post-merge fix.

* Remove references to missing module per #4263.

* Add files changes from manual merges/cherry-picks.

* Finish tests & documentation.

This also renames 'device ID' to be 'installation ID' for more
correctness.

* Lint fixes.

* Post-merge fixes.

* Lint fixes.

* Remove files incorrectly readded.

* Add follow-up tests to improve coverage.

* Update domain's Gradle Mockito version to Bazel's.

* Fix OS-specific issue in ClipboardController.

Co-authored-by: Sarthak Agarwal <agarwal.sarthak262012@gmail.com>
BenHenning added a commit that referenced this pull request May 5, 2022
## Explanation
Fix part of #4064

This PR integrates the constituent Dagger modules introduced in #4267 broadly in the app so that new logs can be easily added without changing a bunch of files (see #4270).

Some notes on the history of this PR:
- This PR is a rebase of #4258 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history.
- This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601.
- This PR introduces new logging that didn't exist in the original PRs (#4118, #4247, and #4248)

This PR broadly adds all of the analytics event described in the TDD above (& its corresponding PRD), needed for an upcoming user study with the app. It *does* enable actual logging if analytics were to be enabled in the Android manifest (even though the UI side isn't implemented yet--that's planned for #4271).

## Essential Checklist
- [x] 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: ...".)
- [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation.
- [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected.

Commits:

* strings for learner analytics

* platform parameter impl for learner analytics

* nit

* nit

* event action enum update

* addition of contexts

* nit

* controller level logging and contexts

* nit

* nit fixes.

* nit fixes.

* event bundle modifications

* sync status, logging identifiers, profile update, lifecycle owner

* ui impl: part 1 -- basic

* admin control strings

* strings correction

* strings correction

* device id correction

* exhaustive when fix.

* exhaustive when fix.

* todo formatting

* nits.

* nits.

* collapsed contexts, added spacing, added comments

* event action removal + nits

* tests + dev options event logs fixes post event action removal

* nits

* removal of method for event action formatted string

* nits, null context changes.

* nits

* reserved fixes and help index fix

* bazel imports

* bazel build fixes

* test fixes

* nit

* logging identifier controller, module + uuid wrapper, real impl

* logging identifier controller tests, fake uuid, tests

* sync status manager + fake

* logging methods, test setup

* profile management, tests

* sync status update.

* lifecycle observer

* Post-merge fixes + Bazel support.

* Lots of reorganizing & changes.

New tests and documentation have also been added. More broadly, this
changes the device ID computation, but actually breaks it so more work
will be needed in subsequent commits.

* Lint fixes.

* Post-merge fix (proper merge of maven_install).

* Lint fixes (includes post-merge cleanups).

* Lots of stuff.

Restructured a lot of the UI, addressed most failing static checks
(except KDocs and lint which will be in the follow-up commit), added
tests, fixed copying, and generally finished the UI.

Sync status seems broken, and it's not yet clear whether events are
actually being logged (I need to investigate this). Analytics are
disabled in local testing, so that might also be the reason for logs
being stuck in an uploading state.

* Documentation + lint fixes.

This also changes the contract of ClipboardController.

* Finish remaining planned tests.

* Move over changes from learner-analytics-proto.

* Manually pull in changes from 3d6c716.

Note that this is operating on a different base).

* Post-merge fixes.

These at least ensure that the app can build, but many tests will still
fail (which is fine seeing as much of this code is going to be split up
soon, anyway).

Rebase version: app build is no longer guaranteed.

* Lint fixes.

* Undo all learner analytics changes.

I'll be pulling in specific components in specific PRs to organize the
changes across 4 PRs.

Note that I took this approach to preserve the history from the earlier
commits. Those changes will still be included in this PR chain, just a
bit awkwardly (i.e. it'll look like I introduced them originally, but
that distinction is lost during the squash-and-merge, anyway).

* Manually pull in non-app module changes.

A bunch of work is still needed to finish these, and I'm still trying to
figure out whether I can de-couple the module changes to make reviewing
a bit nicer.

* Post-merge fixes.

All tests verified as building & passing.

* Add sync status for no connectivity case.

* Remove unnecessary sync manager.

* Copy over changes from #4263.

These are the domain changes needed for finishing learner analytics
support. Cleanup, documentation, and testing all still need to be
completed.

* Add domain changes for AudioPlayerController.

These originate from #4263.

* Add missing Javadoc from #4263.

* Add module deps for learner analytics components.

This ensures that the new domain components introduced in the previous
branch can be used throughout the app in both production and test
components.

* Post-merge fixes from different branch.

* Remove all UserIdProdModule references.

The module was removed during development in favor of a more integrated
approach to managing the logging-related IDs.

* Post-merge fix.

* Remove references to missing module per #4263.

* Add files changes from manual merges/cherry-picks.

* Add event log callsites.

* Add missing event log from #4263.

This one is for audio voiceover playing.

* Finish tests & documentation.

This also renames 'device ID' to be 'installation ID' for more
correctness.

* Lint fixes.

* Post-merge fixes.

* Lint fixes.

* Remove files incorrectly readded.

* Add follow-up tests to improve coverage.

* Post-merge & lint fixes.

* Implement tests to verify event logging.

These tests only verify event logging at the domain layer, though
technically these events will also be logged from UI-based user
interactions. Tests to verify that's the case will be added in #4271 as
not all aspects of logging are properly hooked up in the UI layer as of
this PR.

* Update domain's Gradle Mockito version to Bazel's.

* Add missing tests & needed behaviors.

Fix broken app test.

* Fix OS-specific issue in ClipboardController.

Co-authored-by: Sarthak Agarwal <agarwal.sarthak262012@gmail.com>
@rt4914
Copy link
Contributor

rt4914 commented Jun 20, 2022

Approved. I have checked only for code-owner files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update SyncStatusManagerImpl and LoggingIdentifierController to use StateFlow instead of AtomicReference
5 participants