1376629 - Make timespan measurements in TelemetrySend use monotonicNow()

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Georg Fritzsche [:gfritzsche]]

Date() can be unstable if client clocks are jumping.
We should instead use monotonicNow() here for the timespan measurements:
https://dxr.mozilla.org/mozilla-central/rev/b1b9129838ade91684574f42219b2010928d7db4/toolkit/components/telemetry/TelemetrySend.jsm#1173-1197

This change should pass the tests:
./mach test toolkit/components/telemetry/tests/unit

And the linter:
./mach eslint toolkit/components/telemetry/

We should also verify manually in about:telemetery that the following histograms are recorded properly when submitting a new ping:
TELEMETRY_STRINGIFY
TELEMETRY_COMPRESS

We can submit a ping e.g. by opening about:telemetry, opening the web console and using:
TelemetryController.submitExternalPing("my-test-ping-type", {})

Comment 1

[Vanessa Gutierrez]

Attachment: Replace Date() with monotonicNow() for stability

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8881954 [details] [diff] [review]
Replace Date() with monotonicNow() for stability

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

This patch includes changes to two files that are not needed.
Lets remove that from this patch.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +1193,5 @@
>      payloadStream.data = gzipCompressString(utf8Payload);
>  
>      const compressedPingSizeKB = Math.floor(payloadStream.data.length / 1024);
> +    Telemetry.getHistogramById("TELEMETRY_COMPRESS").add(monotonicNow() - startTime);
> +    startTime = monotonicNow();

Is this startTime actually used anywhere?
Or can we just remove it?

Comment 3

[Vanessa Gutierrez]

Attachment: Remove unused line

Comment 4

[Vanessa Gutierrez]

Attachment: Discard changes to npm-shrinkwrap.json and package.json

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8881979 [details] [diff] [review]
Discard changes to npm-shrinkwrap.json and package.json

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

This looks good, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe78c45508e
Discard changes to npm-shrinkwrap.json and package.json. r=gfritzsche

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8fe78c45508e

Comment 8

[Vanessa Gutierrez]

Attachment: Added active_ticks scalar. Incremented active_ticks where Simple Measurements active ticks is incremented. Implemented equivalent tests where Simple Measurements active ticks is tested

Comment 9

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #8)
> Created attachment 8883128 [details]
> Added active_ticks scalar. Incremented active_ticks where Simple
> Measurements active ticks is incremented. Implemented equivalent tests where
> Simple Measurements active ticks is tested

Please disregard this attachment, I was trying to upload a patch to a different bug.

Comment 10

[Vanessa Gutierrez]

Comment on attachment 8883128 [details]
Added active_ticks scalar. Incremented active_ticks where Simple Measurements active ticks is incremented. Implemented equivalent tests where Simple Measurements active ticks is tested

These edits are in reference to Bug 1376942, please ignore.