Skip to content

Commit

Permalink
Fixes #4897: Follow up alpha MR6 fixes (#4896)
Browse files Browse the repository at this point in the history
## Explanation
Fixes #4897

### Overview of changes

This PR follows up from the changes introduced in #4846 by adding new
functionality to help improve event reliability investigations, as well
as telemetry diagnostics for research project facilitators that need to
ensure events have been successfully uploaded for processing. To help
with this, the following features have been introduced:
- A new "force upload" button that immediately uploads any pending
cached events.
- Event counts to show both the number of events waiting to be uploaded
& the events that are successfully uploaded.
- An improvement on the 'share' button to include a Base64 deflated
binary representation of the entire event log store on the device.

Leading up to, and during, development, a number of theoretical issues
were encountered that were also fixed in case they are playing a role in
some of the ongoing concerns around Firebase event delivery reliability:
- The sync status reported on the learner analytics screen can be wrong
in some cases, leading to the assumption that events have been uploaded
when they may not have been.
- Events can, in some cases, be lost from previous app instances if they
haven't been uploaded yet and, in other cases, duplicated. The former is
much more likely to happen in practice, particularly with the
connectivity patterns being used in the research project.

### Technical specifics

- ``FakeSyncStatusManager`` was renamed to ``TestSyncStatusManager``
since it's actually augmenting the production implementation of
``SyncStatusManager``, not replacing it.
- Events are now being cached indefinitely if the learner study feature
is enabled. This allows for some robustness improvements (see below),
and the ability to share the events themselves if the team suspects
events are not arriving to Firebase.
- The issue of sync status being incorrect came from an unreliable means
of reporting sync status during different event scenarios. Specifically,
if the ``LogUploadWorker`` doesn't kick off and there's network
connectivity, a new event being logged could trigger the sync status to
become "data uploaded" even though there are still pending events. The
new solution is to leverage the now-tracked uploaded events to determine
whether there are still events yet waiting to upload. In this way, the
sync status should always be the most up-to-date information with one
caveat: network connectivity changes won't reflect until a new log is
requested (this is a limitation in how the app tracks network
connectivity, and it didn't seem very important to fix it).
- The issue of possible event duplication comes from
``PersistentCacheStore`` using ``mergeFrom`` with the *current* cached
state and the on-disk proto being loaded. I originally thought that all
pathways to loading the proto from disk could only execute once, but
there's at least one pathway where it's possible to chain multiple calls
into the cache store such that the load happens twice (and both results
end up being merged together, hence the event duplication). This has
been fixed by never reloading the cache store if it's already been
loaded which should always be correct.
- The issue if possible events being lost comes from
``AnalyticsController`` previously not priming the event log cache. This
means that if a new event is logged before the cache is read (e.g. early
events like "open profile screen" can race against ``LogUploadWorker``
kicking off on app startup) then the cache will be completely reset and,
once written, will overwrite any events currently cached on disk. This
has been fixed by ensuring that the event log cache has been primed
before any interactions with it (read or write) occur.

Note that all of the above has been thoroughly tested through automated
testing to ensure that these issues have been properly fixed, and stay
that way in the long-term.

### Test notes & exemptions

- In order to simplify testing ``SyncStatusManagerImpl`` and
``TestSyncStatusManager``, they ought to both pass a common test suite
specific to ``SyncStatusManager`` itself. This has been done by
introducing a ``SyncStatusManagerTestBase`` class that's inherited by
both implementation test suites so that they inherit common API tests.
This pattern is used in one other location in the codebase currently:
https://github.com/oppia/oppia-android/blob/0290ef8037f798d8ba2721441612a80dec4a1f1a/testing/src/test/java/org/oppia/android/testing/threading/TestCoroutineDispatcherTestBase.kt#L33
- ``CircularProgressIndicatorAdaptersTestActivity`` has been exempted
for an a11y label for the same reason as all other test activities: it
technically doesn't need one.
- ``TestSyncStatusManagerTest`` has been allow-listed to use
parameterized tests since it's aggressively verifying many different
force/override sync status cases. Making sure that the test utility
works correctly is extremely important to avoid false failures/passes of
dependent tests (which are, in turn, guarding against regressions for
the core issues covered by this PR, among other event system behaviors).
- ``SyncStatusManagerTestBase`` has been exempted from requiring KDocs
since it's technically a test suite, so KDocs aren't required.
- A number of classes have been exempted from having new test suites as
per existing conventions and requirements: ``ControlButtonsViewModel``
(renamed from ``ShareIdsViewModel``),
``CircularProgressIndicatorAdaptersTestActivity`` (simple test
activities don't require their own tests),
``CircularProgressIndicatorAdaptersTestViewModel`` (view models aren't
tested directly), ``SyncStatusManagerTestBase`` (the test base is more
or less a test suite itself, so it doesn't make sense for it to have
tests as well).
- ``TestSyncStatusManagerTest`` has been disabled in Gradle since it
depends on ``SyncStatusManagerTestBase`` which is part of a different
module. To make this work, the test base would have to be moved to
src/main of the testing module which isn't a very clean approach. The
current setup works fine with Bazel, and longer term this will get
cleaner by moving testing utilities like ``TestSyncStatusManager`` to be
near their corresponding production implementations (we just can't do
this today because of Gradle limitations).
- Due to the asynchronous and data race nature of some of the issues
discovered during test writing, changed & added tests were sometimes
found to be flaky (and, in some cases, very rarely, e.g. 1/50). To
ensure that all of the new & changed tests are stable, I made sure to
run all 8 affected test suites 128 times to ensure stability. Here are
the results of that run ([buildbuddy
link](https://app.buildbuddy.io/invocation/5da310e1-3cc9-451b-a1d0-951a3322c8d7)):

![128xruns_followup_mr6_fixes](https://user-images.githubusercontent.com/12983742/224290120-5bb2991d-59c1-421a-8b73-e05838734253.png)


## 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

Following is a video demonstrating the new functionality introduced in
this PR:


https://user-images.githubusercontent.com/12983742/224289397-73c5e3a7-4f7a-4116-8e71-3316e30fa9d7.mp4

Note that affected UI tests that could run on Espresso were **not**
verified in this PR similar for the reasons listed in #4846
(specifically, the ongoing issue of Espresso tests being time consuming
to investigate, often broken, and this PR being high priority).

Localization & accessibility changes aren't relevant for this change,
and any tablet-specific issues aren't important since the affected
screen is only expected to be accessed on handset devices.
  • Loading branch information
BenHenning authored Mar 10, 2023
1 parent 0290ef8 commit 1addc8f
Show file tree
Hide file tree
Showing 64 changed files with 4,583 additions and 842 deletions.
4 changes: 3 additions & 1 deletion app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ DATABINDING_LAYOUTS = ["src/main/res/layout*/**"]
# keep sorted
VIEW_MODELS_WITH_RESOURCE_IMPORTS = [
"src/main/java/org/oppia/android/app/administratorcontrols/appversion/AppVersionViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/ControlButtonsViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/DeviceIdItemViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/ProfileLearnerIdItemViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/ProfileListViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/ShareIdsViewModel.kt",
"src/main/java/org/oppia/android/app/administratorcontrols/learneranalytics/SyncStatusItemViewModel.kt",
"src/main/java/org/oppia/android/app/devoptions/forcenetworktype/ForceNetworkTypeViewModel.kt",
"src/main/java/org/oppia/android/app/devoptions/mathexpressionparser/MathExpressionParserViewModel.kt",
Expand Down Expand Up @@ -344,6 +344,7 @@ VIEW_MODELS = [
"src/main/java/org/oppia/android/app/story/StoryViewModel.kt",
"src/main/java/org/oppia/android/app/testing/BindableAdapterTestDataModel.kt",
"src/main/java/org/oppia/android/app/testing/BindableAdapterTestViewModel.kt",
"src/main/java/org/oppia/android/app/testing/CircularProgressIndicatorAdaptersTestViewModel.kt",
"src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardViewModel.kt",
"src/main/java/org/oppia/android/app/topic/lessons/TopicLessonViewModel.kt",
"src/main/java/org/oppia/android/app/topic/lessons/TopicLessonsItemViewModel.kt",
Expand Down Expand Up @@ -472,6 +473,7 @@ BINDING_ADAPTERS_WITH_RESOURCE_IMPORTS = [
# keep sorted
BINDING_ADAPTERS = [
"src/main/java/org/oppia/android/app/databinding/AppCompatCheckBoxBindingAdapters.java",
"src/main/java/org/oppia/android/app/databinding/CircularProgressIndicatorAdapters.java",
"src/main/java/org/oppia/android/app/databinding/ConstraintLayoutAdapters.java",
"src/main/java/org/oppia/android/app/databinding/EditTextBindingAdapters.java",
"src/main/java/org/oppia/android/app/databinding/GuidelineBindingAdapters.java",
Expand Down
1 change: 1 addition & 0 deletions app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@
<activity android:name=".app.testing.AppCompatCheckBoxBindingAdaptersTestActivity" />
<activity android:name=".app.testing.AudioFragmentTestActivity" />
<activity android:name=".app.testing.BindableAdapterTestActivity" />
<activity android:name=".app.testing.CircularProgressIndicatorAdaptersTestActivity" />
<activity android:name=".app.testing.ConceptCardFragmentTestActivity" />
<activity android:name=".app.testing.DragDropTestActivity" />
<activity android:name=".app.testing.DrawableBindingAdaptersTestActivity" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import org.oppia.android.app.story.StoryActivity
import org.oppia.android.app.testing.AdministratorControlsFragmentTestActivity
import org.oppia.android.app.testing.AppCompatCheckBoxBindingAdaptersTestActivity
import org.oppia.android.app.testing.AudioFragmentTestActivity
import org.oppia.android.app.testing.CircularProgressIndicatorAdaptersTestActivity
import org.oppia.android.app.testing.ConceptCardFragmentTestActivity
import org.oppia.android.app.testing.DragDropTestActivity
import org.oppia.android.app.testing.DrawableBindingAdaptersTestActivity
Expand Down Expand Up @@ -123,6 +124,7 @@ interface ActivityComponentImpl :
fun inject(appVersionActivity: AppVersionActivity)
fun inject(audioFragmentTestActivity: AudioFragmentTestActivity)
fun inject(audioLanguageActivity: AudioLanguageActivity)
fun inject(circularProgressAdaptersTestActivity: CircularProgressIndicatorAdaptersTestActivity)
fun inject(completedStoryListActivity: CompletedStoryListActivity)
fun inject(conceptCardFragmentTestActivity: ConceptCardFragmentTestActivity)
fun inject(developerOptionsActivity: DeveloperOptionsActivity)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
package org.oppia.android.app.administratorcontrols.learneranalytics

import android.annotation.SuppressLint
import android.content.ActivityNotFoundException
import android.content.Intent
import androidx.appcompat.app.AppCompatActivity
import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.Transformations
import com.google.protobuf.MessageLite
import org.oppia.android.app.administratorcontrols.learneranalytics.ProfileListViewModel.ProfileListItemViewModel
import org.oppia.android.app.model.OppiaEventLogs
import org.oppia.android.domain.oppialogger.OppiaLogger
import org.oppia.android.domain.oppialogger.analytics.AnalyticsController
import org.oppia.android.util.data.AsyncResult
import org.oppia.android.util.data.DataProviders.Companion.toLiveData
import org.oppia.android.util.logging.SyncStatusManager
import org.oppia.android.util.logging.SyncStatusManager.SyncStatus
import java.io.ByteArrayOutputStream
import java.util.Base64
import java.util.zip.GZIPOutputStream
import javax.inject.Inject

/**
* [ProfileListItemViewModel] that represents the portion of the learner analytics admin page which
* provides some control buttons for ID/logs sharing and syncing.
*/
@SuppressLint("StaticFieldLeak") // False positive (Android doesn't manage this model's lifecycle).
class ControlButtonsViewModel private constructor(
private val oppiaLogger: OppiaLogger,
private val activity: AppCompatActivity,
private val analyticsController: AnalyticsController,
private val syncStatusManager: SyncStatusManager,
private val viewModels: List<ProfileListItemViewModel>
) : ProfileListItemViewModel(ProfileListViewModel.ProfileListItemViewType.SHARE_IDS) {
private var monitoredUploadProgress: LiveData<ForceSyncProgress> =
MutableLiveData(ForceSyncProgress())

/** The current [ForceSyncProgress], as affected by [onUploadLogsNowButtonClicked]. */
val forceUploadProgress: LiveData<ForceSyncProgress> get() = monitoredUploadProgress

/** The current [SyncStatus] of event logs. */
val syncStatus: LiveData<SyncStatus> by lazy {
Transformations.map(syncStatusManager.getSyncStatus().toLiveData(), this::processSyncStatus)
}

/**
* Indicates the user wants to share learner & device IDs, along with event log metrics, with
* another app.
*/
fun onShareButtonClicked() {
// Reference: https://developer.android.com/guide/components/intents-common#Email from
// https://stackoverflow.com/a/15022153/3689782.
val logs = retrieveEventLogs(viewModels.filterIsInstance<ProfileLearnerIdItemViewModel>())
val sharedText = viewModels.mapNotNull { viewModel ->
when (viewModel) {
is DeviceIdItemViewModel -> listOf("Oppia app installation ID: ${viewModel.deviceId.value}")
is ProfileLearnerIdItemViewModel -> {
val profile = viewModel.profile
val stats = viewModel.profileSpecificEventsUploadStats.value
val learnerStats = stats?.learnerStats
val uncategorizedStats = stats?.uncategorizedStats
listOfNotNull(
"- Profile name: ${profile.name}, learner ID: ${profile.learnerId}",
learnerStats?.awaitingUploadEventCountText?.let { " - Uploading learner events: $it" },
learnerStats?.uploadedEventCountText?.let { " - Uploaded learner events: $it" },
uncategorizedStats?.awaitingUploadEventCountText?.let {
" - Uploading uncategorized events: $it"
},
uncategorizedStats?.uploadedEventCountText?.let {
" - Uploaded uncategorized events: $it"
}
)
}
is SyncStatusItemViewModel -> {
listOf(
"Current sync status: ${viewModel.syncStatus.value}.",
"Encoded event logs:"
) + (logs?.toCompressedBase64()?.chunked(BASE64_LINE_WRAP_LIMIT) ?: listOf("Missing"))
}
else -> null
}
}.flatten().joinToString(separator = "\n")
try {
activity.startActivity(
Intent(Intent.ACTION_SEND).apply {
type = "text/plain"
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
putExtra(Intent.EXTRA_TEXT, sharedText)
}
)
} catch (e: ActivityNotFoundException) {
oppiaLogger.e("ControlButtonsViewModel", "No activity found to receive shared IDs.", e)
}
}

/**
* Indicates that the user wishes to upload any pending event logs immediately, rather than
* waiting for the app to schedule an event upload.
*
* The status of the upload result can be observed via [forceUploadProgress].
*
* [canStartUploadingLogs] should be used to determine whether now is a valid time to start
* uploading logs.
*/
fun onUploadLogsNowButtonClicked() {
monitoredUploadProgress =
Transformations.map(
analyticsController.uploadEventLogs().toLiveData(), this::processUploadedEventLogs
)
notifyChange() // Recompute bindings since the live data instance has changed.
}

/**
* Returns whether current conditions, per [isCurrentlyUploading] and [syncStatus], indicate that
* there are events that can be manually force-uploaded by the user.
*
* Note that both [isCurrentlyUploading] and [syncStatus] permit null values because of an issue
* with databinding wherein multiple [LiveData]s bound at the same time may result in one or both
* of the values correspond to the *current* [LiveData] value which may be null if the [LiveData]
* hasn't been updated yet (even if its type indicates non-nullness). Permitting null allows the
* implementation to work around this case by assuming that either value missing means no
* determination can be made whether logs can start being uploaded.
*/
fun canStartUploadingLogs(isCurrentlyUploading: Boolean?, syncStatus: SyncStatus?): Boolean =
isCurrentlyUploading == false && syncStatus == SyncStatus.WAITING_TO_START_UPLOADING

private fun retrieveEventLogs(viewModels: List<ProfileLearnerIdItemViewModel>): OppiaEventLogs? =
viewModels.firstOrNull()?.oppiaEventLogs?.value?.let { processEventLogs(it) }

private fun processUploadedEventLogs(result: AsyncResult<Pair<Int, Int>>): ForceSyncProgress {
return when (result) {
is AsyncResult.Pending -> ForceSyncProgress(eventsUploaded = 0, totalEventsToUpload = 0)
is AsyncResult.Success -> {
val (currentUploadedEventCount, totalEventCount) = result.value
ForceSyncProgress(currentUploadedEventCount, totalEventCount)
}
is AsyncResult.Failure -> ForceSyncProgress().also {
oppiaLogger.e(
"ControlButtonsViewModel", "Encountered failure while uploading events.", result.error
)
}
}
}

private fun processSyncStatus(result: AsyncResult<SyncStatus>): SyncStatus {
return when (result) {
is AsyncResult.Pending -> SyncStatus.INITIAL_UNKNOWN
is AsyncResult.Success -> result.value
is AsyncResult.Failure -> SyncStatus.INITIAL_UNKNOWN.also {
oppiaLogger.e(
"ControlButtonsViewModel",
"Encountered failure while retrieving sync status.",
result.error
)
}
}
}

private fun processEventLogs(result: AsyncResult<OppiaEventLogs>): OppiaEventLogs? {
return when (result) {
is AsyncResult.Pending -> null
is AsyncResult.Success -> result.value
is AsyncResult.Failure -> {
oppiaLogger.e(
"ControlButtonsViewModel", "Encountered failure while on-disk event logs.", result.error
)
null
}
}
}

/**
* Progress representation structure that provides the UI with an indication of how much of an
* upload event operation has completed, and how much remains.
*
* Note that a [totalEventsToUpload] value of 0 indicates there are 0 left in the **current**
* operation, but there may have been events to upload in the past.
*
* @property eventsUploaded the number of events that have uploaded so far
* @property totalEventsToUpload the total number of events to upload, or none, or
* [DEFAULT_UNKNOWN_EVENTS_TO_UPLOAD_COUNT] if it's not yet known how many events can be
* uploaded
*/
data class ForceSyncProgress(
val eventsUploaded: Int = 0,
val totalEventsToUpload: Int = DEFAULT_UNKNOWN_EVENTS_TO_UPLOAD_COUNT
) {
/** Returns whether there are events that can be uploaded. */
fun hasEventsToUpload(): Boolean = totalEventsToUpload != DEFAULT_UNKNOWN_EVENTS_TO_UPLOAD_COUNT

/** Returns whether there are events currently being uploaded. */
fun isCurrentlyUploading(): Boolean = eventsUploaded < totalEventsToUpload
}

/** Factory for creating new [ControlButtonsViewModel]s. */
class Factory @Inject constructor(
private val oppiaLogger: OppiaLogger,
private val activity: AppCompatActivity,
private val analyticsController: AnalyticsController,
private val syncStatusManager: SyncStatusManager
) {
/** Returns a new [ControlButtonsViewModel]. */
fun create(viewModels: List<ProfileListItemViewModel>): ControlButtonsViewModel {
return ControlButtonsViewModel(
oppiaLogger, activity, analyticsController, syncStatusManager, viewModels
)
}
}

private companion object {
private const val BASE64_LINE_WRAP_LIMIT = 80
private const val DEFAULT_UNKNOWN_EVENTS_TO_UPLOAD_COUNT = Integer.MIN_VALUE

// Copied from ProtoStringEncoder (which isn't available in production code).
private fun <M : MessageLite> M.toCompressedBase64(): String {
val compressedMessage = ByteArrayOutputStream().also { byteOutputStream ->
GZIPOutputStream(byteOutputStream).use(::writeTo)
}.toByteArray()
return Base64.getEncoder().encodeToString(compressedMessage)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import org.oppia.android.app.administratorcontrols.learneranalytics.ProfileListV
import org.oppia.android.app.administratorcontrols.learneranalytics.ProfileListViewModel.ProfileListItemViewType
import org.oppia.android.app.recyclerview.BindableAdapter
import org.oppia.android.databinding.ProfileAndDeviceIdFragmentBinding
import org.oppia.android.databinding.ProfileListControlButtonsBinding
import org.oppia.android.databinding.ProfileListDeviceIdItemBinding
import org.oppia.android.databinding.ProfileListLearnerIdItemBinding
import org.oppia.android.databinding.ProfileListShareIdsItemBinding
import org.oppia.android.databinding.ProfileListSyncStatusItemBinding
import javax.inject.Inject

Expand Down Expand Up @@ -47,7 +47,7 @@ class ProfileAndDeviceIdFragmentPresenter @Inject constructor(
is DeviceIdItemViewModel -> ProfileListItemViewType.DEVICE_ID
is ProfileLearnerIdItemViewModel -> ProfileListItemViewType.LEARNER_ID
is SyncStatusItemViewModel -> ProfileListItemViewType.SYNC_STATUS
is ShareIdsViewModel -> ProfileListItemViewType.SHARE_IDS
is ControlButtonsViewModel -> ProfileListItemViewType.SHARE_IDS
else -> error("Encountered unexpected view model: $viewModel")
}
}.registerViewDataBinder(
Expand All @@ -67,9 +67,9 @@ class ProfileAndDeviceIdFragmentPresenter @Inject constructor(
transformViewModel = { it as SyncStatusItemViewModel }
).registerViewDataBinder(
viewType = ProfileListItemViewType.SHARE_IDS,
inflateDataBinding = ProfileListShareIdsItemBinding::inflate,
setViewModel = ProfileListShareIdsItemBinding::setViewModel,
transformViewModel = { it as ShareIdsViewModel }
inflateDataBinding = ProfileListControlButtonsBinding::inflate,
setViewModel = ProfileListControlButtonsBinding::setViewModel,
transformViewModel = { it as ControlButtonsViewModel }
).build()
}
}
Loading

0 comments on commit 1addc8f

Please sign in to comment.