1437446 - Make probe process choice more visible in about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Attachment: current process selector look

For the probe views in about:telemetry (scalars, histograms, events and keyed versions), the process selector dropdown on the top right is hard to discover.

We should consider some options here to surface them better.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Off the top of my hat i can think of:

(1) Default to showing data from all processes. This means we need to be able to tell them apart; e.g. by adding something to the name or putting them in per-process sections.

(2) Use the menu on the left to show available processes and to select them.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: mockup of sidebar menu showing processes

Mockup of (2), note the submenu entries on the left side.

Comment 3

[Chris H-C :chutten]

I like both of those ideas. The default view should show all, with headers. And the headers should correspond to items in the left-hand menu for filtering.

It dovetails with existing design (see how the Environment section acts when you select Build, for instance), and ought to be more discoverable than the current approach.

Comment 4

[akriti verma]

Hi,
I know this bug has been assigned, but i had already started working on it while it was assigned to '[e-mail]'. Since i was already assigned two other bugs, i did not comment on this one. Now that one of them is resolved , i have resumed my work on this bug and really want to fix this. So if the mentor and the assignee do not mind , can i submit a patch for this??
Thanks.
akritiverma

Comment 5

[Chris H-C :chutten]

It is okay with me if it is okay with Iarsolav. what do you say?

Comment 6

[akriti verma]

Attachment: Bug 1437446 : Sample patch,

Review commit: https://reviewboard.mozilla.org/r/238536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238536/

Comment 7

[akriti verma]

While we are waiting to hear from Iaroslav i thought i must upload the work done so far, so that further discussion can take place to determine if this approach is viable.

Comment 8

[akriti verma]

Attachment: sample-patch-output.png

Attaching a screenshot of the result.

Comment 9

[Iaroslav Sheptykin]

Hi Guys!

Absolutely no problem with me. Go ahead an take over the bug! Sorry for blocking here.

Comment 10

[Chris H-C :chutten]

Comment on attachment 8969730 [details]
Bug 1437446 : Sample patch,

https://reviewboard.mozilla.org/r/238536/#review244308

This is a good start.

Is it possible to factor out the common code in some way? This repetition makes my inner software engineer unhappy :S

::: toolkit/content/aboutTelemetry.js:1717
(Diff revision 1)
> +
> +  addSubSection(id, aPayload) {
> +    let category = document.querySelector("#categories > [value=" + id + "]");
> +    category.classList.add("has-subsection");
> +
> +    let options = ["parent", "content", "extension", "dynamic", "gpu"];

Since we're given the payload, we can be clever and only show the processes for which there is data, rather than including all of these.

(Also, then we don't have to worry about what to do when new processes show up and start accumulating scalars)

::: toolkit/content/aboutTelemetry.js:1721
(Diff revision 1)
> +
> +    let options = ["parent", "content", "extension", "dynamic", "gpu"];
> +    for (let f of options) {
> +    let subCategory = document.createElement("div");
> +    subCategory.classList.add("category-subsection");
> +    subCategory.classList.add("category-subsection");

Not sure why you're double-adding this class here.

::: toolkit/content/aboutTelemetry.js:1731
(Diff revision 1)
> +      showSection(sectionId);
> +      this.scalarSelect(f, aPayload);
> +    });
> +   subCategory.appendChild(document.createTextNode(f));
> +   category.appendChild(subCategory);
> +    }

There's some indentation issues here. Did you run ./mach lint?

Comment 11

[Chris H-C :chutten]

The design in the screenshot looks nice. Be sure to remove the old process select while you're working on this. The subsection approach is so much easier to work with.

Comment 12

[akriti verma]

Attachment: bug 1437446 : Make probe process choice more visible in about:telemetry. ,

Review commit: https://reviewboard.mozilla.org/r/238930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238930/

Comment 13

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry. ,

Review commit: https://reviewboard.mozilla.org/r/238932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238932/

Comment 14

[akriti verma]

Hi Chris,
I have incorporated those changes and have run the ./mach build & ./mach lint commands. Both give no errors. Please let me know if more changes are needed.

Comment 15

[akriti verma]

Thanks Iaroslav!

Comment 16

[Chris H-C :chutten]

Comment on attachment 8970129 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry. ,

https://reviewboard.mozilla.org/r/238932/#review244708

Thank you for your patch! Looks like you have a much more complete solution. Did you run `mach lint` to ensure that it adheres to our style guides? I've noted down a could of style issues, but the linter will probably have other things to say, too.

