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

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Comment 1

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

Attachment: Bug 1346175 - Record AsyncResource.serverTime in sync telemetry events.

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

Comment 2

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

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

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

thanks!

Comment 3

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

Actually, we better get bsmedberg to sign off on this before landing.

Comment 4

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

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.

Comment 5

[Benjamin Smedberg]

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.

Comment 6

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

(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?

Comment 7

[Benjamin Smedberg]

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.

Comment 8

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

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

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

Comment 9

[Benjamin Smedberg]

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

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

data-r=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be484d483520
Record AsyncResource.serverTime in sync telemetry events. r=bsmedberg,markh

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be484d483520