Make probe process choice more visible in about:telemetry

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
6 days ago

People

(Reporter: gfritzsche, Assigned: clement.allain, Mentored)

Tracking

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox68 fixed)

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(6 attachments, 11 obsolete attachments)

244.69 KB, image/png
Details
244.08 KB, image/png
Details
127.92 KB, image/png
Details
59 bytes, text/x-review-board-request
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
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.
Reporter

Updated

Last year
Attachment #8950147 - Attachment description: Screen Shot 2018-02-12 at 09.57.52.png → current process selector look
Mockup of (2), note the submenu entries on the left side.
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.
Mentor: chutten
Priority: -- → P3
Whiteboard: [good second bug][lang=js]
Reporter

Updated

Last year
Assignee: nobody → yarik.sheptykin

Comment 4

Last year
Hi,
I know this bug has been assigned, but i had already started working on it while it was assigned to 'nobody@mozilla.org'. 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
It is okay with me if it is okay with Iarsolav. what do you say?
Flags: needinfo?(yarik.sheptykin)
Comment hidden (mozreview-request)

Comment 7

Last year
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

Last year
Attaching a screenshot of the result.
Flags: needinfo?(chutten)
Hi Guys!

Absolutely no problem with me. Go ahead an take over the bug! Sorry for blocking here.
Flags: needinfo?(yarik.sheptykin)
Assignee: yarik.sheptykin → akriti.v10

Comment 10

Last year
mozreview-review
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?
Attachment #8969730 - Flags: review?(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.
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8969730 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8970127 - Attachment is obsolete: true
Attachment #8970127 - Flags: review?(chutten)

Comment 14

Last year
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.
Flags: needinfo?(chutten)

Comment 15

Last year
Thanks Iaroslav!

Comment 16

Last year
mozreview-review
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
Attachment #8970129 - Flags: review?(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...
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8970129 - Attachment is obsolete: true

Comment 19

Last year
mozreview-review
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?
Attachment #8970871 - Flags: review?(chutten)
Hey akriti, do you need some more help with this bug?
Flags: needinfo?(akriti.v10)

Comment 21

Last year
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?
Flags: needinfo?(akriti.v10)
(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 hidden (mozreview-request)

Updated

Last year
Attachment #8970871 - Attachment is obsolete: true

Comment 24

Last year
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

Last year
mozreview-review
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.
Attachment #8980991 - Flags: review?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8980991 - Attachment is obsolete: true

Comment 27

Last year
mozreview-review
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.
Attachment #8982812 - Flags: review?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8982812 - Attachment is obsolete: true

Comment 29

Last year
(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

Last year
mozreview-review
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

Last year
mozreview-review
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!
Attachment #8984660 - Flags: review?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8984660 - Attachment is obsolete: true
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

Last year
mozreview-review
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?
Is there anything I can help with on this?
Flags: needinfo?(akriti.v10)

Comment 36

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

https://reviewboard.mozilla.org/r/251208/#review258520
Attachment #8985695 - Flags: review?(chutten)

Comment 37

Last year
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.
Flags: needinfo?(akriti.v10) → needinfo?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8985695 - Attachment is obsolete: true

Comment 39

Last year
mozreview-review
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?
Attachment #8986997 - Flags: review?(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.
Flags: needinfo?(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 hidden (mozreview-request)

Updated

Last year
Attachment #8986997 - Attachment is obsolete: true

Comment 43

Last year
mozreview-review
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]
Attachment #8987252 - Flags: review?(chutten)
Comment hidden (mozreview-request)

Updated

Last year
Attachment #8987252 - Attachment is obsolete: true

Comment 45

Last year
mozreview-review
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
Attachment #8988110 - Flags: review?(chutten) → review-
Mentor: gfritzsche
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 hidden (mozreview-request)

Updated

11 months ago
Attachment #8988110 - Attachment is obsolete: true
Reporter

Comment 48

11 months ago
mozreview-review
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.
Attachment #8994794 - Flags: review?(gfritzsche)
Reporter

Comment 49

11 months ago
(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!
Assignee

Comment 50

3 months ago

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.

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

Assignee: akriti.v10 → clement.allain
Status: NEW → ASSIGNED
Assignee

Comment 52

3 months ago

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 ?

Flags: needinfo?(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?

Flags: needinfo?(chutten)
Assignee

Comment 54

3 months ago

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.

Assignee

Comment 55

2 months ago

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.

Assignee

Comment 56

2 months ago
  • 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 :/

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 59

Last month
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc29b6ed69e8
Make probe process choice more visible in about:telemetry r=chutten,flod

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

Last month
bugherder
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Thanks Clement, this is great to see!

Reporter

Updated

Last month
Blocks: 1550692
QA Whiteboard: [qa-68b-p2]
You need to log in before you can comment on or make changes to this bug.