Use monotonic clocks for Telemetry subsessionLength

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [unifiedTelemetry] [uplift4])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Attachment #8639964 - Flags: review?(alessio.placitelli)
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;
Attachment #8639964 - Flags: review?(alessio.placitelli) → review+
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()?
https://hg.mozilla.org/mozilla-central/rev/48e058ea4b74
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1189164
Attachment #8639964 - Attachment is obsolete: true
Attachment #8641671 - Flags: review+
Iteration: --- → 42.3 - Aug 10
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.
Attachment #8641671 - Flags: approval-mozilla-aurora?
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.
Attachment #8641671 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.