1376605 - Process Payload assembly is getting repetitive

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

The number of processes is still increasing:
i will add a "dynamic" process section to the payload in bug 1302681, "part 1".

Comment 3

[Anthony Miyaguchi [:amiyaguchi]]

Attachment: Refactor process payload assembly

Comment 4

[Georg Fritzsche [:gfritzsche]]

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).

Comment 5

[Anthony Miyaguchi [:amiyaguchi]]

Attachment: Refactor process payload assembly

Comment 6

[Georg Fritzsche [:gfritzsche]]

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.

Comment 7

[Anthony Miyaguchi [:amiyaguchi]]

Attachment: Refactor process payload assembly

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d0086d41415
Refactor process payload assembly. r=gfritzsche

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2d0086d41415