(See bug 671038 for more context) Sync Time to sync bookmarks Time to sync history Time to sync passwords Time to sync cookies ...

Would also like to capture data on success vs failure. Would specifically like the ability to know when Sync starts failing for people. Would be nice to correlate this to specific time(s), servers, Firefox versions, specific engines.

An important metric we should be following explicitly: "user saw a sync error bar".

This patch adds telemetry support for sync status. It's currently only 'success' or 'failure', but more detailed information (for example cause of failure) could be added to the enumeration in the future. I didn't add any information on sync time, as :rnewman said there wasn't too much use for that.

Comment on attachment 8515527 [details] [diff] [review] bug671045 Review of attachment 8515527 [details] [diff] [review]: ----------------------------------------------------------------- Reassigning to rnewman since I'm not actively doing Sync foo these days.

Comment on attachment 8515527 [details] [diff] [review] bug671045 Review of attachment 8515527 [details] [diff] [review]: ----------------------------------------------------------------- gfritzche just added some stuff that will help here: --- Per bug 1069873 and bug 1069874, Telemetry now has: * a new histogram type for counts (so there is no need anymore to use boolean histograms for that) * keyed histograms, basically providing a key/value store (mapping string keys to histograms) I updated the documentation accordingly: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

This new version uses the keyed count histograms.

Comment on attachment 8522460 [details] [diff] [review] bug671045-2.patch Review of attachment 8522460 [details] [diff] [review]: ----------------------------------------------------------------- Nearly! You might consider moving this to engines.js instead of enginesync, so you have more fine-grained control over what really counts as failure or success. ::: services/sync/modules/stages/enginesync.js @@ +206,5 @@ > // Here we simply want to muffle the exception and return an > // appropriate value. > + try { > + let histogram = Services.telemetry.getKeyedHistogramById("SYNC_ENGINESYNC_FAILURE"); > + histogram.add(engine.name); This shouldn't be in the 401 section, I think. @@ +215,5 @@ > return false; > } > } > + try { > + let histogram = Services.telemetry.getKeyedHistogramById("SYNC_ENGINESYNC_SUCCESS"); This isn't quite correct. If you look up to line 199, you'll see that we're calling engine.sync(). This will return successfully if the engine is disabled. ::: toolkit/components/telemetry/Histograms.json @@ +6820,5 @@ > + "SYNC_ENGINESYNC_FAILURE": { > + "expires_in_version": "never", > + "kind": "count", > + "keyed": true, > + "description": "Stores the amount of enginesync failures by type of engine" s/amount/number s/enginesync/Sync engine @@ +6826,5 @@ > + "SYNC_ENGINESYNC_SUCCESS": { > + "expires_in_version": "never", > + "kind": "count", > + "keyed": true, > + "description": "Stores the amount of successful enginesyncs by type of engine" same

/r/1745 - Bug 671045: add telemetry metrics on engine sync failure Pull down this commit: hg pull review -r ac3b3c124b374b9cd467ec3173fbdc8488ea478d

https://reviewboard.mozilla.org/r/1743/#review1313 Looks good to me with these two changes. ::: services/sync/modules/engines.js (Diff revision 1) > + this._log.warn("Getting telemetry histogram failed for engine " + engine.name.toUpperCase() + ": " + No need for the uppercasing here, I think. ::: services/sync/modules/engines.js (Diff revision 1) > + Utils.exceptionStr(ex)); I think you can now just do: ``` this._log.warn("...", ex); ``` and the right thing will happen.

Hi Mathias, Anything else we can do to keep this moving forward?

Hi Mark, I'm afraid I lost track of this bug. It should be relatively simple to fix (the comments by Richard are easy to address), but I don't have time to focus on Mozilla in the next few weeks (I should be able to take care of it at the end of May though). Alternatively, since it's just fixing a few small cosmetic parts of the patch, perhaps this is a good first bug for someone to finish?

Katie, wondering if you can help us with establishing some Sync client telemetry, and analysis.

Unfortunately, the measurement team cannot spare resources to help until unified telemetry is on solid footing https://bugzilla.mozilla.org/show_bug.cgi?id=671045

I today's Sync meeting this came up again; we'd like to get an idea of ongoing and persistent failures with individual Sync engines. Pretty sure this bug can be repurposed.

Hi Chris, is this bug superseded by bug 8703490 et al or is it still relevant?

georg, can you clarify which bug you're asking about? bug 8703490 isn't valid (typo?).

Sorry, that was supposed to be bug 1236383 :)

We report engine failures via https://dxr.mozilla.org/mozilla-central/rev/3f780f4b14acab29b16bc4141a44180a8af9dd08/services/sync/modules/policies.js#586, which helped us determine that addons are failing much more than they should be. Given the other work we are doing with telemetry, I think it's safe to dupe this to bug 1236383