::: toolkit/content/aboutTelemetry.js:50
(Diff revision 1)
>  
>  // Cached value of document's RTL mode
>  var documentRTLMode = "";
>  
> +// Data Collection Processes
> +var pOptions = ["parent", "content", "extension", "dynamic", "gpu"];

globals should start with `g`

::: toolkit/content/aboutTelemetry.js:1712
(Diff revision 1)
>     * Render the scalar data - if present - from the payload in a simple key-value table.
>     * @param aPayload A payload object to render the data from.
>     */
>    render(aPayload) {
> +    this.scalarSelect("parent", aPayload);
> +    let sectionid = "scalars-section";

sectionid is only passed to checkProcessData. May as well just use a string literal.

::: toolkit/content/aboutTelemetry.js:1735
(Diff revision 1)
> +    }
> +   }
> +   addSubSection(id, aPayload, prOptions, Scalars.scalarSelect);
> +  },
> +
> +  scalarSelect(hprocess, aPayload) {

arguments start with `a`

::: toolkit/content/aboutTelemetry.js:2058
(Diff revision 1)
>  }
>  
> +/**
> + * Shows the selected sub-section
> + */
> +function showSection(sectionid) {

style is to use camelCase in this file, with `a` prepended for arguments

Comment 17

[Chris H-C :chutten]

Arg, sorry, I just noticed you said you ran mach lint.

I wonder why it isn't catching things like the two-line conditional and snake_case variable names...

Comment 18

[akriti verma]

Attachment: Bug 1437446 : Make Probe Process Choice more Visible in about:telemetry. ,

Review commit: https://reviewboard.mozilla.org/r/239638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239638/

Comment 19

[Chris H-C :chutten]

Comment on attachment 8970871 [details]
Bug 1437446 : Make Probe Process Choice more Visible in about:telemetry. ,

https://reviewboard.mozilla.org/r/239638/#review245932

Be sure to test this on non-current non-main pings (select a ping by clicking the top-left where it says "Current Ping" then choosing "Archived Ping Data")

::: toolkit/content/aboutTelemetry.js:1719
(Diff revision 1)
> +
> +  checkProcessData(aId, aPayload) {
> +    let prOptions = [];
> +    var i = 0;
> +    for (let f of gOptions) {
> +    if (f === "parent") {

Shouldn't the body of the for loop be indented?

::: toolkit/content/aboutTelemetry.js:1725
(Diff revision 1)
> +      prOptions[i] = f;
> +      i = i + 1;
> +    } else if ("processes" in aPayload && f in aPayload.processes &&
> +               "scalars" in aPayload.processes[f]) {
> +      let scalars = aPayload.processes[f].scalars;
> +      if (Object.keys(scalars).length > 0 && scalars) {

The `&& scalars` may never run because Object.keys(scalars) will only ever be true when scalars is truthy, no?

::: toolkit/content/aboutTelemetry.js:1730
(Diff revision 1)
> +      if (Object.keys(scalars).length > 0 && scalars) {
> +       prOptions[i] = f;
> +       i = i + 1;
> +      }
> +    }
> +   }

I'm not 100% sure what's going on with this function. It's transforming the list of possible processes to a list starting with parent and only including the processes that have at least one scalar?

Then how about

let processesWithScalars = gProcesses.filter(process => {
  return process === "parent" ||
    (process in aPayload.processes
      && "scalars" in aPayload.processes[process]
      && Object.keys(scalars).length > 0)
});

::: toolkit/content/aboutTelemetry.js:1792
(Diff revision 1)
> +       i = i + 1;
> +      }
> +    }
> +   }
> +   addSubSection(aId, aPayload, prOptions, KeyedScalars.keyedScalarSelect);
> +  },

Need whitespace between these functions

