1440116 - Bookmark merger should submit fewer telemetry events

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

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

We currently have a limit of 1000 events allowed to be stored in the sync ping before we start dropping them. We should consider submitting when this happens instead.

It seems like the bookmark mirror in particular likes submitting a lot of events: https://sql.telemetry.mozilla.org/queries/51554

I think the right logic will be something like to submit if the sync finishes and we're within 80% of the limit for events or so, so that we reduce the chance we'll drop events during the next sync and avoid submitting events with no sync.

Comment 1

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

Thanks for opening this, but a contrarian view is that we should never generate 1000 events :) We should consider having the bookmark merger report a single event regardless of how often a particular event actually happens (for example, if 1k bookmarks all trigger the condition that forces an event to be sent, that sync should probably still just report the event once (and possibly with the number of times that sync hit it as part of metadata for that event)

Comment 2

[Lina Butler [:lina]]

Alternatively, do you think it makes sense for the mirror to roll up and emit a single `merge` event, with extra properties containing timings and counts? (Or maybe one event with timings for building the trees, and another with counts of dupes, inconsistencies, interesting structure changes, etc).

Comment 3

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

Yes, I think that makes perfect sense. From an analysis POV, this seems like exactly the granularity we are after.

Comment 4

[Lina Butler [:lina]]

Attachment: Bug 1440116 - Add an `ObjectUtils.isEmpty` helper.

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

Comment 5

[Lina Butler [:lina]]

Attachment: Bug 1440116 - Roll up telemetry events in the bookmarks merger.

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

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

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

Comment 7

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

Comment on attachment 8961507 [details]
Bug 1440116 - Add an `ObjectUtils.isEmpty` helper.

https://reviewboard.mozilla.org/r/230280/#review236882

Comment 8

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

Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

Thom, do you mind taking this?

Comment 9

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

Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

https://reviewboard.mozilla.org/r/230282/#review237238

Honestly, I think this is an abuse of event telemetry, and we'd be better off adding some mergeStats data to the telemetry engine record or something.

That said, doing so would be a lot more work, it's a bit late for me to come out and say that now, and this is certainly an improvement to the status quo.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3377
(Diff revision 2)
> +    }
> +    let structureExtra = {};
> +    for (let key in this.structureCounts) {
> +      let count = this.structureCounts[key];
> +      if (count > 0) {
> +        structureExtra[key] = count;

Should this be `String(count)`?

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

https://reviewboard.mozilla.org/r/230282/#review237238

That makes sense! Filed bug 1450385 as a follow-up.

> Should this be `String(count)`?

ObjectUtils.deepEqual({ a: "1" }, { a: 1 });
// => true

*side-eyes `ObjectUtils.deepEqual`* Thanks for catching that!

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8961507 [details]
Bug 1440116 - Add an `ObjectUtils.isEmpty` helper.

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

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37b327816c15
Add an `ObjectUtils.isEmpty` helper. r=markh
https://hg.mozilla.org/integration/autoland/rev/3ea30ddc0c47
Roll up telemetry events in the bookmarks merger. r=tcsc

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/37b327816c15
https://hg.mozilla.org/mozilla-central/rev/3ea30ddc0c47