1353307 - Add main process start time to sync ping

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Georg Fritzsche [:gfritzsche]]

Per bug 1349324, the sync ping should have the main process start time as a data point.
We ended up going for hourly precision here for now.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Heads-up.

Comment 2

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

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Heads-up.

Head's up, thx :)

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1353307 - Add main process start time to sync ping.

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

Comment 4

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

Comment on attachment 8868302 [details]
Bug 1353307 - Add main process start time to sync ping.

https://reviewboard.mozilla.org/r/139890/#review143706

LGTM, thanks. Can you please fix the documentation and request review from Georg?

::: toolkit/components/telemetry/docs/data/sync-ping.rst:25
(Diff revision 1)
>          version: 1,
>          discarded: <integer count> // Number of syncs discarded -- left out if zero.
>          why: <string>, // Why did we submit the ping? Either "shutdown", "schedule", or "idchanged".
>          uid: <string>, // Hashed FxA unique ID, or string of 32 zeros. If this changes between syncs, the payload is submitted.
>          deviceID: <string>, // Hashed FxA Device ID, hex string of 64 characters, not included if the user is not logged in. If this changes between syncs, the payload is submitted.
> +        sessionStartTime: <integer milliseconds since epoch>, // Session start time.

This should document the 1-hour granularity we impose.

Comment 5

[Edouard Oger [:eoger]]

Comment on attachment 8868302 [details]
Bug 1353307 - Add main process start time to sync ping.

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

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8868302 [details]
Bug 1353307 - Add main process start time to sync ping.

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

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8868302 [details]
Bug 1353307 - Add main process start time to sync ping.

https://reviewboard.mozilla.org/r/139890/#review145426

::: toolkit/components/telemetry/docs/data/sync-ping.rst:25
(Diff revision 3)
>          version: 1,
>          discarded: <integer count> // Number of syncs discarded -- left out if zero.
>          why: <string>, // Why did we submit the ping? Either "shutdown", "schedule", or "idchanged".
>          uid: <string>, // Hashed FxA unique ID, or string of 32 zeros. If this changes between syncs, the payload is submitted.
>          deviceID: <string>, // Hashed FxA Device ID, hex string of 64 characters, not included if the user is not logged in. If this changes between syncs, the payload is submitted.
> +        sessionStartTime: <integer milliseconds since epoch>, // Session start time (hourly precision).

I don't remember the whole conversation we had on this.
Is it sufficient to have the "start time" here?
Or does this need a "start date"?

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Comment on attachment 8868302 [details]
> Bug 1353307 - Add main process start time to sync ping.
> 
> https://reviewboard.mozilla.org/r/139890/#review145426
> 
> ::: toolkit/components/telemetry/docs/data/sync-ping.rst:25
> (Diff revision 3)
> >          version: 1,
> >          discarded: <integer count> // Number of syncs discarded -- left out if zero.
> >          why: <string>, // Why did we submit the ping? Either "shutdown", "schedule", or "idchanged".
> >          uid: <string>, // Hashed FxA unique ID, or string of 32 zeros. If this changes between syncs, the payload is submitted.
> >          deviceID: <string>, // Hashed FxA Device ID, hex string of 64 characters, not included if the user is not logged in. If this changes between syncs, the payload is submitted.
> > +        sessionStartTime: <integer milliseconds since epoch>, // Session start time (hourly precision).
> 
> I don't remember the whole conversation we had on this.
> Is it sufficient to have the "start time" here?
> Or does this need a "start date"?

Comment 9

[bugzilla]

Truncated millis since epoch then start time should be fine from a computational perspective, of course, but is there a reason to choose this format over the ISO timestamp that we used in the main ping? If not, I think the latter would be preferable for the sake of consistency.

Comment 10

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

Georg, thanks for raising this - do you have a reference to the main ping format (or code that implements it)?

Comment 11

[Georg Fritzsche [:gfritzsche]]

Docs are on sessionStartDate & subsessionStartDate here:
https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/main-ping.html

Code:
https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/toolkit/components/telemetry/TelemetrySession.jsm#1055-1056

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8868302 [details]
Bug 1353307 - Add main process start time to sync ping.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df452af1e39a
Add main process start time to sync ping. r=gfritzsche,markh

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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