1324558 - Add detailed bookmark validation errors and stats collection

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect, P1)

Description

[Stephan Leroux [:sleroux]]

On iOS, we only capture problems with bookmark merging at a higher level than desktop: Missing Values, Missing Structure, Overlapping Structure, Parent ID Disagreement. The Sync Ping on desktop goes into more detail on these issues [1]. If we want to be able to understand the issues on iOS relative to other platforms in the sync ecosystem we should strive to report similar data for easier comparison.

[1] https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/bookmark_validator.js#148

Comment 1

[Stephan Leroux [:sleroux]]

I vaguely remember talking about how we shouldn't expect to have the same data points between platforms but I can see us having value in compare apples w/ apples when looking at the sync data. Do you think this would be a requirement to get any valuable information from the Sync Ping?

Comment 2

[Stephan Leroux [:sleroux]]

For reference, iOS currently pokes the DB to get an idea of what could be happening:

https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/SQLiteBookmarksSyncing.swift#L773

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Stephan Leroux [:sleroux] from comment #1)
> I vaguely remember talking about how we shouldn't expect to have the same
> data points between platforms but I can see us having value in compare
> apples w/ apples when looking at the sync data. Do you think this would be a
> requirement to get any valuable information from the Sync Ping?

In theory it would be perfect if the validation results were identical, but in practice I think that will be difficult. There might be some things we can consolidate though which probably is valuable.

The desktop validator is at https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/bookmark_validator.js and the comments near the top of that file show what we record. From a quick look, we probably have finer-grained info that iOS might have, but they were decided based purely on "what inconsistencies are possible" rather than "what inconsistencies lead to a particular bad outcome" (which IIUC is where iOS is coming from)

For example, I expect that what you record as "MissingStructure", in desktop might be a combination of "orphans", "multipleParents", "deletedParents", "duplicateChildren", "childrenOnNonFolder", "parentNotFolder", "clientMissing", "serverMissing", "serverDeleted" etc - however, I'm not convinced iOS should try and replicate all of them. Indeed, once we do a little more analysis, I expect we will find that many of these cluster (eg, an "orphan" is probably associated with, say, "serverDeleted" or "serverMissing" etc).

Given all that, I think that iOS should do the smallest necessary to expose its concept of "invalid", and we can allow our dashboards or other ad-hoc telemetry to try and group these into buckets based on that device submitted the ping. As a followup next year we can consider working with Android to come up with a set of validation issues that all platforms report consistently.

Comment 4

[Richard Newman [:rnewman]]

Note that it would be relatively straightforward to extend the set of validations on iOS — e.g., a query for children with multiple parents is very easy to express — and we will likely do so as part of repair, because we'd need to determine which records to fetch.

Comment 5

[Stephan Leroux [:sleroux]]

Moving out of this sprint to focus on 7.0 stability issues.

Comment 6

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2734

Mostly contains changes to some of the validation code you wrote Edouard!

For the most part this follows the sync ping spec [1] - we just omit some properties like version/checked/failureReason.

[1] http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/sync-ping.html#syncs-devices

Comment 7

[Stephan Leroux [:sleroux]]

master f75916c078c47a0c363bf27830cc49c9a6c36f89