Add detailed bookmark validation errors and stats collection

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sleroux, Assigned: sleroux)

Tracking

unspecified
Other
iOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fxios9.0)

Details

(Whiteboard: [MobileCore][ios-sync])

Attachments

(1 attachment)

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
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?
Flags: needinfo?(rnewman)
Flags: needinfo?(markh)
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
(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.
Flags: needinfo?(markh)
Flags: needinfo?(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.
Whiteboard: [MobileCore]
Iteration: --- → 1.13
Priority: -- → P1
Iteration: 1.13 → 1.14
Iteration: 1.14 → 1.15
Summary: Add detailed bookmark validation errors → Add detailed bookmark validation errors and stats collection
Assignee: nobody → sleroux
Status: NEW → ASSIGNED
Iteration: 1.15 → 1.16
Whiteboard: [MobileCore] → [MobileCore][ios-sync]
Moving out of this sprint to focus on 7.0 stability issues.
Iteration: 1.16 → ---
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
Attachment #8868607 - Flags: review?(eoger)
Iteration: --- → 1.22
Attachment #8868607 - Flags: review?(eoger) → review+
master f75916c078c47a0c363bf27830cc49c9a6c36f89
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.