1188416 - Use monotonic clocks for Telemetry subsessionLength

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We are seeing negative payload.info.subsessionLength values in the wild.
This may just be jumping/broken clocks and us not using monotonic clocks for this.

We need to fix not using monotonic clocks for that calculation.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Use monotonic clocks for Telemetry subsessionLength

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8639964 [details] [diff] [review]
Use monotonic clocks for Telemetry subsessionLength

Review of attachment 8639964 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/head.js
@@ +247,5 @@
>  
>    return new Date(date);
>  }
>  
> +function fakeMonotonicNow(ms) {

Are we planning on using monotonic clocks somewhere else right now? 
Is there any reason why this isn't simply:

let m = Cu.import("resource://gre/modules/TelemetrySession.jsm");
m.Policy.monotonicNow = () => ms;
return ms;

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8639964 [details] [diff] [review]
Use monotonic clocks for Telemetry subsessionLength

Review of attachment 8639964 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +805,5 @@
>    // The running no. of all subsessions for the whole profile life time
>    _profileSubsessionCounter: 0,
>    // Date of the last session split
>    _subsessionStartDate: null,
> +  // Start time of the current subsession per performance.now() for monotonically increasing

performance.now() ? Did you mean Policy.now()?

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/48e058ea4b74

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48e058ea4b74

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: Use monotonic clocks for Telemetry subsessionLength

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8641671 [details] [diff] [review]
Use monotonic clocks for Telemetry subsessionLength

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This is a simple fix for how we count how long a subsession / fragment of a session is.
We relied on potentially unstable clocks, this makes us use monotonic clocks instead.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we are monitoring it.
[Risks and why]:
Low-risk, we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8641671 [details] [diff] [review]
Use monotonic clocks for Telemetry subsessionLength

[Triage Comment]

Approved for uplift to Beta41. This patch has been in Nightly for a while so should be safe to uplift.

Comment 9

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a5c248399f2d