Closed
Bug 1376605
Opened 7 years ago
Closed 7 years ago
Process Payload assembly is getting repetitive
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: chutten, Assigned: amiyaguchi, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
The process of assembling the processes.* part of the ping is very repetitive. We can do better, and probably ought to given the likelihood that the number of process types is going to increase in future.
Updated•7 years ago
|
Mentor: gfritzsche
Priority: -- → P3
Whiteboard: [lang=js]
Comment 1•7 years ago
|
||
This is the section of the code that we want to refactor:
https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/toolkit/components/telemetry/TelemetrySession.jsm#1306
Updated•7 years ago
|
Assignee: nobody → amiyaguchi
Comment 2•7 years ago
|
||
The number of processes is still increasing:
i will add a "dynamic" process section to the payload in bug 1302681, "part 1".
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8894561 -
Flags: review?(gfritzsche)
Comment 4•7 years ago
|
||
Comment on attachment 8894561 [details] [diff] [review]
Refactor process payload assembly
Review of attachment 8894561 [details] [diff] [review]:
-----------------------------------------------------------------
This looks much better, thanks!
Overall this looks good, with some small things fixed.
::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1303,5 @@
> };
>
> + let measurementsContainGPU = Object
> + .keys(measurements)
> + .filter(key => key != "events")
We can drop this line, not adding "gpu" if there are events for it was an oversight.
@@ +1307,5 @@
> + .filter(key => key != "events")
> + .some(key => "gpu" in measurements[key]);
> +
> + payloadObj.processes = {};
> + const processTypes = ["parent", "content", "extension", "gpu"];
Why not handle `measurementsContainGPU` here, outside of the loop below?
You can just `.push("gpu")` conditionally here.
You will need to rebase this, i added the process type "dynamic" in bug 1302681.
@@ +1314,5 @@
> + for (const processType of processTypes) {
> + let processPayload = {};
> + // Only include the GPU process if we've accumulated data for it.
> + if (processType == "gpu" && !measurementsContainGPU) { continue; }
> +
Nit: Trailing spaces.
"./mach eslint -wo" will also complain about this (and other possible style issues).
Attachment #8894561 -
Flags: review?(gfritzsche) → feedback+
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8894561 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895471 -
Flags: review?(gfritzsche)
Comment 6•7 years ago
|
||
Comment on attachment 8895471 [details] [diff] [review]
Refactor process payload assembly
Review of attachment 8895471 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, i only have some small style comments below.
::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1311,2 @@
> // Only include the GPU process if we've accumulated data for it.
> + if (measurementsContainGPU) { processTypes.push("gpu") }
We always put statements inside blocks on new lines:
if (...) {
...
}
@@ +1316,5 @@
> + let processPayload = {};
> +
> + for (const key in measurements) {
> + let payloadLoc = processPayload;
> + // Parent histograms are added to the top-level payload object instead of the process payload
Nit: Missing trailing "." for this comment and the ones below.
@@ +1321,5 @@
> + if (processType == "parent" && (key == "histograms" || key == "keyedHistograms")) {
> + payloadLoc = payloadObj;
> + }
> + // Dynamic processes only collect events
> + if (processType == "dynamic" && key != "events") { continue; }
Ditto on the formatting, the `continue` should go on a new line.
Attachment #8895471 -
Flags: review?(gfritzsche) → feedback+
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8895471 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895878 -
Flags: review?(gfritzsche)
Comment 8•7 years ago
|
||
Comment on attachment 8895878 [details] [diff] [review]
Refactor process payload assembly
Review of attachment 8895878 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, this looks good!
You can set the keyword "checkin-needed" to have the patch landed for you.
Attachment #8895878 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2d0086d41415
Refactor process payload assembly. r=gfritzsche
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•