Closed Bug 1376629 Opened 7 years ago Closed 7 years ago

Make timespan measurements in TelemetrySend use monotonicNow()

Categories

(Toolkit :: Telemetry, enhancement, P4)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: gfritzsche, Assigned: vgutierrez5, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(4 obsolete files)

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", {})
Attachment #8881954 - Flags: review?(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?
Attachment #8881954 - Flags: review?(gfritzsche)
Attached patch Remove unused line (obsolete) — Splinter Review
Attachment #8881971 - Flags: review?(gfritzsche)
Attachment #8881954 - Attachment is obsolete: true
Attachment #8881979 - Flags: review?(gfritzsche)
Attachment #8881971 - Attachment is obsolete: true
Attachment #8881971 - Flags: review?(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!
Attachment #8881979 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe78c45508e
Discard changes to npm-shrinkwrap.json and package.json. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8fe78c45508e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attachment #8881979 - Attachment is obsolete: true
(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 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.
Attachment #8883128 - Attachment is obsolete: true
Attachment #8883128 - Attachment is patch: false
Attachment #8883128 - Flags: review?(gfritzsche)
You need to log in before you can comment on or make changes to this bug.