::: toolkit/content/aboutTelemetry.js:2056
(Diff revision 1)
>  }
>  
> +/**
> + * Shows the selected sub-section
> + */
> +function showSection(aSectionId) {

Isn't this similar to showSubSection?

::: toolkit/content/aboutTelemetry.js:2061
(Diff revision 1)
> +function showSection(aSectionId) {
> +  if (!aSectionId) {
> +    return;
> +  }
> +  let current_selection = document.querySelector(".category-subsection.selected");
> +  if (current_selection)

Two-line conditionals are not allowed. We'll need {} on this.

::: toolkit/content/aboutTelemetry.js:2069
(Diff revision 1)
> +}
> +
> +/**
> + * Adds sub-sections
> + */
> +function addSubSection(aId, aPayload, aProcessList, aCallFunc) {

Could you reuse GenericSubsection.render for some of this?

Comment 20

[Chris H-C :chutten]

Hey akriti, do you need some more help with this bug?

Comment 21

[akriti verma]

I have tried this on Archived ping data and it works fine.
(In reply to Chris H-C :chutten from comment #19)
> Comment on attachment 8970871 [details]
> Bug 1437446 : Make Probe Process Choice more Visible in about:telemetry. ,
> 
> https://reviewboard.mozilla.org/r/239638/#review245932
> 
> Be sure to test this on non-current non-main pings (select a ping by
> clicking the top-left where it says "Current Ping" then choosing "Archived
> Ping Data")
> 
> ::: toolkit/content/aboutTelemetry.js:1719
> (Diff revision 1)
> > +
> > +  checkProcessData(aId, aPayload) {
> > +    let prOptions = [];
> > +    var i = 0;
> > +    for (let f of gOptions) {
> > +    if (f === "parent") {
> 
> Shouldn't the body of the for loop be indented?
> 
> ::: toolkit/content/aboutTelemetry.js:1725
> (Diff revision 1)
> > +      prOptions[i] = f;
> > +      i = i + 1;
> > +    } else if ("processes" in aPayload && f in aPayload.processes &&
> > +               "scalars" in aPayload.processes[f]) {
> > +      let scalars = aPayload.processes[f].scalars;
> > +      if (Object.keys(scalars).length > 0 && scalars) {
> 
> The `&& scalars` may never run because Object.keys(scalars) will only ever
> be true when scalars is truthy, no?
> 
> ::: toolkit/content/aboutTelemetry.js:1730
> (Diff revision 1)
> > +      if (Object.keys(scalars).length > 0 && scalars) {
> > +       prOptions[i] = f;
> > +       i = i + 1;
> > +      }
> > +    }
> > +   }
> 
> I'm not 100% sure what's going on with this function. It's transforming the
> list of possible processes to a list starting with parent and only including
> the processes that have at least one scalar?
> 
> Then how about
> 
> let processesWithScalars = gProcesses.filter(process => {
>   return process === "parent" ||
>     (process in aPayload.processes
>       && "scalars" in aPayload.processes[process]
>       && Object.keys(scalars).length > 0)
> });
> 
> ::: toolkit/content/aboutTelemetry.js:1792
> (Diff revision 1)
> > +       i = i + 1;
> > +      }
> > +    }
> > +   }
> > +   addSubSection(aId, aPayload, prOptions, KeyedScalars.keyedScalarSelect);
> > +  },
> 
> Need whitespace between these functions
> 
> ::: toolkit/content/aboutTelemetry.js:2056
> (Diff revision 1)
> >  }
> >  
> > +/**
> > + * Shows the selected sub-section
> > + */
> > +function showSection(aSectionId) {
> 
> Isn't this similar to showSubSection?
> 
> ::: toolkit/content/aboutTelemetry.js:2061
> (Diff revision 1)
> > +function showSection(aSectionId) {
> > +  if (!aSectionId) {
> > +    return;
> > +  }
> > +  let current_selection = document.querySelector(".category-subsection.selected");
> > +  if (current_selection)
> 
> Two-line conditionals are not allowed. We'll need {} on this.
> 
> ::: toolkit/content/aboutTelemetry.js:2069
> (Diff revision 1)
> > +}
> > +
> > +/**
> > + * Adds sub-sections
> > + */
> > +function addSubSection(aId, aPayload, aProcessList, aCallFunc) {
> 
> Could you reuse GenericSubsection.render for some of this?

I have tested this for archived ping data and it works fine.

let processesWithScalars = gProcesses.filter(process => {
>   return process === "parent" ||
>     (process in aPayload.processes
>       && "scalars" in aPayload.processes[process]
>       && Object.keys(scalars).length > 0)
> });
 Had tried this earlier, but it doesn't work. therefore , i added 'checkProcessData(id, aPayload)' to filter out the processes with data. 
 

> ::: toolkit/content/aboutTelemetry.js:2069
> (Diff revision 1)
> > +}
> > +
> > +/**
> > + * Adds sub-sections
> > + */
> > +function addSubSection(aId, aPayload, aProcessList, aCallFunc) {
> 
> Could you reuse GenericSubsection.render for some of this?

I thought it would be best to not disturb the GenericSubsection.render function because the "addSubSection(aId, aPayload, aProcessList, aCallFunc)" takes several arguments and adding these in "GenericSubsection.render()" would require some major changes to the function which can complicate things.
What do you think?

Comment 22

[Chris H-C :chutten]

(In reply to akriti verma from comment #21)
> I thought it would be best to not disturb the GenericSubsection.render
> function because the "addSubSection(aId, aPayload, aProcessList, aCallFunc)"
> takes several arguments and adding these in "GenericSubsection.render()"
> would require some major changes to the function which can complicate things.
> What do you think?

It looked similar so I was wondering if it'd help out. If you don't think it will, then no worries.

I look forward to see an updated patch with the issues resolved.

Comment 23

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry ,

Review commit: https://reviewboard.mozilla.org/r/247118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247118/

Comment 24

[akriti verma]

Hi Chris,
I have made the required changes in the code.
Regarding the 'checkProcessData()' function , it filters out the processes with data.
I had already tried this =>
              let processesWithScalars = gProcesses.filter(process => {
                 return process === "parent" ||
                 (process in aPayload.processes
                 && "scalars" in aPayload.processes[process]
                 && Object.keys(scalars).length > 0)
              });
It doesn't give the desired result. Therefore have not removed 'checkProcessData()' function. Can you suggest me some other workaround for this problem.
Thanks

Comment 25

[Chris H-C :chutten]

Comment on attachment 8980991 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/247118/#review253334

This is looking better! Now we're getting to just the small things.

::: toolkit/content/aboutTelemetry.js:50
(Diff revision 1)
>  
>  // Cached value of document's RTL mode
>  var documentRTLMode = "";
>  
> +// Data Collection Processes
> +var gOptions = ["parent", "content", "extension", "dynamic", "gpu"];

In the future I hope we'll be able to access the values from Processes.yaml directly so we don't have to duplicate it here.

::: toolkit/content/aboutTelemetry.js:1752
(Diff revision 1)
>      }
>  
>      let scalars = aPayload.processes[selectedProcess].scalars || {};
> -    let hasData = Array.from(processesSelect.options).some((option) => {
> -      let value = option.getAttribute("value");
> +    let hasData = scalarOptions.some((option) => {
> +      let value = selectedProcess;
>        let sclrs = aPayload.processes[value].scalars;

Why not use aProcess directly?

::: toolkit/content/aboutTelemetry.js:1814
(Diff revision 1)
>      }
>  
>      let keyedScalars = aPayload.processes[selectedProcess].keyedScalars || {};
> -    let hasData = Array.from(processesSelect.options).some((option) => {
> -      let value = option.getAttribute("value");
> +    let hasData = keyedScalarOptions.some((option) => {
> +      let value = selectedProcess;
>        let keyedS = aPayload.processes[value].keyedScalars;

Same here. We can use aProcess directly and remove value and selectedProcess

::: toolkit/content/aboutTelemetry.js:1929
(Diff revision 1)
>    let whitelist = [
> -    "scalars-section",
> -    "keyed-scalars-section",
> -    "histograms-section",
> -    "keyed-histograms-section",
>      "events-section"

Going to leave events for a follow-up patch? Or a follow-up bug?

If we can convert them all we can remove all the process selector code completely.

::: toolkit/content/aboutTelemetry.js:2059
(Diff revision 1)
>    adjustSearchState();
>    changeUrlPath(selectedValue);
>  }
>  
> +/**
> + * Shows the selected sub-section

That's what showSubSection does. This comment should highlight what's different between showSubSection and showSection

::: toolkit/content/aboutTelemetry.js:2335
(Diff revision 1)
>                 "histograms" in aPayload.processes[hgramsProcess]) {
>        histograms = aPayload.processes[hgramsProcess].histograms;
>      }
>  
> -    let hasData = Array.from(hgramsSelect.options).some((option) => {
> -      let value = option.getAttribute("value");
> +    let hasData = hgramsOptions.some((option) => {
> +      let value = hgramsProcess;

Here, too, we could use aProcess and remove hgramsProcess and value.

::: toolkit/content/aboutTelemetry.js:2395
(Diff revision 1)
>                 "keyedHistograms" in aPayload.processes[keyedHgramsProcess]) {
>        keyedHistograms = aPayload.processes[keyedHgramsProcess].keyedHistograms;
>      }
>  
> -    let hasData = Array.from(keyedHgramsSelect.options).some((option) => {
> -      let value = option.getAttribute("value");
> +    let hasData = keyedHgramsOptions.some((option) => {
> +      let value = keyedHgramsProcess;

Here as well.

Comment 26

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry,

Review commit: https://reviewboard.mozilla.org/r/248694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248694/

Comment 27

[Chris H-C :chutten]

Comment on attachment 8982812 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry,

https://reviewboard.mozilla.org/r/248694/#review255474

We're very close, now!

::: toolkit/content/aboutTelemetry.js
(Diff revision 1)
> -  let whitelist = [
> -    "scalars-section",
> -    "keyed-scalars-section",
> -    "histograms-section",
> -    "keyed-histograms-section",
> -    "events-section"

Where did you implement per-process subsections for Events? I see implementations for the other four sections in this patch only.

::: toolkit/content/aboutTelemetry.js
(Diff revision 1)
> -    "keyed-scalars-section",
> -    "histograms-section",
> -    "keyed-histograms-section",
> -    "events-section"
> -  ];
> -  let processes = document.getElementById("processes");

This means we can get rid of #processes from aboutTelemetry.xhtml as well.

Comment 28

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry,

Review commit: https://reviewboard.mozilla.org/r/250528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250528/

Comment 29

[akriti verma]

(In reply to Chris H-C :chutten from comment #27)
> Comment on attachment 8982812 [details]
> Bug 1437446 : Make probe process choice more visible in about:telemetry,
> 
> https://reviewboard.mozilla.org/r/248694/#review255474
> 
> We're very close, now!
> 
> ::: toolkit/content/aboutTelemetry.js
> (Diff revision 1)
> > -  let whitelist = [
> > -    "scalars-section",
> > -    "keyed-scalars-section",
> > -    "histograms-section",
> > -    "keyed-histograms-section",
> > -    "events-section"
> 
> Where did you implement per-process subsections for Events? I see
> implementations for the other four sections in this patch only.

In the latest patch i have added subsections to Events as well. Since currently there is no data for Events , none of the processes are shown. 

> ::: toolkit/content/aboutTelemetry.js
> (Diff revision 1)
> > -    "keyed-scalars-section",
> > -    "histograms-section",
> > -    "keyed-histograms-section",
> > -    "events-section"
> > -  ];
> > -  let processes = document.getElementById("processes");
> 
> This means we can get rid of #processes from aboutTelemetry.xhtml as well.

I think it won't be possible to remove #processes from aboutTelemetry.xhtml , because it's being used in 'PingPicker' and 'displayRichPingData' function as well.

Comment 30

[Code Review Bot [:reviewbot]]

Comment on attachment 8984660 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry,

https://reviewboard.mozilla.org/r/250528/#review256846


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/aboutTelemetry.js:1868
(Diff revision 1)
> +
> +   addSubSection(aId, aPayload, prOptions, Events.eventsSelect);
> +
> +  },
> +
> +   

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/aboutTelemetry.js:2333
(Diff revision 1)
> +  histogramSelect(aProcess, aPayload) {
>      let hgramDiv = document.getElementById("histograms");
>      removeAllChildNodes(hgramDiv);
>  
>      let histograms = {};
> -    let hgramsSelect = document.getElementById("processes");
> +    

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/aboutTelemetry.js:2391
(Diff revision 1)
> +  keyedHistogramSelect(aProcess, aPayload) {
>      let keyedDiv = document.getElementById("keyed-histograms");
>      removeAllChildNodes(keyedDiv);
>  
>      let keyedHistograms = {};
> -    let keyedHgramsSelect = document.getElementById("processes");
> +    

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

Comment 31

[Chris H-C :chutten]

Comment on attachment 8984660 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry,

https://reviewboard.mozilla.org/r/250528/#review257334

The linting errors need to be fixed (thank you, :reviewbot), and in addition we should remove the process select from the xhtml (and any localised string entities from aboutTelemetry.dtd). But then that should be it!

Comment 32

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry ,

Review commit: https://reviewboard.mozilla.org/r/251208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251208/

Comment 33

[Chris H-C :chutten]

The specific element I expect we can remove is this one here: https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/toolkit/content/aboutTelemetry.xhtml#130

It will never be shown, populated, or have mouse events, so we can remove that element and any code that calls getElementById("processes"), like https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/toolkit/content/aboutTelemetry.js#282

Comment 34

[Chris H-C :chutten]

Comment on attachment 8985695 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/251208/#review257586

::: toolkit/content/aboutTelemetry.js:1779
(Diff revision 1)
> +  checkProcessData(aId, aPayload) {
> +    let prOptions = [];
> +    var i = 0;
> +    for (let f of gOptions) {
> +      if (f === "parent" && aPayload.keyedScalars) {
> +         prOptions[i] = f;

For these loops, couldn't we skip the "i" part and use prOptions.push(f) instead?

Comment 35

[Chris H-C :chutten]

Is there anything I can help with on this?

Comment 36

[Chris H-C :chutten]

Comment on attachment 8985695 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/251208/#review258520

Comment 37

[akriti verma]

Hi Chris,
I'm sorry for a late reply. I had some work , so couldn't work on it. I am uploading a new patch with the required changes.
Please let me know if more changes are needed on this one.
Thanks.

Comment 38

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry ,

Review commit: https://reviewboard.mozilla.org/r/252256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252256/

Comment 39

[Chris H-C :chutten]

Comment on attachment 8986997 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/252256/#review258816

I apologize for not going this deeply on the review earlier. I should have recommended naming changes and highlighted the dead code much earlier.

Most of what I have to say is tiny variable naming things to make the code read easier for the future. Naming is hard, so if any of my suggestions are wrong or could be improved on, please let me know.

::: toolkit/content/aboutTelemetry.js:50
(Diff revision 1)
>  
>  // Cached value of document's RTL mode
>  var documentRTLMode = "";
>  
> +// Data Collection Processes
> +var gOptions = ["parent", "content", "extension", "dynamic", "gpu"];

For clarity we should probably call this gProcessTypes

::: toolkit/content/aboutTelemetry.js:1714
(Diff revision 1)
>    render(aPayload) {
> +    this.scalarSelect("parent", aPayload);
> +    this.checkProcessData("scalars-section", aPayload);
> +  },
> +
> +  checkProcessData(aId, aPayload) {

This is more of a "addSubsectionsForProcessData" kind of method, no?

::: toolkit/content/aboutTelemetry.js:1715
(Diff revision 1)
> +    this.scalarSelect("parent", aPayload);
> +    this.checkProcessData("scalars-section", aPayload);
> +  },
> +
> +  checkProcessData(aId, aPayload) {
> +    let prOptions = [];

The important information about this array is not that the processes are options, but that they are process names, I think. So maybe we should call it processNames.

::: toolkit/content/aboutTelemetry.js:1716
(Diff revision 1)
> +    this.checkProcessData("scalars-section", aPayload);
> +  },
> +
> +  checkProcessData(aId, aPayload) {
> +    let prOptions = [];
> +    for (let f of gOptions) {

With it gProcessTypes we can use 'processType' instead of 'f' in these types of loop.

::: toolkit/content/aboutTelemetry.js:1725
(Diff revision 1)
> +               "scalars" in aPayload.processes[f]) {
> +           let scalars = aPayload.processes[f].scalars;
> +           if (Object.keys(scalars).length > 0 && scalars) {
> +              prOptions.push(f);
> +           }
> +        }

This should be one level less indented, right?

::: toolkit/content/aboutTelemetry.js:1726
(Diff revision 1)
> +           let scalars = aPayload.processes[f].scalars;
> +           if (Object.keys(scalars).length > 0 && scalars) {
> +              prOptions.push(f);
> +           }
> +        }
> +   }

The indentation here seems incorrect. Did you run your code through `./mach eslint -wo`? This should match the indentation of the f in "for", no?

::: toolkit/content/aboutTelemetry.js:2074
(Diff revision 1)
> +}
> +
> +/**
> + * Adds sub-sections
> + */
> +function addSubSection(aId, aPayload, aProcessList, aCallFunc) {

aProcessNames instead of aProcessList. Then the loop can use processName instead of f.

::: toolkit/content/aboutTelemetry.js:2477
(Diff revision 1)
>  
>      return result;
>    },
>  };
>  
>  function renderProcessList(ping, selectEl) {

We can remove this function as well now, right?

Comment 40

[Chris H-C :chutten]

To simplify the submission and review process, so long as you submit to mozreview a commit with the same MozReview-Commit-ID in the commit msg, it should take it and overlay it on top of the existing changes. That way I can chose to be shown only the things that have changed, making it faster for me to get back to you with your review.

Comment 41

[Chris H-C :chutten]

Oh, and also: Don't worry about the speed of your contributions. We're happy to have them at whatever speed makes you happy :)

Comment 42

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry ,

MozReview-Commit-ID : 7DLhmRPCn2k

Review commit: https://reviewboard.mozilla.org/r/252516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252516/

Comment 43

[Code Review Bot [:reviewbot]]

Comment on attachment 8987252 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/252516/#review259002


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/aboutTelemetry.js:49
(Diff revision 1)
>  var gPingData = null;
>  
>  // Cached value of document's RTL mode
>  var documentRTLMode = "";
>  
> +<<<<<<< local

Error: Parsing error: unexpected token << [eslint]

Comment 44

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry ,

Review commit: https://reviewboard.mozilla.org/r/253362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253362/

Comment 45

[Chris H-C :chutten]

Comment on attachment 8988110 [details]
Bug 1437446 : Make probe process choice more visible in about:telemetry ,

https://reviewboard.mozilla.org/r/253362/#review260016

We're down to just indentation style nits now!

...though it occurs to me that I removed the Events section in a patch that recently landed, so you may wish to rebase your patch before resubmitting for review. That way I'll be able to push it directly.

::: toolkit/content/aboutTelemetry.js:1714
(Diff revision 1)
>    render(aPayload) {
> +    this.scalarSelect("parent", aPayload);
> +    this.addSubsectionsForProcessData("scalars-section", aPayload);
> +  },
> +
> +  addSubsectionsForProcessData(aId, aPayload) {

This indentation fits the style of the rest of the file:

  addSubsectionsForProcessData(aId, aPayload) {
    let processNames = [];
    for (let processType of gProcessTypes) {
      if (processType === "parent" && aPayload.scalars) {
        processNames.push(processType);
      } else if ("processes" in aPayload &&
                 processType in aPayload.processes &&
                 "scalars" in aPayload.processes[processType]) {
        let scalars = aPayload.processes[processType].scalars;
        if (Object.keys(scalars).length > 0 && scalars) {
          processNames.push(processType);
        }
      }
    }

    addSubSection(aId, aPayload, processNames, Scalars.scalarSelect);

  },

::: toolkit/content/aboutTelemetry.js:1725
(Diff revision 1)
> +               "scalars" in aPayload.processes[processType]) {
> +           let scalars = aPayload.processes[processType].scalars;
> +           if (Object.keys(scalars).length > 0 && scalars) {
> +              processNames.push(processType);
> +           }
> +        }

This inner block should be outdented a little bit. "let" and "if" should line up like "processNames" does in the block above.

I'm actually a little surprised lint didn't ask you to move these, as the two-space indent rule is prevalent through the codebase.

::: toolkit/content/aboutTelemetry.js:1728
(Diff revision 1)
> +              processNames.push(processType);
> +           }
> +        }
> +    }
> +
> +   addSubSection(aId, aPayload, processNames, Scalars.scalarSelect);

This line only has three spaces of indent. It needs just one more to line up with the closing brace above it.

::: toolkit/content/aboutTelemetry.js:1784
(Diff revision 1)
> +              processNames.push(processType);
> +           }
> +        }
> +    }
> +
> +   addSubSection(aId, aPayload, processNames, KeyedScalars.keyedScalarSelect);

Same indendtation issues in this copy of addSubsectionsForProcessData

::: toolkit/content/aboutTelemetry.js:1840
(Diff revision 1)
>    render(aPayload) {
> +    this.eventsSelect("parent", aPayload);
> +    this.addSubsectionsForProcessData("events-section", aPayload);
> +  },
> +
> +  addSubsectionsForProcessData(aId, aPayload) {

and in this copy too

::: toolkit/content/aboutTelemetry.js:2291
(Diff revision 1)
>    render(aPayload) {
> +    this.histogramSelect("parent", aPayload);
> +    this.addSubsectionsForProcessData("histograms-section", aPayload);
> +  },
> +
> +  addSubsectionsForProcessData(aId, aPayload) {

and here

Comment 46

[Chris H-C :chutten]

I will be off on vacation for the next few weeks, so if you need any help in the interim, please reach out to Georg.

Comment 47

[akriti verma]

Attachment: Bug 1437446 : Make probe process choice more visible in about:Telemetry ,

Review commit: https://reviewboard.mozilla.org/r/259330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259330/

Comment 48

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8994794 [details]
Bug 1437446 : Make probe process choice more visible in about:Telemetry ,

https://reviewboard.mozilla.org/r/259330/#review266374

Thanks, i tested this out locally and it works really well!

There is one issue i noticed though:
When i click on a probe category (e.g. scalars), it initially shows all the "parent" probes, but the "Parent" subcategory is not shown as selected (blue).
When i specifically click on the "Parent" subcategory, it still shows the same probe data, but the "Parent" subcategory is shown as selected (blue).
I think if we only show the "Parent" probe data initially, we should already show it as selected.

The other issues i commented on below are just small style issues i'd like to see fixed.

::: toolkit/content/aboutTelemetry.js:1730
(Diff revision 1)
> +        }
> +      }
> +    }
> +
> +    addSubSection(aId, aPayload, processNames, Scalars.scalarSelect);
> +

A small thing: This empty line is redundant, let's remove it.
Also for the other `render()` functions below.

::: toolkit/content/aboutTelemetry.js:2061
(Diff revision 1)
> + */
> +function showSection(aSectionId) {
> +  if (!aSectionId) {
> +    return;
> +  }
> +  let current_selection = document.querySelector(".category-subsection.selected");

A small style issue:
Our coding style is to use `camelCase`, i.e. here you'd use `currentSelection`.

::: toolkit/content/aboutTelemetry.js:2310
(Diff revision 1)
> +        }
> +      }
> +    }
> +
> +    addSubSection(aId, aPayload, processNames, HistogramSection.histogramSelect);
> +

