From 879b8e35ad08ea8aa8fd58c485323cff132f8344 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Thu, 19 Dec 2019 16:20:54 -0800 Subject: [PATCH] Closes #5371 - Introduce 'sync' ping, add 'syncUuid' to all sync-related pings This patch introduce an 'overarching' sync ping, which is meant to contain information describing a sync overall, vs individual engine runs. Currently it contains global sync errors. syncUuid was also added to all sync-related pings, allowing us to tie together in an analysis all pings that were emitted as part of a single "sync". --- .../fxa/sync/WorkManagerSyncManager.kt | 17 +- .../sync/logins/SyncableLoginsStorage.kt | 2 +- .../support/sync-telemetry/docs/metrics.md | 16 + .../support/sync-telemetry/metrics.yaml | 41 +++ components/support/sync-telemetry/pings.yaml | 17 ++ .../support/sync/telemetry/SyncTelemetry.kt | 266 ++++++++++------ .../sync/telemetry/SyncTelemetryTest.kt | 285 +++++++++++++++++- 7 files changed, 531 insertions(+), 113 deletions(-) diff --git a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt b/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt index 50f6fc80911..f29b49c8e10 100644 --- a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt +++ b/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt @@ -410,22 +410,7 @@ class WorkManagerSyncWorker( } // Process telemetry. - syncResult.telemetry?.let { - // Yes, this is non-ideal... - // But, what this does: individual 'process' function will report global sync errors - // as part of its corresponding ping. We don't want to report a global sync error multiple times, - // so we check for the boolean flag that indicates if this happened or not. - // There's a complete mismatch between what Glean supports and what we need it to do here. - // Glean doesn't support "nested metrics" and so we resort to these hacks. - // It shouldn't matter in which order these 'process' functions are called. - var noGlobalErrorsReported = SyncTelemetry.processBookmarksPing(it) - if (noGlobalErrorsReported) { - noGlobalErrorsReported = SyncTelemetry.processHistoryPing(it) - } - if (noGlobalErrorsReported) { - SyncTelemetry.processPasswordsPing(it) - } - } + syncResult.telemetry?.let { SyncTelemetry.processSyncTelemetry(it) } // Finally, declare success, failure or request a retry based on 'sync status'. return when (syncResult.status) { diff --git a/components/service/sync-logins/src/main/java/mozilla/components/service/sync/logins/SyncableLoginsStorage.kt b/components/service/sync-logins/src/main/java/mozilla/components/service/sync/logins/SyncableLoginsStorage.kt index 48cb86aa96d..81d8bf0c9c6 100644 --- a/components/service/sync-logins/src/main/java/mozilla/components/service/sync/logins/SyncableLoginsStorage.kt +++ b/components/service/sync-logins/src/main/java/mozilla/components/service/sync/logins/SyncableLoginsStorage.kt @@ -255,7 +255,7 @@ class SyncableLoginsStorage( @Throws(SyncAuthInvalidException::class, RequestFailedException::class, LoginsStorageException::class) suspend fun sync(syncInfo: SyncUnlockInfo): SyncTelemetryPing = withContext(coroutineContext) { conn.getStorage().sync(syncInfo).also { - SyncTelemetry.processPasswordsPing(it) + SyncTelemetry.processLoginsPing(it) } } } diff --git a/components/support/sync-telemetry/docs/metrics.md b/components/support/sync-telemetry/docs/metrics.md index f0b78d958ae..324150ab4c9 100644 --- a/components/support/sync-telemetry/docs/metrics.md +++ b/components/support/sync-telemetry/docs/metrics.md @@ -10,6 +10,7 @@ This means you might have to go searching through the dependency tree to get a f - [bookmarks-sync](#bookmarks-sync) - [history-sync](#history-sync) - [logins-sync](#logins-sync) + - [sync](#sync) ## bookmarks-sync @@ -29,6 +30,7 @@ The following metrics are added to the ping: | bookmarks_sync.remote_tree_problems |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |Records counts for structure problems and divergences in the remote bookmarks tree. These are documented in https://github.com/mozilla/dogear/blob/fbade15f2a4f11215e30b8f428a0a8df3defeaec/src/tree.rs#L1273-L1294. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | | bookmarks_sync.started_at |[datetime](https://mozilla.github.io/glean/book/user/metrics/datetime.html) |Records when the bookmark sync started. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | | bookmarks_sync.uid |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |The user's hashed Firefox Account ID. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | +| sync.sync_uuid |[uuid](https://mozilla.github.io/glean/book/user/metrics/uuid.html) |Unique identifier for this sync, used to correlate together individual pings for data types that were synchronized together (history, bookmarks, logins). If a data type is synchronized by itself via the legacy 'sync' API (as opposed to the Sync Manager), then this field will not be set on the corresponding ping. |[1](https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687)||never | ## history-sync @@ -46,6 +48,7 @@ The following metrics are added to the ping: | history_sync.outgoing_batches |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |Records the number of batches needed to upload all outgoing records. The Sync server has a hard limit on the number of records (and request body bytes) on the number of records that can fit into a single batch, and large syncs may require multiple batches. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | | history_sync.started_at |[datetime](https://mozilla.github.io/glean/book/user/metrics/datetime.html) |Records when the history sync started. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | | history_sync.uid |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |The user's hashed Firefox Account ID. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | +| sync.sync_uuid |[uuid](https://mozilla.github.io/glean/book/user/metrics/uuid.html) |Unique identifier for this sync, used to correlate together individual pings for data types that were synchronized together (history, bookmarks, logins). If a data type is synchronized by itself via the legacy 'sync' API (as opposed to the Sync Manager), then this field will not be set on the corresponding ping. |[1](https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687)||never | ## logins-sync @@ -63,6 +66,19 @@ The following metrics are added to the ping: | logins_sync.outgoing_batches |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |Records the number of batches needed to upload all outgoing records. The Sync server has a hard limit on the number of records (and request body bytes) on the number of records that can fit into a single batch, and large syncs may require multiple batches. |[1](https://github.com/mozilla-mobile/android-components/pull/5294)||never | | logins_sync.started_at |[datetime](https://mozilla.github.io/glean/book/user/metrics/datetime.html) |Records when the passwords sync started. |[1](https://github.com/mozilla-mobile/android-components/pull/5294)||never | | logins_sync.uid |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |The user's hashed Firefox Account ID. |[1](https://github.com/mozilla-mobile/android-components/pull/5294)||never | +| sync.sync_uuid |[uuid](https://mozilla.github.io/glean/book/user/metrics/uuid.html) |Unique identifier for this sync, used to correlate together individual pings for data types that were synchronized together (history, bookmarks, logins). If a data type is synchronized by itself via the legacy 'sync' API (as opposed to the Sync Manager), then this field will not be set on the corresponding ping. |[1](https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687)||never | + +## sync + +A summary ping, sent every time a sync is performed. During each Sync one or more data types could be synchronized, depending on which data types user configured to sync. Alongside with 'sync' ping one or more individual data type specific pings will be sent. For example, if history and bookmarks data types are configured to be synchronized, the following pings will be sent: 'sync', 'history-sync' and 'bookmarks-sync'. Alternatively, if only history is configured to be synchronized then 'sync' and 'history-sync' pings will be sent. In case of a "global failure" where none of the data type syncs could even start, e.g. device is offline, only the 'sync' ping will be sent. This ping doesn't include the `client_id` because it reports a hashed version of the user's Firefox Account ID. + + +The following metrics are added to the ping: + +| Name | Type | Description | Data reviews | Extras | Expiration | +| --- | --- | --- | --- | --- | --- | +| sync.failure_reason |[labeled_string](https://mozilla.github.io/glean/book/user/metrics/labeled_strings.html) |Records a global sync failure: either due to an authentication error, unexpected exception, or other error that caused the sync to fail. Error strings are truncated and sanitized to omit PII, like URLs and file system paths. |[1](https://github.com/mozilla-mobile/android-components/pull/3092)||never | +| sync.sync_uuid |[uuid](https://mozilla.github.io/glean/book/user/metrics/uuid.html) |Unique identifier for this sync, used to correlate together individual pings for data types that were synchronized together (history, bookmarks, logins). If a data type is synchronized by itself via the legacy 'sync' API (as opposed to the Sync Manager), then this field will not be set on the corresponding ping. |[1](https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687)||never | diff --git a/components/support/sync-telemetry/metrics.yaml b/components/support/sync-telemetry/metrics.yaml index 48abd33bc23..66ad7197aa8 100644 --- a/components/support/sync-telemetry/metrics.yaml +++ b/components/support/sync-telemetry/metrics.yaml @@ -4,6 +4,47 @@ $schema: moz://mozilla.org/schemas/glean/metrics/1-0-0 +sync: + sync_uuid: + type: uuid + description: > + Unique identifier for this sync, used to correlate together individual pings for data types that + were synchronized together (history, bookmarks, logins). If a data type is synchronized by itself via + the legacy 'sync' API (as opposed to the Sync Manager), then this field will not be set on the corresponding ping. + send_in_pings: + - sync + - history-sync + - bookmarks-sync + - logins-sync + bugs: + - https://github.com/mozilla-mobile/android-components/issues/5371 + data_reviews: + - https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687 + notification_emails: + - sync-core@mozilla.com + expires: never + lifetime: ping + failure_reason: + type: labeled_string + labels: + - other + - unexpected + - auth + description: > + Records a global sync failure: either due to an authentication error, unexpected exception, + or other error that caused the sync to fail. Error strings are truncated and sanitized to omit + PII, like URLs and file system paths. + send_in_pings: + - sync + bugs: + - https://github.com/mozilla-mobile/android-components/pull/3092 + data_reviews: + - https://github.com/mozilla-mobile/android-components/pull/3092 + notification_emails: + - sync-core@mozilla.com + expires: never + lifetime: ping + # `history-sync`, `logins-sync` and `bookmarks-sync` metrics all use the same structure, # but must be specified individually. We can't define them once and use # `send_in_pings` because the stores might be synced in parallel, and we can't diff --git a/components/support/sync-telemetry/pings.yaml b/components/support/sync-telemetry/pings.yaml index b6ba738522a..96780574b80 100644 --- a/components/support/sync-telemetry/pings.yaml +++ b/components/support/sync-telemetry/pings.yaml @@ -4,6 +4,23 @@ $schema: moz://mozilla.org/schemas/glean/pings/1-0-0 +sync: + description: > + A summary ping, sent every time a sync is performed. During each Sync one or more data types + could be synchronized, depending on which data types user configured to sync. Alongside with 'sync' ping + one or more individual data type specific pings will be sent. For example, if history and bookmarks + data types are configured to be synchronized, the following pings will be sent: 'sync', 'history-sync' + and 'bookmarks-sync'. Alternatively, if only history is configured to be synchronized then 'sync' and 'history-sync' + pings will be sent. In case of a "global failure" where none of the data type syncs could even start, + e.g. device is offline, only the 'sync' ping will be sent. + This ping doesn't include the `client_id` because it reports a hashed version of the user's Firefox Account ID. + include_client_id: false + bugs: + - https://github.com/mozilla-mobile/android-components/issues/5371 + notification_emails: + - sync-core@mozilla.com + data_reviews: + - https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687 history-sync: description: > A ping sent for every history sync. It doesn't include the `client_id` diff --git a/components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt b/components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt index 794db95d70f..53252ebd8ea 100644 --- a/components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt +++ b/components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt @@ -4,14 +4,17 @@ package mozilla.components.support.sync.telemetry +import mozilla.appservices.sync15.EngineInfo import mozilla.appservices.sync15.FailureName import mozilla.appservices.sync15.FailureReason import mozilla.appservices.sync15.SyncTelemetryPing import mozilla.components.service.glean.private.LabeledMetricType import mozilla.components.service.glean.private.StringMetricType +import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.sync.telemetry.GleanMetrics.BookmarksSync import mozilla.components.support.sync.telemetry.GleanMetrics.HistorySync import mozilla.components.support.sync.telemetry.GleanMetrics.LoginsSync +import mozilla.components.support.sync.telemetry.GleanMetrics.Sync import mozilla.components.support.sync.telemetry.GleanMetrics.Pings const val MAX_FAILURE_REASON_LENGTH = 100 @@ -21,6 +24,62 @@ const val MAX_FAILURE_REASON_LENGTH = 100 */ @Suppress("LargeClass") object SyncTelemetry { + private val logger = Logger("SyncTelemetry") + + /** + * Process [SyncTelemetryPing] as returned from [mozilla.appservices.syncmanager.SyncManager]. + */ + fun processSyncTelemetry( + syncTelemetry: SyncTelemetryPing, + + // The following are present to make this function testable. In tests, we need to "intercept" + // values set in singleton ping objects before they're reset by a 'submit' call. + submitGlobalPing: () -> Unit = { Pings.sync.submit() }, + submitHistoryPing: () -> Unit = { Pings.historySync.submit() }, + submitBookmarksPing: () -> Unit = { Pings.bookmarksSync.submit() }, + submitLoginsPing: () -> Unit = { Pings.loginsSync.submit() } + ) { + syncTelemetry.syncs.forEach { syncInfo -> + // Note that `syncUuid` is configured to be submitted in all of the sync pings (it's set + // once, and will be attached by glean to history-sync, bookmarks-sync, and logins-sync pings). + // However, this only happens if sync telemetry is being submitted via [processSyncTelemetry]. + // That is, if different data types were synchronized together, as happens when using a sync manager. + // We can then use 'syncUuid' to associate together all of the individual syncs that happened together. + // If a data type is synchronized individually via the legacy 'sync' API on specific storage layers, + // then the corresponding ping will not have 'syncUuid' set. + Sync.syncUuid.generateAndSet() + + // It's possible for us to sync some engines, and then get a hard error that fails the + // entire sync. Examples of such errors are an HTTP server error, token authentication + // error, or other kind of network error. + // We can have some engines that succeed (and others that fail, with different reasons) + // and still have a global failure_reason. + syncInfo.failureReason?.let { failureReason -> + recordFailureReason(failureReason, Sync.failureReason) + } + + syncInfo.engines.forEach { engineInfo -> + when (engineInfo.name) { + "bookmarks" -> { + individualBookmarksSync(syncTelemetry.uid, engineInfo) + submitBookmarksPing() + } + "history" -> { + individualHistorySync(syncTelemetry.uid, engineInfo) + submitHistoryPing() + } + "passwords" -> { + individualLoginsSync(syncTelemetry.uid, engineInfo) + submitLoginsPing() + } + else -> logger.warn("Ignoring telemetry for engine ${engineInfo.name}") + } + } + + submitGlobalPing() + } + } + /** * Processes a history-related ping information from the [ping]. * @return 'false' if global error was encountered, 'true' otherwise. @@ -40,36 +99,7 @@ object SyncTelemetry { if (engine.name != "history") { return@eachEngine } - HistorySync.apply { - val base = BaseGleanSyncPing.fromEngineInfo(ping.uid, engine) - uid.set(base.uid) - startedAt.set(base.startedAt) - finishedAt.set(base.finishedAt) - if (base.applied > 0) { - // Since all Sync ping counters have `lifetime: ping`, and - // we send the ping immediately after, we don't need to - // reset the counters before calling `add`. - incoming["applied"].add(base.applied) - } - if (base.failedToApply > 0) { - incoming["failed_to_apply"].add(base.failedToApply) - } - if (base.reconciled > 0) { - incoming["reconciled"].add(base.reconciled) - } - if (base.uploaded > 0) { - outgoing["uploaded"].add(base.uploaded) - } - if (base.failedToUpload > 0) { - outgoing["failed_to_upload"].add(base.failedToUpload) - } - if (base.outgoingBatches > 0) { - outgoingBatches.add(base.outgoingBatches) - } - base.failureReason?.let { - recordFailureReason(it, failureReason) - } - } + individualHistorySync(ping.uid, engine) sendPing() } } @@ -81,7 +111,7 @@ object SyncTelemetry { * @return 'false' if global error was encountered, 'true' otherwise. */ @Suppress("ComplexMethod", "NestedBlockDepth") - fun processPasswordsPing( + fun processLoginsPing( ping: SyncTelemetryPing, sendPing: () -> Unit = { Pings.loginsSync.submit() } ): Boolean { @@ -95,36 +125,7 @@ object SyncTelemetry { if (engine.name != "passwords") { return@eachEngine } - LoginsSync.apply { - val base = BaseGleanSyncPing.fromEngineInfo(ping.uid, engine) - uid.set(base.uid) - startedAt.set(base.startedAt) - finishedAt.set(base.finishedAt) - if (base.applied > 0) { - // Since all Sync ping counters have `lifetime: ping`, and - // we send the ping immediately after, we don't need to - // reset the counters before calling `add`. - incoming["applied"].add(base.applied) - } - if (base.failedToApply > 0) { - incoming["failed_to_apply"].add(base.failedToApply) - } - if (base.reconciled > 0) { - incoming["reconciled"].add(base.reconciled) - } - if (base.uploaded > 0) { - outgoing["uploaded"].add(base.uploaded) - } - if (base.failedToUpload > 0) { - outgoing["failed_to_upload"].add(base.failedToUpload) - } - if (base.outgoingBatches > 0) { - outgoingBatches.add(base.outgoingBatches) - } - base.failureReason?.let { - recordFailureReason(it, failureReason) - } - } + individualLoginsSync(ping.uid, engine) sendPing() } } @@ -156,44 +157,123 @@ object SyncTelemetry { if (engine.name != "bookmarks") { return@eachEngine } - BookmarksSync.apply { - val base = BaseGleanSyncPing.fromEngineInfo(ping.uid, engine) - uid.set(base.uid) - startedAt.set(base.startedAt) - finishedAt.set(base.finishedAt) - if (base.applied > 0) { - incoming["applied"].add(base.applied) - } - if (base.failedToApply > 0) { - incoming["failed_to_apply"].add(base.failedToApply) - } - if (base.reconciled > 0) { - incoming["reconciled"].add(base.reconciled) - } - if (base.uploaded > 0) { - outgoing["uploaded"].add(base.uploaded) - } - if (base.failedToUpload > 0) { - outgoing["failed_to_upload"].add(base.failedToUpload) - } - if (base.outgoingBatches > 0) { - outgoingBatches.add(base.outgoingBatches) - } - base.failureReason?.let { - recordFailureReason(it, failureReason) - } - engine.validation?.let { - it.problems.forEach { - remoteTreeProblems[it.name].add(it.count) - } - } - } + individualBookmarksSync(ping.uid, engine) sendPing() } } return true } + @Suppress("ComplexMethod") + private fun individualLoginsSync(hashedFxaUid: String, engineInfo: EngineInfo) { + require(engineInfo.name == "passwords") { "Expected 'passwords', got ${engineInfo.name}" } + + LoginsSync.apply { + val base = BaseGleanSyncPing.fromEngineInfo(hashedFxaUid, engineInfo) + uid.set(base.uid) + startedAt.set(base.startedAt) + finishedAt.set(base.finishedAt) + if (base.applied > 0) { + // Since all Sync ping counters have `lifetime: ping`, and + // we send the ping immediately after, we don't need to + // reset the counters before calling `add`. + incoming["applied"].add(base.applied) + } + if (base.failedToApply > 0) { + incoming["failed_to_apply"].add(base.failedToApply) + } + if (base.reconciled > 0) { + incoming["reconciled"].add(base.reconciled) + } + if (base.uploaded > 0) { + outgoing["uploaded"].add(base.uploaded) + } + if (base.failedToUpload > 0) { + outgoing["failed_to_upload"].add(base.failedToUpload) + } + if (base.outgoingBatches > 0) { + outgoingBatches.add(base.outgoingBatches) + } + base.failureReason?.let { + recordFailureReason(it, failureReason) + } + } + } + + @Suppress("ComplexMethod") + private fun individualBookmarksSync(hashedFxaUid: String, engineInfo: EngineInfo) { + require(engineInfo.name == "bookmarks") { "Expected 'bookmarks', got ${engineInfo.name}" } + + BookmarksSync.apply { + val base = BaseGleanSyncPing.fromEngineInfo(hashedFxaUid, engineInfo) + uid.set(base.uid) + startedAt.set(base.startedAt) + finishedAt.set(base.finishedAt) + if (base.applied > 0) { + incoming["applied"].add(base.applied) + } + if (base.failedToApply > 0) { + incoming["failed_to_apply"].add(base.failedToApply) + } + if (base.reconciled > 0) { + incoming["reconciled"].add(base.reconciled) + } + if (base.uploaded > 0) { + outgoing["uploaded"].add(base.uploaded) + } + if (base.failedToUpload > 0) { + outgoing["failed_to_upload"].add(base.failedToUpload) + } + if (base.outgoingBatches > 0) { + outgoingBatches.add(base.outgoingBatches) + } + base.failureReason?.let { + recordFailureReason(it, failureReason) + } + engineInfo.validation?.let { + it.problems.forEach { problemInfo -> + remoteTreeProblems[problemInfo.name].add(problemInfo.count) + } + } + } + } + + @Suppress("ComplexMethod") + private fun individualHistorySync(hashedFxaUid: String, engineInfo: EngineInfo) { + require(engineInfo.name == "history") { "Expected 'history', got ${engineInfo.name}" } + + HistorySync.apply { + val base = BaseGleanSyncPing.fromEngineInfo(hashedFxaUid, engineInfo) + uid.set(base.uid) + startedAt.set(base.startedAt) + finishedAt.set(base.finishedAt) + if (base.applied > 0) { + // Since all Sync ping counters have `lifetime: ping`, and + // we send the ping immediately after, we don't need to + // reset the counters before calling `add`. + incoming["applied"].add(base.applied) + } + if (base.failedToApply > 0) { + incoming["failed_to_apply"].add(base.failedToApply) + } + if (base.reconciled > 0) { + incoming["reconciled"].add(base.reconciled) + } + if (base.uploaded > 0) { + outgoing["uploaded"].add(base.uploaded) + } + if (base.failedToUpload > 0) { + outgoing["failed_to_upload"].add(base.failedToUpload) + } + if (base.outgoingBatches > 0) { + outgoingBatches.add(base.outgoingBatches) + } + base.failureReason?.let { + recordFailureReason(it, failureReason) + } + } + } + private fun recordFailureReason(reason: FailureReason, failureReasonMetric: LabeledMetricType) { val metric = when (reason.name) { FailureName.Other, FailureName.Unknown -> failureReasonMetric["other"] diff --git a/components/support/sync-telemetry/src/test/java/mozilla/components/support/sync/telemetry/SyncTelemetryTest.kt b/components/support/sync-telemetry/src/test/java/mozilla/components/support/sync/telemetry/SyncTelemetryTest.kt index 1873aa7959f..cc3687911d3 100644 --- a/components/support/sync-telemetry/src/test/java/mozilla/components/support/sync/telemetry/SyncTelemetryTest.kt +++ b/components/support/sync-telemetry/src/test/java/mozilla/components/support/sync/telemetry/SyncTelemetryTest.kt @@ -19,16 +19,19 @@ import mozilla.components.support.sync.telemetry.GleanMetrics.BookmarksSync import mozilla.components.support.sync.telemetry.GleanMetrics.LoginsSync import mozilla.components.support.sync.telemetry.GleanMetrics.HistorySync import mozilla.components.support.sync.telemetry.GleanMetrics.Pings +import mozilla.components.support.sync.telemetry.GleanMetrics.Sync import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull import org.junit.Assert.fail import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import java.util.Date +import java.util.UUID private fun Date.asSeconds() = time / BaseGleanSyncPing.MILLIS_PER_SEC @@ -122,6 +125,7 @@ class SyncTelemetryTest { assertEquals(14, outgoing["uploaded"].testGetValue()) assertEquals(7, outgoing["failed_to_upload"].testGetValue()) assertEquals(2, outgoingBatches.testGetValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } } 1 -> { @@ -137,6 +141,7 @@ class SyncTelemetryTest { outgoing["failed_to_upload"], outgoingBatches ).none { it.testHasValue() }) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } } else -> fail() @@ -259,26 +264,31 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } 2 -> HistorySync.apply { assertEquals("Unexpected error: 418", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } 3 -> HistorySync.apply { assertEquals("Splines not reticulated", failureReason["auth"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["unexpected"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } 4 -> HistorySync.apply { assertEquals("Kaboom!", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } 5 -> HistorySync.apply { assertEquals("Qualia unsynchronized", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } else -> fail() } @@ -312,6 +322,7 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("history-sync")) } else -> fail() } @@ -327,7 +338,7 @@ class SyncTelemetryTest { @Test fun `sends passwords telemetry pings on success`() { - val noGlobalError = SyncTelemetry.processPasswordsPing(SyncTelemetryPing( + val noGlobalError = SyncTelemetry.processLoginsPing(SyncTelemetryPing( version = 1, uid = "abc123", syncs = listOf( @@ -406,6 +417,7 @@ class SyncTelemetryTest { assertEquals(14, outgoing["uploaded"].testGetValue()) assertEquals(7, outgoing["failed_to_upload"].testGetValue()) assertEquals(2, outgoingBatches.testGetValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } } 1 -> { @@ -421,6 +433,7 @@ class SyncTelemetryTest { outgoing["failed_to_upload"], outgoingBatches ).none { it.testHasValue() }) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } } else -> fail() @@ -437,7 +450,7 @@ class SyncTelemetryTest { @Test fun `sends passwords telemetry pings on engine failure`() { - val noGlobalError = SyncTelemetry.processPasswordsPing(SyncTelemetryPing( + val noGlobalError = SyncTelemetry.processLoginsPing(SyncTelemetryPing( version = 1, uid = "abc123", syncs = listOf( @@ -543,26 +556,31 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } 2 -> LoginsSync.apply { assertEquals("Unexpected error: 418", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } 3 -> LoginsSync.apply { assertEquals("Splines not reticulated", failureReason["auth"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["unexpected"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } 4 -> LoginsSync.apply { assertEquals("Kaboom!", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } 5 -> LoginsSync.apply { assertEquals("Qualia unsynchronized", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } else -> fail() } @@ -578,7 +596,7 @@ class SyncTelemetryTest { @Test fun `sends passwords telemetry pings on sync failure`() { - val noGlobalError = SyncTelemetry.processPasswordsPing(SyncTelemetryPing( + val noGlobalError = SyncTelemetry.processLoginsPing(SyncTelemetryPing( version = 1, uid = "abc123", syncs = listOf( @@ -596,6 +614,7 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("logins-sync")) } else -> fail() } @@ -664,6 +683,7 @@ class SyncTelemetryTest { assertEquals(10, outgoing["uploaded"].testGetValue()) assertEquals(5, outgoing["failed_to_upload"].testGetValue()) assertEquals(1, outgoingBatches.testGetValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } } else -> fail() @@ -775,26 +795,31 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } 2 -> BookmarksSync.apply { assertEquals("Unexpected error: 418", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } 3 -> BookmarksSync.apply { assertEquals("Splines not reticulated", failureReason["auth"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["unexpected"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } 4 -> BookmarksSync.apply { assertEquals("Kaboom!", failureReason["unexpected"].testGetValue()) assertFalse(failureReason["other"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } 5 -> BookmarksSync.apply { assertEquals("Qualia unsynchronized", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } else -> fail() } @@ -828,6 +853,7 @@ class SyncTelemetryTest { assertEquals("Synergies not aligned", failureReason["other"].testGetValue()) assertFalse(failureReason["unexpected"].testHasValue()) assertFalse(failureReason["auth"].testHasValue()) + assertFalse(Sync.syncUuid.testHasValue("bookmarks-sync")) } else -> fail() } @@ -840,4 +866,257 @@ class SyncTelemetryTest { assertEquals(1, pingCount) assertFalse(noGlobalError) } + + @Test + fun `sends a global sync ping alongside individual data type pings`() { + val pings = mutableListOf>(HashMap()) + var globalPingCount = 0 + val globalSyncUuids = mutableListOf() + + val syncTelemetry = SyncTelemetryPing( + version = 1, + uid = "abc123", + syncs = listOf( + SyncInfo( + at = now, + took = 10000, + engines = listOf( + EngineInfo( + name = "passwords", + at = now, + took = 5000, + incoming = IncomingInfo( + applied = 5, + failed = 4, + newFailed = 3, + reconciled = 2 + ), + outgoing = listOf( + OutgoingInfo( + sent = 10, + failed = 5 + ), + OutgoingInfo( + sent = 4, + failed = 2 + ) + ), + failureReason = null, + validation = null + ), + EngineInfo( + name = "history", + at = now, + took = 5000, + incoming = IncomingInfo( + applied = 5, + failed = 4, + newFailed = 3, + reconciled = 2 + ), + outgoing = listOf( + OutgoingInfo( + sent = 10, + failed = 5 + ), + OutgoingInfo( + sent = 4, + failed = 2 + ) + ), + failureReason = null, + validation = null + ) + ), + failureReason = FailureReason(FailureName.Unknown, "Synergies not aligned") + ), + SyncInfo( + at = now + 10, + took = 5000, + engines = listOf( + EngineInfo( + name = "history", + at = now + 10, + took = 5000, + incoming = null, + outgoing = emptyList(), + failureReason = null, + validation = null + ) + ), + failureReason = null + ), + SyncInfo( + at = now + 20, + took = 8000, + engines = listOf( + EngineInfo( + name = "bookmarks", + at = now + 25, + took = 6000, + incoming = null, + outgoing = listOf( + OutgoingInfo( + sent = 10, + failed = 5 + ) + ), + failureReason = null, + validation = ValidationInfo( + version = 2, + problems = listOf( + ProblemInfo( + name = "missingParents", + count = 5 + ), + ProblemInfo( + name = "missingChildren", + count = 7 + ) + ), + failureReason = null + ) + ) + ), + failureReason = null + ) + ), + events = emptyList() + ) + + fun setOrAssertGlobalSyncUuid(currentPingIndex: Int, pingName: String) { + if (globalSyncUuids.elementAtOrNull(currentPingIndex) == null) { + globalSyncUuids.add(Sync.syncUuid.testGetValue(pingName)) + } else { + assertEquals(globalSyncUuids[currentPingIndex], Sync.syncUuid.testGetValue(pingName)) + } + } + + fun setOrIncrementPingCount(currentPingIndex: Int, pingName: String) { + if (pings.elementAtOrNull(currentPingIndex) == null) { + pings.add(mutableMapOf(pingName to 1)) + } else { + pings[currentPingIndex].incrementForKey(pingName) + } + } + + SyncTelemetry.processSyncTelemetry( + syncTelemetry, + submitGlobalPing = { + assertNotNull(globalSyncUuids.elementAtOrNull(globalPingCount)) + assertTrue(Sync.syncUuid.testHasValue("sync")) + assertEquals(globalSyncUuids[globalPingCount], Sync.syncUuid.testGetValue("sync")) + + // Assertions above already assert syncUuid; below, let's make sure that 'failureReason' is processed. + when (globalPingCount) { + 0 -> { + assertEquals("Synergies not aligned", Sync.failureReason["other"].testGetValue()) + } + 1 -> { + assertFalse(Sync.failureReason["other"].testHasValue()) + } + 2 -> { + assertFalse(Sync.failureReason["other"].testHasValue()) + } + else -> fail() + } + + Pings.sync.submit() + globalPingCount++ + }, + submitHistoryPing = { + when (val currentPingIndex = globalPingCount) { + 0 -> { + setOrAssertGlobalSyncUuid(currentPingIndex, "history-sync") + setOrIncrementPingCount(currentPingIndex, "history") + HistorySync.apply { + assertEquals("abc123", uid.testGetValue()) + assertEquals(now, startedAt.testGetValue().asSeconds()) + assertEquals(now + 5, finishedAt.testGetValue().asSeconds()) + assertEquals(5, incoming["applied"].testGetValue()) + assertEquals(7, incoming["failed_to_apply"].testGetValue()) + assertEquals(2, incoming["reconciled"].testGetValue()) + assertEquals(14, outgoing["uploaded"].testGetValue()) + assertEquals(7, outgoing["failed_to_upload"].testGetValue()) + assertEquals(2, outgoingBatches.testGetValue()) + } + Pings.historySync.submit() + } + 1 -> { + setOrAssertGlobalSyncUuid(currentPingIndex, "history-sync") + setOrIncrementPingCount(currentPingIndex, "history") + HistorySync.apply { + assertEquals("abc123", uid.testGetValue()) + assertEquals(now + 10, startedAt.testGetValue().asSeconds()) + assertEquals(now + 15, finishedAt.testGetValue().asSeconds()) + assertTrue(listOf( + incoming["applied"], + incoming["failed_to_apply"], + incoming["reconciled"], + outgoing["uploaded"], + outgoing["failed_to_upload"], + outgoingBatches + ).none { it.testHasValue() }) + } + Pings.historySync.submit() + } + else -> fail() + } + }, + submitLoginsPing = { + when (val currentPingIndex = globalPingCount) { + 0 -> { + setOrAssertGlobalSyncUuid(currentPingIndex, "logins-sync") + setOrIncrementPingCount(currentPingIndex, "passwords") + LoginsSync.apply { + assertEquals("abc123", uid.testGetValue()) + assertEquals(now, startedAt.testGetValue().asSeconds()) + assertEquals(now + 5, finishedAt.testGetValue().asSeconds()) + assertEquals(5, incoming["applied"].testGetValue()) + assertEquals(7, incoming["failed_to_apply"].testGetValue()) + assertEquals(2, incoming["reconciled"].testGetValue()) + assertEquals(14, outgoing["uploaded"].testGetValue()) + assertEquals(7, outgoing["failed_to_upload"].testGetValue()) + assertEquals(2, outgoingBatches.testGetValue()) + } + Pings.loginsSync.submit() + } + else -> fail() + } + }, + submitBookmarksPing = { + when (val currentPingIndex = globalPingCount) { + 2 -> { + setOrAssertGlobalSyncUuid(currentPingIndex, "bookmarks-sync") + setOrIncrementPingCount(currentPingIndex, "bookmarks") + BookmarksSync.apply { + assertEquals("abc123", uid.testGetValue()) + assertEquals(now + 25, startedAt.testGetValue().asSeconds()) + assertEquals(now + 31, finishedAt.testGetValue().asSeconds()) + assertFalse(incoming["applied"].testHasValue()) + assertFalse(incoming["failed_to_apply"].testHasValue()) + assertFalse(incoming["reconciled"].testHasValue()) + assertEquals(10, outgoing["uploaded"].testGetValue()) + assertEquals(5, outgoing["failed_to_upload"].testGetValue()) + assertEquals(1, outgoingBatches.testGetValue()) + } + Pings.bookmarksSync.submit() + } + } + } + ) + + assertEquals( + listOf( + mapOf("history" to 1, "passwords" to 1), + mapOf("history" to 1), + mapOf("bookmarks" to 1) + ), + pings + ) + } + + private fun MutableMap.incrementForKey(key: String) { + this[key] = 1 + this.getOrElse(key, { 0 }) + } } \ No newline at end of file