Merge #4480

4480: Closes #2727: Rust SyncManager integration r=csadilek a=grigoryk Fixes #2727. This has most of the pieces in the right places. I think all of the necessary public API changes are done. It mostly works, except that I've hit some bugs in the rust library: - mozilla/application-services#1831 (blocker) - mozilla/application-services#1829 (minor) Fenix PR that enables "choose what to sync" on top of this: mozilla-mobile/fenix#5450 Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
mozilla-mobile · Sep 27, 2019 · 83ffd01 · 83ffd01
2 parents 4bd8ede + d9abf6e
commit 83ffd01
Show file tree

Hide file tree

Showing 46 changed files with 1,955 additions and 1,171 deletions.
diff --git a/.buildconfig.yml b/.buildconfig.yml
@@ -272,6 +272,10 @@ projects:
     path: components/support/utils
     description: 'A collection of generic helper classes.'
     publish: true
+  support-sync-telemetry:
+    path: components/support/sync-telemetry
+    description: 'A collection of sync-related telemetry helper classes and ping descriptions.'
+    publish: true
   support-rustlog:
     path: components/support/rustlog
     description: 'A bridge allowing log messages from Rust code to be sent to the log system in support-base'

diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt
@@ -27,7 +27,7 @@ object Versions {
     const val disklrucache = "2.0.2"
     const val leakcanary = "1.6.3"
 
-    const val mozilla_appservices = "0.39.1"
+    const val mozilla_appservices = "0.40.0"
 
     const val material = "1.0.0"
 
@@ -122,6 +122,7 @@ object Dependencies {
 
     const val mozilla_sync_logins = "org.mozilla.appservices:logins:${Versions.mozilla_appservices}"
     const val mozilla_places = "org.mozilla.appservices:places:${Versions.mozilla_appservices}"
+    const val mozilla_sync_manager = "org.mozilla.appservices:syncmanager:${Versions.mozilla_appservices}"
 
     const val mozilla_push = "org.mozilla.appservices:push:${Versions.mozilla_appservices}"
 

diff --git a/components/browser/storage-sync/README.md b/components/browser/storage-sync/README.md
@@ -5,7 +5,7 @@ A syncable implementation of `concept-storage` backed by [application-services'
 ## Before using this component
 Products sending telemetry and using this component *must request* a data-review following [this process](https://wiki.mozilla.org/Firefox/Data_Collection).
 This component provides data collection using the [Glean SDK](https://mozilla.github.io/glean/book/index.html).
-The list of metrics being collected is available in the [metrics documentation](docs/metrics.md).
+The list of metrics being collected is available in the [metrics documentation](../../support/telemetry/docs/metrics.md).
 
 ### Setting up the dependency
 

diff --git a/components/browser/storage-sync/build.gradle b/components/browser/storage-sync/build.gradle
@@ -2,10 +2,6 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-plugins {
-    id "com.jetbrains.python.envs" version "0.0.26"
-}
-
 apply plugin: 'com.android.library'
 apply plugin: 'kotlin-android'
 
@@ -50,9 +46,8 @@ dependencies {
     api project(':concept-storage')
     api project(':concept-sync')
 
-    implementation project(':service-glean')
     implementation project(':support-utils')
-    implementation Dependencies.mozilla_sync15
+    implementation project(':support-sync-telemetry')
 
     implementation Dependencies.kotlin_stdlib
 
@@ -68,13 +63,9 @@ dependencies {
 
     testImplementation Dependencies.mozilla_places
     testImplementation Dependencies.testing_mockwebserver
-    testImplementation Dependencies.androidx_work_testing
 
     testImplementation Dependencies.mozilla_full_megazord_forUnitTests
 }
 
 apply from: '../../../publish.gradle'
 ext.configurePublish(config.componentsGroupId, archivesBaseName, project.ext.description)
-
-ext.gleanGenerateMarkdownDocs = true
-apply from: '../../service/glean/scripts/sdk_generator.gradle'
diff --git a/.../browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt b/.../browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt
@@ -8,18 +8,12 @@ import androidx.annotation.GuardedBy
 import mozilla.appservices.places.PlacesApi
 import mozilla.appservices.places.PlacesReaderConnection
 import mozilla.appservices.places.PlacesWriterConnection
-import mozilla.appservices.sync15.FailureName
-import mozilla.appservices.sync15.FailureReason
-import mozilla.appservices.sync15.SyncTelemetryPing
-import mozilla.components.browser.storage.sync.GleanMetrics.BookmarksSync
-import mozilla.components.browser.storage.sync.GleanMetrics.HistorySync
-import mozilla.components.browser.storage.sync.GleanMetrics.Pings
 import mozilla.components.concept.sync.SyncAuthInfo
+import mozilla.components.support.sync.telemetry.SyncTelemetry
 import java.io.Closeable
 import java.io.File
 
 const val DB_NAME = "places.sqlite"
-const val MAX_FAILURE_REASON_LENGTH = 100
 
 /**
  * A slight abstraction over [PlacesApi].
@@ -31,123 +25,20 @@ const val MAX_FAILURE_REASON_LENGTH = 100
  * Writer is always the same, as guaranteed by [PlacesApi].
  */
 internal interface Connection : Closeable {
+    /**
+     * This should be removed. See: https://github.com/mozilla/application-services/issues/1877
+     *
+     * @return raw internal handle that could be used for referencing underlying [PlacesApi]. Use it with SyncManager.
+     */
+    fun getHandle(): Long
+
     fun reader(): PlacesReaderConnection
     fun writer(): PlacesWriterConnection
 
     // Until we get a real SyncManager in application-services libraries, we'll have to live with this
     // strange split that doesn't quite map all that well to our internal storage model.
     fun syncHistory(syncInfo: SyncAuthInfo)
     fun syncBookmarks(syncInfo: SyncAuthInfo)
-
-    // These are implemented as default methods on `Connection` instead of
-    // `RustPlacesConnection` to make testing easier.
-    @Suppress("ComplexMethod", "NestedBlockDepth")
-    fun assembleHistoryPing(ping: SyncTelemetryPing) {
-        ping.syncs.forEach eachSync@{ sync ->
-            sync.failureReason?.let {
-                recordHistoryFailureReason(it)
-                sendHistoryPing()
-                return@eachSync
-            }
-            sync.engines.forEach eachEngine@{ engine ->
-                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 {
-                        recordHistoryFailureReason(it)
-                    }
-                }
-                sendHistoryPing()
-            }
-        }
-    }
-
-    @Suppress("EmptyFunctionBlock")
-    fun sendHistoryPing() {}
-
-    // This function is almost identical to `recordHistoryPing`, with additional
-    // reporting for validation problems. Unfortunately, since the
-    // `BookmarksSync` and `HistorySync` metrics are two separate objects, we
-    // can't factor this out into a generic function.
-    @Suppress("ComplexMethod", "NestedBlockDepth")
-    fun assembleBookmarksPing(ping: SyncTelemetryPing) {
-        ping.syncs.forEach eachSync@{ sync ->
-            sync.failureReason?.let {
-                // If the entire sync fails, don't try to unpack the ping; just
-                // report the error and bail.
-                recordBookmarksFailureReason(it)
-                sendBookmarksPing()
-                return@eachSync
-            }
-            sync.engines.forEach eachEngine@{ engine ->
-                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 {
-                        recordBookmarksFailureReason(it)
-                    }
-                    engine.validation?.let {
-                        it.problems.forEach {
-                            remoteTreeProblems[it.name].add(it.count)
-                        }
-                    }
-                }
-                sendBookmarksPing()
-            }
-        }
-    }
-
-    @Suppress("EmptyFunctionBlock")
-    fun sendBookmarksPing() {}
 }
 
 /**
@@ -173,6 +64,11 @@ internal object RustPlacesConnection : Connection {
         cachedReader = api!!.openReader()
     }
 
+    override fun getHandle(): Long {
+        check(api != null) { "must call init first" }
+        return api!!.getHandle()
+    }
+
     override fun reader(): PlacesReaderConnection = synchronized(this) {
         check(cachedReader != null) { "must call init first" }
         return cachedReader!!
@@ -186,21 +82,13 @@ internal object RustPlacesConnection : Connection {
     override fun syncHistory(syncInfo: SyncAuthInfo) {
         check(api != null) { "must call init first" }
         val ping = api!!.syncHistory(syncInfo.into())
-        assembleHistoryPing(ping)
-    }
-
-    override fun sendHistoryPing() {
-        Pings.historySync.send()
+        SyncTelemetry.processHistoryPing(ping)
     }
 
     override fun syncBookmarks(syncInfo: SyncAuthInfo) {
         check(api != null) { "must call init first" }
         val ping = api!!.syncBookmarks(syncInfo.into())
-        assembleBookmarksPing(ping)
-    }
-
-    override fun sendBookmarksPing() {
-        Pings.bookmarksSync.send()
+        SyncTelemetry.processBookmarksPing(ping)
     }
 
     override fun close() = synchronized(this) {
@@ -209,25 +97,3 @@ internal object RustPlacesConnection : Connection {
         api = null
     }
 }
-
-private fun recordHistoryFailureReason(reason: FailureReason) {
-    val metric = when (reason.name) {
-        FailureName.Other, FailureName.Unknown -> HistorySync.failureReason["other"]
-        FailureName.Unexpected, FailureName.Http -> HistorySync.failureReason["unexpected"]
-        FailureName.Auth -> HistorySync.failureReason["auth"]
-        FailureName.Shutdown -> return
-    }
-    val message = reason.message ?: "Unexpected error: ${reason.code}"
-    metric.set(message.take(MAX_FAILURE_REASON_LENGTH))
-}
-
-private fun recordBookmarksFailureReason(reason: FailureReason) {
-    val metric = when (reason.name) {
-        FailureName.Other, FailureName.Unknown -> BookmarksSync.failureReason["other"]
-        FailureName.Unexpected, FailureName.Http -> BookmarksSync.failureReason["unexpected"]
-        FailureName.Auth -> BookmarksSync.failureReason["auth"]
-        FailureName.Shutdown -> return
-    }
-    val message = reason.message ?: "Unexpected error: ${reason.code}"
-    metric.set(message.take(MAX_FAILURE_REASON_LENGTH))
-}
diff --git a/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt b/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt
@@ -184,4 +184,13 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
             }
         }
     }
+
+    /**
+     * This should be removed. See: https://github.com/mozilla/application-services/issues/1877
+     *
+     * @return raw internal handle that could be used for referencing underlying [PlacesApi]. Use it with SyncManager.
+     */
+    override fun getHandle(): Long {
+        return places.getHandle()
+    }
 }
diff --git a/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
@@ -6,6 +6,7 @@ package mozilla.components.browser.storage.sync
 
 import android.content.Context
 import kotlinx.coroutines.withContext
+import mozilla.appservices.places.PlacesApi
 import mozilla.appservices.places.VisitObservation
 import mozilla.components.concept.storage.HistoryAutocompleteResult
 import mozilla.components.concept.storage.HistoryStorage
@@ -177,4 +178,13 @@ open class PlacesHistoryStorage(context: Context) : PlacesStorage(context), Hist
             }
         }
     }