Also an empty line that should be removed.

Comment 49

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #48)
> There is one issue i noticed though:
> When i click on a probe category (e.g. scalars), it initially shows all the
> "parent" probes, but the "Parent" subcategory is not shown as selected
> (blue).
> When i specifically click on the "Parent" subcategory, it still shows the
> same probe data, but the "Parent" subcategory is shown as selected (blue).
> I think if we only show the "Parent" probe data initially, we should already
> show it as selected.

Here is an example to make this more clear:
https://drive.google.com/drive/folders/1KbZnPqUIUl7R86h8ZcF-QWfpxKZCimN4

Let me know if we can help figuring out how to fix this!

Comment 50

[clement.allain]

Hey ! Can I also do this one ?

It looks interesting to me :)

I know its almost done but maybe I could try some refactoring to avoid duplication.

Comment 51

[Chris H-C :chutten]

Sure! Though you may find that the patch as written will not apply cleanly because of recent changes like bug 1498169. The design may need a few tweaks, too :)

Comment 52

[clement.allain]

Speaking of design, I have checked all the about: pages and didn't find any equivalent.

I think there is 2 possibilities :

1. A third level in the sidebar
2. Tabs like this https://design.firefox.com/photon/components/tabs.html

Any preferences ?

Comment 53

[Chris H-C :chutten]

