Closed
Bug 1376629
Opened 7 years ago
Closed 7 years ago
Make timespan measurements in TelemetrySend use monotonicNow()
Categories
(Toolkit :: Telemetry, enhancement, P4)
Toolkit
Telemetry
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", {})
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8881954 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 2•7 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•7 years ago
|
||
Attachment #8881971 -
Flags: review?(gfritzsche)
Assignee | ||
Updated•7 years ago
|
Attachment #8881954 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8881979 -
Flags: review?(gfritzsche)
Assignee | ||
Updated•7 years ago
|
Attachment #8881971 -
Attachment is obsolete: true
Attachment #8881971 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 5•7 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•7 years ago
|
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8fe78c45508e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8883128 -
Flags: review?(gfritzsche)
Assignee | ||
Updated•7 years ago
|
Attachment #8881979 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 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•7 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.
Description
•