Sync event telemetry should record AsyncResource.serverTime in the extra field for more reliable flow analysis

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: markh, Assigned: tcsc)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

8 months ago
Priority: -- → P1
Assignee: nobody → tchiovoloni
Comment hidden (mozreview-request)
(Reporter)

Comment 2

8 months ago
mozreview-review
Comment on attachment 8854515 [details]
Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events.

https://reviewboard.mozilla.org/r/126476/#review129112

thanks!
Attachment #8854515 - Flags: review?(markh) → review+
(Reporter)

Comment 3

8 months ago
Actually, we better get bsmedberg to sign off on this before landing.
(Reporter)

Comment 4

8 months ago
Comment on attachment 8854515 [details]
Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events.

Benjamin, in this patch we are recording the "server time" in our Sync event telemetry. The server time is actually the timestamp from the current user's most recent request to the server. We want to use this as a stable clock we can use to ensure valid ordering of events across multiple devices.

Given sync pings already record a (hashed) version of a sync UID and a (hashed) set of all devices IDs connected to the account, and given the times recorded are specific to this user (as each user makes requests independently and at effectively arbitrary times), I don't see information leakage risks, but asking for data review anyway.
Attachment #8854515 - Flags: review?(benjamin)

Comment 5

8 months ago
mozreview-review
Comment on attachment 8854515 [details]
Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events.

https://reviewboard.mozilla.org/r/126476/#review129518

::: commit-message-8d837:1
(Diff revision 1)
> +Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events. r?markh

I do data review against changes to documentation, so I am unable to data-review this.
Attachment #8854515 - Flags: review?(benjamin) → review-
(Reporter)

Comment 6

8 months ago
(In reply to Benjamin Smedberg [:bsmedberg] from comment #5)
> I do data review against changes to documentation, so I am unable to
> data-review this.

Can you suggest someone who can?
Flags: needinfo?(benjamin)

Comment 7

8 months ago
I'm sorry I wasn't clear. I am the right person to data-review this, but I need the patch to include documentation updates.
Flags: needinfo?(benjamin)
Comment hidden (mozreview-request)

Comment 9

7 months ago
mozreview-review
Comment on attachment 8854515 [details]
Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events.

https://reviewboard.mozilla.org/r/126476/#review131612

data-r=me
Attachment #8854515 - Flags: review?(benjamin) → review+

Comment 10

7 months ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be484d483520
Record AsyncResource.serverTime in sync telemetry events. r=bsmedberg,markh

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be484d483520
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.