I really do like the look of Tabs, but it doesn't solve the core issue.

The main complaint we've received from our use of the process selector was that users were confused why we filtered to only the one header. Whatever solution we choose should show by default all the processes.

So if that means a third level in the sidebar, then so be it :(

However, we could also skip the sidebar and leave the process headers in the body. Which would you prefer?

Comment 54

[clement.allain]

After some discussions on IRC with Chris, we have concluded that it would be clearer to have the processes as second level menu in the sidebar (which was the first intent of this bug) and put the stores in the dropdown instead.

Clicking on the first level menu in the sidebar should display data from all processes.
A click on a process in the second level menu should filter data by this process.

The "main" store will be selected by default.

A label "Store" will be added near the dropdown.

Comment 55

[clement.allain]

Hello, I add this comment just to say I've started but I'm really busy this days but I hope to finish it before the end of the month or the start of may.

Comment 56

[clement.allain]

Attachment: Bug 1437446 - Make probe process choice more visible in about:telemetry

• Make process the second level menu of (Scalars, Keyed Scalars, Histograms, Keyed Histograms and Events)
• Change the process dropdown to be a store dropdown
• Main store is selected by default
• Added a label before the store dropdown
• Refactor a bit the code to avoid lot of duplications

Note: I don't know why this revision is mixed with the previous bug I've resolved :/

Comment 57

[Chris H-C :chutten]

Maybe the commit message has the wrong title? Phabricator said you had to change it "Dalc retitled this revision from Bug 1438896 - Add a probe to count Telemetry failures by ping type. to Bug 1437446 - Make probe process choice more visible in about:telemetry."

Comment 58

[clement.allain]

Attachment: Bug 1437446 : Make probe process choice more visible in about:telemetry

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc29b6ed69e8
Make probe process choice more visible in about:telemetry r=chutten,flod

Comment 60

[Chris H-C :chutten]

I have posted about the excellent work to the firefox-dev mailing list: https://mail.mozilla.org/pipermail/firefox-dev/2019-May/007067.html

Comment 61

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bc29b6ed69e8

Comment 62

[Georg Fritzsche [:gfritzsche]]

Thanks Clement, this is great to see!