Repository navigation
Include logins in the Fenix Sync Ping #1867
Description
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.
┆Issue is synchronized with this Jira Story
┆Story Points: 3
┆Epic: Logins Component 1.0
┆Sprint: SYNC - end 2019-12-20
Metadata
Metadata
Assignees
Labels
No labels
Type
Fields
Priority
None yet
Activity
[-]Submit a Sync ping for logins[/-][+]Submit a Sync ping in Fenix[/+]data-sync-user commentedon Nov 20, 2019
➤ 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}
[-]Submit a Sync ping in Fenix[/-][+]Include logins in the Fenix Sync Ping[/+]rfk commentedon Dec 4, 2019
I did a little dive here to see what all the moving parts are. IIUC here's what would be involved in this change:
history_syncmetrics definitions and rename them tologins_sync. Request what is hopefully a straightforward data-review for these new metrics.SyncTelemetry.processHistoryPingand rename it toprocessLoginsPing, and copy-pasta some testcases to match.processLoginsPingmethodprocessLoginsPingnext to the existing calls toprocessHistoryPingandprocessBookmarksPing(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 commentedon Dec 5, 2019
Yep, that's right!
😂😭🤪🙎♀️
rfk commentedon Dec 6, 2019
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 commentedon Dec 11, 2019
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 commentedon Dec 11, 2019
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 commentedon Dec 12, 2019
Happening in mozilla-mobile/android-components#5294
data-sync-user commentedon Dec 19, 2019
➤ 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.