+
+    /**
+     * This should be removed. See: https://github.com/mozilla/application-services/issues/1877
+     *
+     * @return raw internal handle that could be used for referencing underlying [PlacesApi]. Use it with SyncManager.
+     */
+    override fun getHandle(): Long {
+        return places.getHandle()
+    }
 }
diff --git a/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt b/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt
@@ -6,9 +6,6 @@
 package mozilla.components.browser.storage.sync
 
 import mozilla.appservices.places.SyncAuthInfo
-import java.util.Date
-import mozilla.appservices.sync15.EngineInfo
-import mozilla.appservices.sync15.FailureReason
 import mozilla.components.concept.storage.VisitInfo
 import mozilla.components.concept.storage.VisitType
 
@@ -64,51 +61,3 @@ internal fun mozilla.appservices.places.VisitInfo.into(): VisitInfo {
         visitType = this.visitType.into()
     )
 }
-
-/**
- * Holds fields common to all Glean sync engine pings.
- */
-internal data class BaseGleanSyncPing(
-    val uid: String,
-    val startedAt: Date,
-    val finishedAt: Date,
-    val applied: Int,
-    val failedToApply: Int,
-    val reconciled: Int,
-    val uploaded: Int,
-    val failedToUpload: Int,
-    val outgoingBatches: Int,
-    val failureReason: FailureReason?
-) {
-    companion object {
-        const val MILLIS_PER_SEC = 1000L
-
-        fun fromEngineInfo(uid: String, info: EngineInfo): BaseGleanSyncPing {
-            val failedToApply = info.incoming?.let {
-                it.failed + it.newFailed
-            } ?: 0
-            val (uploaded, failedToUpload) = info.outgoing.fold(Pair(0, 0)) { totals, batch ->
-                val (uploaded, failedToUpload) = totals
-                Pair(uploaded + batch.sent, failedToUpload + batch.failed)
-            }
-            return BaseGleanSyncPing(
-                uid = uid,
-                startedAt = Date(info.at.toLong() * MILLIS_PER_SEC),
-                // Glean intentionally doesn't support recording arbitrary
-                // durations in timespans, and we can't use the timespan
-                // measurement API because everything is measured in Rust
-                // code. Instead, we record absolute start and end times.
-                // The Sync ping records both `at` _and_ `took`, so this doesn't
-                // leak additional info.
-                finishedAt = Date(info.at.toLong() * MILLIS_PER_SEC + info.took),
-                applied = info.incoming?.applied ?: 0,
-                failedToApply = failedToApply,
-                reconciled = info.incoming?.reconciled ?: 0,
-                uploaded = uploaded,
-                failedToUpload = failedToUpload,
-                outgoingBatches = info.outgoing.size,
-                failureReason = info.failureReason
-            )
-        }
-    }
-}