837238 - Don't report session data in milliseconds

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Session data as implemented reports in milliseconds. The requirements spreadsheet specified seconds. Should we change data to seconds or leave in milliseconds?

Essentially the only time that will be changed is the total wall time of the session. IMO milliseconds shouldn't be too important here and we can throw them away. Another benefit of seconds over milliseconds is that milliseconds will overflow a 32 bit signed integer after 24.85 days. We don't have to worry about overflow if we store seconds.

Comment 1

[Daniel Einspanjer [:dre] [:deinspanjer]]

Definitely go ahead and change to seconds.  I'd go so far as to says minutes, but I'd want buy in from an analyst before authorizing the change just for me.

Comment 2

["Saptarshi Guha[:joy]"]

Seconds is great. Minutes is good, but the data becomes unnecessary more discrete. A lot of stat routines assume the data is generated by a continuous  process yet by reducing it to minutes our data will end up looking very discrete.

Comment 3

["Saptarshi Guha[:joy]"]

Just in case i confused anyone, let's stay with seconds

Comment 4

[Gregory Szorc [:gps]]

Attachment: Record total session time in seconds, v1

While this technically breaks backwards compatibility, I think it is the easiest solution.

We now store seconds instead of milliseconds for the total time and only the total time. I could have stored seconds for start time as well, but meh. (We'd need migration code.)

I thought it was important to store seconds and not milliseconds in preferences because of concerns about 32 bit overflow with milliseconds. We still hold on to the raw milliseconds locally so we don't lose fragments due to rounding.

I also bumped the FHR measurement versions to signify a change in semantics.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Record total session time in seconds, v2

qref

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 710372 [details] [diff] [review]
Record total session time in seconds, v2

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

::: services/datareporting/sessions.jsm
@@ +121,4 @@
>      this._prefs.set("current.activeTicks", ++this._activeTicks);
>    },
>  
>    get totalTime() {

Add comment mentioning that this returns seconds.

::: services/datareporting/tests/xpcshell/test_session_recorder.js
@@ +71,5 @@
>  
> +  let oldTime = recorder.totalTime;
> +  yield sleep(1000);
> +  recorder.updateTotalTime();
> +  do_check_eq(recorder.totalTime - 1, oldTime);

This seems really fragile; I wouldn't want to count on the behavior of sleep being within the rounding range of a unit second.

Please instead modify recorder.startDate and re-run updateTotalTime.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/d5390130b80e

Comment 8

[Gregory Szorc [:gps]]

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