Closed Bug 1440116 Opened 3 years ago Closed 2 years ago

Bookmark merger should submit fewer telemetry events

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: tcsc, Assigned: lina)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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)
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).
Yes, I think that makes perfect sense. From an analysis POV, this seems like exactly the granularity we are after.
Blocks: 1433177
Summary: Consider submitting sync ping when we hit the event limit. → Bookmark merger should submit fewer telemetry events
Priority: -- → P2
Assignee: nobody → kit
Comment on attachment 8961507 [details]
Bug 1440116 - Add an `ObjectUtils.isEmpty` helper.

https://reviewboard.mozilla.org/r/230280/#review236882
Attachment #8961507 - Flags: review?(markh) → review+
Comment on attachment 8961508 [details]
Bug 1440116 - Roll up telemetry events in the bookmarks merger.

Thom, do you mind taking this?
Attachment #8961508 - Flags: review?(markh) → review?(tchiovoloni)
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)`?
Attachment #8961508 - Flags: review?(tchiovoloni) → review+
See Also: → 1450385
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!
Pushed by kcambridge@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/37b327816c15
https://hg.mozilla.org/mozilla-central/rev/3ea30ddc0c47
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.