Include logins in the Fenix Sync Ping

[linabutler]

We send pings for history and bookmark syncs now, but not logins. With the Fennec to Fenix transition, Fenix will start syncing logins soon, so we'll want to send pings for logins, too.

I think this might be a change that we make purely in a-c...as far as I remember, sync15_passwords_sync already returns the ping (we just need to make sure it does that for errors, too!), but the a-c logins component doesn't unpack that into a Glean ping.

+@grigoryk

┆Issue is synchronized with this Jira Story
┆Story Points: 3
┆Epic: Logins Component 1.0
┆Sprint: SYNC - end 2019-12-20

[data-sync-user]

➤ Janet Dragojevic commented:

From Grisha: {color:#1d1c1d}  {color}support-sync-telemetrycomponent in ac should simplify the process of sending sync telemetry.  This should also enable us to resolve SYNC331 at the same time.  He also noted that:

{quote}the data review process will be quite weird here; we may need to expose more granular control over telemetry that these sync components submit, so that products do not just automatically inherit telemetry; otherwise, we land something in ac with a datareview that covers fenix, and lockwise is suddenly submitting these pings as well; in case of sync, that's exactly what we'd like, but it's not a great model for telemetry
{quote}

[rfk]

I did a little dive here to see what all the moving parts are. IIUC here's what would be involved in this change:

• In the support-sync-telemetry android component:
  • Copy-pasta the history_sync metrics definitions and rename them to logins_sync. Request what is hopefully a straightforward data-review for these new metrics.
  • Copy-pasta SyncTelemetry.processHistoryPing and rename it to processLoginsPing, and copy-pasta some testcases to match.
• In the browser-storage-sync android component:
  • Add a wrapper to expose the new processLoginsPing method
• In service-firefox-accounts android component:
  • Update WorkManagerSyncManager to add a call to processLoginsPing next to the existing calls to processHistoryPing and processBookmarksPing (and laugh-cry at the comment therein that opens with "Yes, this is complete garbage").

I think that's everything, since above that the existing sync-manager is in charge of driving the sync and obtaining the telemetry ping from rust code. @linacambridge does that sound about right?

Also the code above as a TODO linking to this a-c issue which is more-or-less a dupe: mozilla-mobile/android-components#4556

[linabutler]

Yep, that's right!

and laugh-cry at the comment therein that opens with "Yes, this is complete garbage"

😂😭🤪🙎‍♀️

[rfk]

Copy-pasta the history_sync metrics definitions and rename them to logins_sync.
Request what is hopefully a straightforward data-review for these new metrics.

Based on the discussion in #2325 and friends, it's actually not very clear to me what the data-review requirements are here. I guess we need to get approval for the new metrics and there integration into all current users of android-components sync infrastructure.

[grigoryk]

I'll land the ping & request a data review for it in mozilla-mobile/android-components#4556.

Regarding data reviews for integration - the current state of the world is such that the only time to meaningfully do this is when a consumer of a-c bumps the a-c version to one which includes new telemetry. Even that isn't very clearcut - Fenix Nightly is using snapshot builds of a-c, which will simply auto-enable the telemetry whenever a-c snapshot, generated nightly, will include the telemetry.

I don't think we need to solve this problem now (let's just consider single data review in a-c as enough, since this new ping is very non-controversial), but rather catalogue this as an example of why the current setup isn't right.

[rfk]

the current state of the world is such that the only time to meaningfully do this is when a
consumer of a-c bumps the a-c version to one which includes new telemetry

My read of https://bugzilla.mozilla.org/show_bug.cgi?id=1597895#c14 and related comments is that we factor in known impact on known consumers at the time of doing the review for the change itself in a-c, and do not need to do a separate review for existing consumers of the component.

[grigoryk]

Happening in mozilla-mobile/android-components#5294

[data-sync-user]

➤ Ryan Kelly commented:

This has r+ over in mozilla-mobile/android-components#5294, ( mozilla-mobile/android-components#5294, ) I think we can safely close it here.