1405205 - Ensure telemetry for syncs and for validation reports what bookmark engine is in use.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

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

I'm actually not 100% sure the patch in bug 1305563 doesn't already do this, but it seems likely that a no-op sync doesn't.

Knowing which engine is involved in the sync will help us accurately determine what problems are unique to either engine, and the reporting it for validation will help us track introduction rates between the engines.

I think this change is fine as a followup, so I'm opening this bug rather than commenting in bug 1305563.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

Review commit: https://reviewboard.mozilla.org/r/188534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188534/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

https://reviewboard.mozilla.org/r/188534/#review193778

::: services/sync/modules/telemetry.js:153
(Diff revision 1)
>      if (error) {
>        this.failureReason = SyncTelemetry.transformError(error);
>      }
> +    // We want the name to be different for bookmarks and bookmarks-buffered,
> +    // so we just check if the pref enabling buffered is set here.
> +    if (this.name == "bookmarks" && Svc.Prefs.get("engine.bookmarks.buffer", false)) {

This is admittedly cludgy, but it's vastly simpler and less error prone than whatever threading we'd need to do to expose a different name just to telemetry.

Another option is to look at the actual engine (e.g. in Service.engineManager) and see if it's buffered, maybe via a new property or something. If you'd rather I do that, np.

Comment 3

[Lina Butler (ex-Mozilla)]

(In reply to Thom Chiovoloni [:tcsc] from comment #2)
> This is admittedly cludgy, but it's vastly simpler and less error prone than
> whatever threading we'd need to do to expose a different name just to
> telemetry.

Agreed, let's not thread a different name throughout just for telemetry.

> Another option is to look at the actual engine (e.g. in
> Service.engineManager) and see if it's buffered, maybe via a new property or
> something. If you'd rather I do that, np.

I'm leaning toward this, just because flipping the pref still requires a restart to switch engines. If we decide to roll this out  gradually, via a system add-on, we'll count the existing engine as buffered, and vice versa.

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188534/diff/1-2/

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

https://reviewboard.mozilla.org/r/188534/#review193814

Awesome, thanks!

Comment 6

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

Thom, can this be landed?

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Not until kit's buffer patch (bug 1305563) lands.

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188534/diff/2-3/

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

It's been so long, please take a peek and tell me that it still makes sense?

I mean, it's not like there's that much code there, but still, carrying over an r+ from October seems like a stretch

Comment 10

[Lina Butler (ex-Mozilla)]

Comment on attachment 8917572 [details]
Bug 1405205 - Report bookmark engine as bookmarks-buffered in the sync ping

https://reviewboard.mozilla.org/r/188534/#review222134

Still looks good, thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2398885a5762
Report bookmark engine as bookmarks-buffered in the sync ping r=kitcambridge

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2398885a5762

Comment 13

[Julien Cristau [:jcristau]]

Assuming this is wontfix for 59 based on the dependency on bug 1305563