1013601 - Add uptimeMillis() to UITelemetry.jsm for getting current timestamp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Part 1: handle timestamp in UITelemetry

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Part 2: Change callers from generating timestamps

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8425912 [details] [diff] [review]
Part 1: handle timestamp in UITelemetry

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

::: toolkit/components/telemetry/UITelemetry.jsm
@@ +83,5 @@
>     * and should be registered with addSimpleMeasureFunction.
>     */
>    _simpleMeasureFunctions: {},
>  
> +  _uptimeMillis: function() {

Comment the hack! We're attempting to convert between two dissimilar timelines by using a single known point of intersection, and this will fail under clock corrections and pauses.

@@ +107,5 @@
>        type: "event",
>        action: aAction,
>        method: aMethod,
>        sessions: sessions,
> +      timestamp: aTimestamp || this._uptimeMillis(),

These guards should probably be

  (aTimestamp == undefined) ? this._uptimeMillis() : aTimestamp

Here's why:

  0 == undefined
  false

  null == undefined
  true

  undefined == undefined
  true

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8425913 [details] [diff] [review]
Part 2: Change callers from generating timestamps

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

::: mobile/android/chrome/content/aboutReader.js
@@ -289,5 @@
>      this._isReadingListItem = (this._isReadingListItem == 1) ? 0 : 1;
>      this._updateToggleButton();
>  
> -    // Create a relative timestamp for telemetry
> -    let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;

Keep this one, but make it call the utility function (and kill the comment). The timestamp is more accurate if it's computed prior to the storeArticleInCache callback.

Furthermore, I don't think it's bad practice for callers to be specifying timestamps; by all means elide it in the simplest of cases, but this is not a concept I want to hide from developers adding probes -- it's a core concept.

Comment 5

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/e63a1fb3757e
https://hg.mozilla.org/integration/fx-team/rev/e563583ad137

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/e63a1fb3757e
https://hg.mozilla.org/mozilla-central/rev/e563583ad137