Make timespan measurements in TelemetrySend use monotonicNow()

RESOLVED FIXED in Firefox 56

Status

()

P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: vgutierrez5, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
1

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 obsolete attachments)

(Reporter)

Description

2 years ago
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", {})
(Assignee)

Comment 1

2 years ago
Attachment #8881954 - Flags: review?(gfritzsche)
(Reporter)

Comment 2

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Posted patch Remove unused line (obsolete) — Splinter Review
Attachment #8881971 - Flags: review?(gfritzsche)
(Assignee)

Updated

2 years ago
Attachment #8881954 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Attachment #8881979 - Flags: review?(gfritzsche)
(Assignee)

Updated

2 years ago
Attachment #8881971 - Attachment is obsolete: true
Attachment #8881971 - Flags: review?(gfritzsche)
(Reporter)

Comment 5

2 years ago
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+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8fe78c45508e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 years ago
Attachment #8881979 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Comment 10

2 years ago
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.