Merge #7618

7618: Record send-tab telemetry for Fenix. r=grigoryk a=mhammond

application-services needs to be closely involved in creating the
data to be recorded in telemetry - however, we are trying to avoid
exposing this telemetry directly to the android-components "concepts"
or "features" - for example, the part of android-components that sends
a tab ideally wouldn't have to deal with any interface changes just because
telemetry is being sent or changed - otherwise new/changed telemetry is always
a "breaking change"

In the future, we expect the rust components to interact directly with Glean,
and consumers like android-components wouldn't need to get involved at all.
However, until then...

The idea with this PR is that there's one new public account function,
`processPendingTelemetry()`, which should be called whenever something is
done which might record telemetry. However, it doesn't need to know
what is actually recorded - that knowledge is only in the telemetry
code - the code that is tightly bound to glean.

This hasn't been tested yet - but it builds :) I'm soliciting feedback on
the general shape of this before I invest too much more in tests etc.

I also considered trying to do with Observers(), but that didn't seem
any easier. I'm obviously open to alternative approaches!

See also mozilla/application-services#3308



Co-authored-by: Mark Hammond <[email protected]>

Author: MozLando

buildSrc/src/main/java/Dependencies.kt

@@ -27,7 +27,7 @@ object Versions {
     const val disklrucache = "2.0.2"
     const val leakcanary = "2.4"
 
-    const val mozilla_appservices = "61.0.10"
+    const val mozilla_appservices = "61.0.12"
 
     const val mozilla_glean = "32.1.0"
 

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt

@@ -17,6 +17,7 @@ import mozilla.components.concept.sync.AuthFlowUrl
 import mozilla.components.concept.sync.DeviceConstellation
 import mozilla.components.concept.sync.OAuthAccount
 import mozilla.components.concept.sync.StatePersistenceCallback
+import mozilla.components.support.base.crash.CrashReporting
 import mozilla.components.support.base.log.logger.Logger
 import org.json.JSONObject
 
@@ -27,7 +28,8 @@ typealias PersistCallback = mozilla.appservices.fxaclient.FirefoxAccount.Persist
  */
 @Suppress("TooManyFunctions")
 class FirefoxAccount internal constructor(
-    private val inner: InternalFxAcct
+    private val inner: InternalFxAcct,
+    private val crashReporter: CrashReporting? = null
 ) : OAuthAccount {
     private val job = SupervisorJob()
     private val scope = CoroutineScope(Dispatchers.IO) + job
@@ -65,7 +67,7 @@ class FirefoxAccount internal constructor(
     }
 
     private var persistCallback = WrappingPersistenceCallback()
-    private val deviceConstellation = FxaDeviceConstellation(inner, scope)
+    private val deviceConstellation = FxaDeviceConstellation(inner, scope, crashReporter)
 
     init {
         inner.registerPersistCallback(persistCallback)
@@ -82,13 +84,16 @@ class FirefoxAccount internal constructor(
      * is saved in a secure location, as it can contain Sync Keys and
      * OAuth tokens.
      *
+     * @param crashReporter A crash reporter instance.
+     *
      * Note that it is not necessary to `close` the Config if this constructor is used (however
      * doing so will not cause an error).
      */
     constructor(
         config: ServerConfig,
-        persistCallback: PersistCallback? = null
-    ) : this(InternalFxAcct(config, persistCallback))
+        persistCallback: PersistCallback? = null,
+        crashReporter: CrashReporting? = null
+    ) : this(InternalFxAcct(config, persistCallback), crashReporter)
 
     override fun close() {
         job.cancel()

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FxaDeviceConstellation.kt

@@ -22,17 +22,20 @@ import mozilla.components.concept.sync.AccountEventsObserver
 import mozilla.components.concept.sync.DeviceCommandOutgoing
 import mozilla.components.concept.sync.DevicePushSubscription
 import mozilla.components.concept.sync.DeviceType
+import mozilla.components.support.base.crash.CrashReporting
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
+import mozilla.components.support.sync.telemetry.SyncTelemetry
 
 /**
  * Provides an implementation of [DeviceConstellation] backed by a [FirefoxAccount].
  */
 @SuppressWarnings("TooManyFunctions")
 class FxaDeviceConstellation(
     private val account: FirefoxAccount,
-    private val scope: CoroutineScope
+    private val scope: CoroutineScope,
+    private val crashReporter: CrashReporting? = null
 ) : DeviceConstellation, Observable<AccountEventsObserver> by ObserverRegistry() {
     private val logger = Logger("FxaDeviceConstellation")
 
@@ -111,6 +114,7 @@ class FxaDeviceConstellation(
                 when (outgoingCommand) {
                     is DeviceCommandOutgoing.SendTab -> {
                         account.sendSingleTab(targetDeviceId, outgoingCommand.title, outgoingCommand.url)
+                        SyncTelemetry.processFxaTelemetry(account.gatherTelemetry(), crashReporter)
                     }
                     else -> logger.debug("Skipped sending unsupported command type: $outgoingCommand")
                 }
@@ -130,6 +134,7 @@ class FxaDeviceConstellation(
                 false
             } else {
                 processEvents(events)
+                SyncTelemetry.processFxaTelemetry(account.gatherTelemetry(), crashReporter)
                 true
             }
         }

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt

@@ -1015,7 +1015,7 @@ open class FxaAccountManager(
 
     @VisibleForTesting
     open fun createAccount(config: ServerConfig): OAuthAccount {
-        return FirefoxAccount(config)
+        return FirefoxAccount(config, null, crashReporter)
     }
 
     @VisibleForTesting

components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaDeviceConstellationTest.kt

@@ -167,6 +167,7 @@ class FxaDeviceConstellationTest {
 
     @Test
     fun `send command to device`() = runBlocking(coroutinesTestRule.testDispatcher) {
+        `when`(account.gatherTelemetry()).thenReturn("{}")
         assertTrue(constellation.sendCommandToDeviceAsync(
             "targetID", DeviceCommandOutgoing.SendTab("Mozilla", "https://www.mozilla.org")
         ).await())
@@ -236,6 +237,7 @@ class FxaDeviceConstellationTest {
     @ExperimentalCoroutinesApi
     fun `polling for commands triggers observers`() = runBlocking(coroutinesTestRule.testDispatcher) {
         // No commands, no observers.
+        `when`(account.gatherTelemetry()).thenReturn("{}")
         `when`(account.pollDeviceCommands()).thenReturn(emptyArray())
         assertTrue(constellation.pollForCommandsAsync().await())
 

components/support/sync-telemetry/build.gradle

@@ -44,6 +44,7 @@ dependencies {
 
     implementation project(':service-glean')
     implementation Dependencies.kotlin_stdlib
+    implementation Dependencies.kotlin_coroutines
     implementation Dependencies.mozilla_sync15
 
     testImplementation Dependencies.testing_robolectric

components/support/sync-telemetry/docs/metrics.md

@@ -109,6 +109,8 @@ The following metrics are added to the ping:
 
 | Name | Type | Description | Data reviews | Extras | Expiration | [Data Sensitivity](https://wiki.mozilla.org/Firefox/Data_Collection) |
 | --- | --- | --- | --- | --- | --- | --- |
+| fxa_tab.received |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |Recorded when a tab is received.  Also sent by desktop - see also the docs at https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/data/sync-ping.html  |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1652902)|<ul><li>flow_id: A guid, unique for the URL being sent but common for all target devices. </li><li>reason: The reason we discovered the tab. Will be one of 'push', 'push-missed' or 'poll'. </li><li>stream_id: A guid, unique for both the URL being sent and the target device. </li></ul>|never | |
+| fxa_tab.sent |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |Recorded when a tab is sent. Also sent by desktop - see also the docs at https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/data/sync-ping.html  |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1652902)|<ul><li>flow_id: A guid, unique for the URL being sent but common for all target devices. The value is randomly generated so can not identify details about the user or tab. </li><li>stream_id: A guid, unique for both the URL being sent and the target device. The value is randomly generated so can not identify details about the user or tab. </li></ul>|never | |
 | 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)|<ul><li>other</li><li>unexpected</li><li>auth</li></ul>|never |2 |
 | 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 |1 |
 

components/support/sync-telemetry/metrics.yaml

@@ -491,3 +491,58 @@ bookmarks_sync:
       - [email protected]
     expires: never
     lifetime: ping
+fxa_tab:
+  sent:
+    type: event
+    description: >
+      Recorded when a tab is sent. Also sent by desktop - see also the docs at
+      https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/data/sync-ping.html
+    send_in_pings:
+      - sync
+    extra_keys:
+      flow_id:
+        description: >
+          A guid, unique for the URL being sent but common for all target
+          devices. The value is randomly generated so can not identify details
+          about the user or tab.
+      stream_id:
+        description: >
+          A guid, unique for both the URL being sent and the target device. The
+          value is randomly generated so can not identify details about the
+          user or tab.
+    bugs:
+      - https://github.com/mozilla/application-services/pull/3308
+      - https://github.com/mozilla-mobile/android-components/pull/7618
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1652902
+    notification_emails:
+      - [email protected]
+    expires: never
+  received:
+    type: event
+    description: >
+      Recorded when a tab is received.  Also sent by desktop - see also the
+      docs at
+      https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/data/sync-ping.html
+    send_in_pings:
+      - sync
+    extra_keys:
+      flow_id:
+        description: >
+          A guid, unique for the URL being sent but common for all target
+          devices.
+      stream_id:
+        description: >
+          A guid, unique for both the URL being sent and the target device.
+      reason:
+        description: >
+          The reason we discovered the tab. Will be one of 'push', 'push-missed'
+          or 'poll'.
+    bugs:
+      - https://github.com/mozilla/application-services/pull/3308
+      - https://github.com/mozilla-mobile/android-components/pull/7618
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1652902
+    notification_emails:
+      - [email protected]
+    expires: never

components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt

@@ -4,21 +4,33 @@
 
 package mozilla.components.support.sync.telemetry
 
+import org.json.JSONException
+import org.json.JSONObject
 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.crash.CrashReporting
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.sync.telemetry.GleanMetrics.BookmarksSync
+import mozilla.components.support.sync.telemetry.GleanMetrics.FxaTab
 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
 
+// The exceptions we report to the crash reporter, but otherwise don't escape this module.
+internal sealed class InvalidTelemetryException(cause: Exception) : Exception(cause) {
+    // The top-level data passed in is invalid.
+    class InvalidData(cause: JSONException) : InvalidTelemetryException(cause)
+    // The sent or received tabs data is invalid.
+    class InvalidEvents(cause: JSONException) : InvalidTelemetryException(cause)
+}
+
 /**
  * Contains functionality necessary to process instances of [SyncTelemetryPing].
  */
@@ -284,4 +296,45 @@ object SyncTelemetry {
         val message = reason.message ?: "Unexpected error: ${reason.code}"
         metric.set(message.take(MAX_FAILURE_REASON_LENGTH))
     }
+
+    fun processFxaTelemetry(jsonStr: String, crashReporter: CrashReporting? = null) {
+        val json = try {
+            JSONObject(jsonStr)
+        } catch (e: JSONException) {
+            crashReporter?.submitCaughtException(InvalidTelemetryException.InvalidData(e))
+            logger.error("Invalid JSON in FxA telemetry", e)
+            return
+        }
+        try {
+            val sent = json.getJSONArray("commands_sent")
+            for (i in 0..sent.length() - 1) {
+                val one = sent.getJSONObject(i)
+                val extras = mapOf(
+                    FxaTab.sentKeys.flowId to one.getString("flow_id"),
+                    FxaTab.sentKeys.streamId to one.getString("stream_id")
+                )
+                FxaTab.sent.record(extras)
+            }
+            logger.info("Reported telemetry for ${sent.length()} sent commands")
+        } catch (e: JSONException) {
+            crashReporter?.submitCaughtException(InvalidTelemetryException.InvalidEvents(e))
+            logger.error("Failed to report sent commands", e)
+        }
+        try {
+            val recd = json.getJSONArray("commands_received")
+            for (i in 0..recd.length() - 1) {
+                val one = recd.getJSONObject(i)
+                val extras = mapOf(
+                    FxaTab.receivedKeys.flowId to one.getString("flow_id"),
+                    FxaTab.receivedKeys.streamId to one.getString("stream_id"),
+                    FxaTab.receivedKeys.reason to one.getString("reason")
+                )
+                FxaTab.received.record(extras)
+            }
+            logger.info("Reported telemetry for ${recd.length()} received commands")
+        } catch (e: JSONException) {
+            crashReporter?.submitCaughtException(InvalidTelemetryException.InvalidEvents(e))
+            logger.error("Failed to report received commands", e)
+        }
+    }
 }

components/support/sync-telemetry/src/test/java/mozilla/components/support/sync/telemetry/SyncTelemetryTest.kt

@@ -15,12 +15,17 @@ import mozilla.appservices.sync15.SyncInfo
 import mozilla.appservices.sync15.SyncTelemetryPing
 import mozilla.appservices.sync15.ValidationInfo
 import mozilla.components.service.glean.testing.GleanTestRule
+import mozilla.components.support.base.crash.CrashReporting
 import mozilla.components.support.sync.telemetry.GleanMetrics.BookmarksSync
+import mozilla.components.support.sync.telemetry.GleanMetrics.FxaTab
 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.any
+import mozilla.components.support.test.mock
 import mozilla.components.support.test.robolectric.testContext
+import org.json.JSONException
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Assert.assertFalse
@@ -30,6 +35,8 @@ import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mockito.verify
+import org.mockito.Mockito.times
 import java.util.Date
 import java.util.UUID
 
@@ -1116,6 +1123,75 @@ class SyncTelemetryTest {
         )
     }
 
+    @Test
+    fun `checks sent tab telemetry records what it should`() {
+        val json = """
+            {
+                "commands_sent":[{
+                    "flow_id":"test-flow-id",
+                    "stream_id":"test-stream-id"
+                }],
+                "commands_received":[]
+            }
+        """
+        SyncTelemetry.processFxaTelemetry(json)
+        assertTrue(FxaTab.sent.testHasValue())
+        val events = FxaTab.sent.testGetValue()
+        assertEquals(1, events.size)
+        assertEquals("test-flow-id", events.elementAt(0).extra!!["flow_id"])
+        assertEquals("test-stream-id", events.elementAt(0).extra!!["stream_id"])
+
+        assertFalse(FxaTab.received.testHasValue())
+    }
+
+    @Test
+    fun `checks received tab telemetry records what it should`() {
+        val json = """
+            {
+                "commands_received":[{
+                    "flow_id":"test-flow-id",
+                    "stream_id":"test-stream-id",
+                    "reason":"test-reason"
+                }]
+            }
+        """
+        SyncTelemetry.processFxaTelemetry(json)
+        assertTrue(FxaTab.received.testHasValue())
+        val events = FxaTab.received.testGetValue()
+        assertEquals(1, events.size)
+        assertEquals("test-flow-id", events.elementAt(0).extra!!["flow_id"])
+        assertEquals("test-stream-id", events.elementAt(0).extra!!["stream_id"])
+        assertEquals("test-reason", events.elementAt(0).extra!!["reason"])
+
+        assertFalse(FxaTab.sent.testHasValue())
+    }
+
+    @Test
+    fun `checks invalid tab telemetry doesn't record anything and doesn't crash`() {
+        val crashReporter: CrashReporting = mock()
+        // commands_sent is missing the stream_id, command_received is missing a reason
+        val json = """
+            {
+                "commands_sent":[{
+                    "flow_id":"test-flow-id"
+                }],
+                "commands_received":[{
+                    "flow_id":"test-flow-id",
+                    "stream_id":"test-stream-id"
+                }]
+            }
+        """
+        SyncTelemetry.processFxaTelemetry(json, crashReporter)
+        // one exception for each of 'send' and 'received'
+        verify(crashReporter, times(2)).submitCaughtException(any<JSONException>())
+        // completely invalid json
+        SyncTelemetry.processFxaTelemetry(""" foo bar """, crashReporter)
+        assertFalse(FxaTab.sent.testHasValue())
+        assertFalse(FxaTab.received.testHasValue())
+        // One more exception, making it the 3rd time
+        verify(crashReporter, times(3)).submitCaughtException(any<JSONException>())
+    }
+
     private fun MutableMap<String, Int>.incrementForKey(key: String) {
         this[key] = 1 + this.getOrElse(key, { 0 })
     }