1277504 - Update about:telemetry to content process Telemetry changes

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

After bug 1218576, we will have the content process histograms & keyed histograms in a different section on the main ping.

This means we will have to update about:telemetry to show those sections and to remove the previous code and markup for child payload selection.

Comment 1

[Chris H-C :chutten]

Attachment: 0001-bug-1277504-Add-a-content-process-section-to-about-t.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: about:telemetry screen shot with patch applied

Look of about:telemetry with the patch applied.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8783657 [details] [diff] [review]
0001-bug-1277504-Add-a-content-process-section-to-about-t.patch

Review of attachment 8783657 [details] [diff] [review]:
-----------------------------------------------------------------

(1) like done here, "X process" -> histograms/scalars/...
(2a) the histograms/scalars/... sections each having a "process" switch (main, content, ...)
(2b) similar, but with a global/top "process" switch

I think we should go for (2a):
* seems structurally clearer
* can be extended to adding a "all processes" entry
* scales better with adding more process types
* (2a) seems more convenient for exploring the data than (2b)

Comment 4

[Georg Fritzsche [:gfritzsche]]

I meant to add:
There are at least two options for the structure:
> (1) like done here, "X process" -> histograms/scalars/...
> (2a) the histograms/scalars/... sections each having a "process" switch
> (main, content, ...)
> (2b) similar, but with a global/top "process" switch
> 
> I think we should go for (2a):
> * seems structurally clearer
> * can be extended to adding a "all processes" entry
> * scales better with adding more process types
> * (2a) seems more convenient for exploring the data than (2b)

Comment 5

[Chris H-C :chutten]

The question then becomes one of usability. What is about:telemetry useful for? What is it _used_ for? What use cases should we simplify? How friendly do we want it to be, and to what audience?

For instance:
* 1) makes it easier to compare histogram collection between processes, where 2x makes it impossible (though "all processes" becomes interesting. Will it be confusing that there are multiple copies of each hgram? How will we differentiate them? Flag histograms suck, by the by).
* 2x) are cleaner on the top level, but the top level doesn't provide any information.

Y'know, the hard design-type questions :S

Should we pull the "Modernize/Organize about:telemetry" out as a separate concern? I'm not sure we have sufficient information on usage patterns to make lasting design decisions here. Also complicating things is that simplemeasurements are still in childpayloads. This means we still need a child payload selector somewhere, independent of any process selectors.

Luckily, I don't think this page is used by anyone who isn't technologically savvy.

Unluckily, I don't think this page is used by anyone who isn't technologically savvy.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #5)
> The question then becomes one of usability. What is about:telemetry useful
> for? What is it _used_ for? What use cases should we simplify? How friendly
> do we want it to be, and to what audience?

It's fine for this to be pretty technical right now.

> Should we pull the "Modernize/Organize about:telemetry" out as a separate
> concern? I'm not sure we have sufficient information on usage patterns to
> make lasting design decisions here.

Yes, we shouldn't address the bigger "modernize about:telemetry" topic here (ideally we'd get someone from UX to help kick of a reboot).
But we should choose a good option for now that will work for the next months.

Following the current pattern, structuring this by "payload sub categories" and "data types" seems to work out ok.

> Also complicating things is that
> simplemeasurements are still in childpayloads. This means we still need a
> child payload selector somewhere, independent of any process selectors.

Yeah, i could adopt these to the same style in a follow-up or so.

Comment 7

[Chris H-C :chutten]

Attachment: 0001-bug-1277504-Support-aggregated-child-histograms-in-a.patch

Comment 8

[Chris H-C :chutten]

Attachment: Screenshot of change

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #8)
> Created attachment 8784351 [details]
> Screenshot of change

Cool!

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8784350 [details] [diff] [review]
0001-bug-1277504-Support-aggregated-child-histograms-in-a.patch

Review of attachment 8784350 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nice, cheers.
r=me with the below addressed, let me know if anything is unclear.

::: toolkit/content/aboutTelemetry.css
@@ +233,5 @@
> +  display: none;
> +}
> +
> +.has-data.expanded .processes-ui {
> +  display: initial;

Why not `inline`?

::: toolkit/content/aboutTelemetry.js
@@ +1788,5 @@
>  
> +function renderProcessList(ping, selectEl) {
> +  removeAllChildNodes(selectEl);
> +  let option = document.createElement("option");
> +  let text = bundle.GetStringFromName("parentProcess");

We don't translate the other entries either, let's just use the literal string here.

@@ +1805,5 @@
> +    // TODO: parent hgrams are on root payload, not in payload.processes.parent
> +    // When/If that gets moved, you'll need to remove this:
> +    if (process === "parent") {
> +      continue;
> +    }

Lets not silence this so we actually discover this later.
Lets throw an error here or log to the error console.

@@ +1885,1 @@
>    // Update the payload list

Nit: Update comment.

@@ +1976,2 @@
>    hasData = Object.keys(histograms).length > 0;
> +  setHasData("histograms-section", hasData || hgramsSelect.options.length);

Doesn't this lead to strange behavior when switching to, say, an empty "content" histograms entry?
Ditto for keyed histograms below.

::: toolkit/content/aboutTelemetry.xhtml
@@ +164,4 @@
>          <h1 class="section-name">&aboutTelemetry.keyedHistogramsSection;</h1>
>          <span class="toggle-caption">&aboutTelemetry.toggle;</span>
>          <span class="empty-caption">&aboutTelemetry.emptySection;</span>
> +        <div class="processes-ui">

Nit: This could visually use a bit space to "click to toggle section".

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +81,5 @@
>  # Note to translators:
>  # - The %1$S will be replaced with the number of the child payload (e.g. "1", "2")
>  childPayloadN = Child Payload %1$S
> +
> +parentProcess = Parent

Lets make this lower-case, currently the case doesn't match with "content".

Comment 11

[Chris H-C :chutten]

Attachment: 0001-bug-1277504-Support-aggregated-child-histograms-in-a.patch

Comment on attachment 8784350 [details] [diff] [review] [diff] [review]
0001-bug-1277504-Support-aggregated-child-histograms-in-a.patch

Review of attachment 8784350 [details] [diff] [review] [diff] [review]:
-----------------------------------------------------------------

This looks nice, cheers.
r=me with the below addressed, let me know if anything is unclear.

::: toolkit/content/aboutTelemetry.css
@@ +233,5 @@
> +  display: none;
> +}
> +
> +.has-data.expanded .processes-ui {
> +  display: initial;

>Why not `inline`?

Because `initial` makes no judgment about what the .processes-ui's original display was.

::: toolkit/content/aboutTelemetry.js
@@ +1788,5 @@
>  
> +function renderProcessList(ping, selectEl) {
> +  removeAllChildNodes(selectEl);
> +  let option = document.createElement("option");
> +  let text = bundle.GetStringFromName("parentProcess");

>We don't translate the other entries either, let's just use the literal string here.

Done.

@@ +1805,5 @@
> +    // TODO: parent hgrams are on root payload, not in payload.processes.parent
> +    // When/If that gets moved, you'll need to remove this:
> +    if (process === "parent") {
> +      continue;
> +    }

>Lets not silence this so we actually discover this later.
>Lets throw an error here or log to the error console.

This is not an error condition, this is just a side-effect of there being a processes.parent, but no processes.parent.histograms or processes.parent.keyedHistograms.

There will be other changes necessary in this file when that happens, so we don't really need a reminder.

@@ +1885,1 @@
>    // Update the payload list

> Nit: Update comment.

Done.

@@ +1976,2 @@
>    hasData = Object.keys(histograms).length > 0;
> +  setHasData("histograms-section", hasData || hgramsSelect.options.length);

>Doesn't this lead to strange behavior when switching to, say, an empty "content" histograms entry?
>Ditto for keyed histograms below.

As in the screenshot, if you switch to a process with empty content, it disables the whole section, preventing you from changing process. This way you always have access to the process selector.

::: toolkit/content/aboutTelemetry.xhtml
@@ +164,4 @@
>          <h1 class="section-name">&aboutTelemetry.keyedHistogramsSection;</h1>
>          <span class="toggle-caption">&aboutTelemetry.toggle;</span>
>          <span class="empty-caption">&aboutTelemetry.emptySection;</span>
> +        <div class="processes-ui">

>Nit: This could visually use a bit space to "click to toggle section".

0.5em of horizontal margin added.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +81,5 @@
>  # Note to translators:
>  # - The %1$S will be replaced with the number of the child payload (e.g. "1", "2")
>  childPayloadN = Child Payload %1$S
> +
> +parentProcess = Parent

>Lets make this lower-case, currently the case doesn't match with "content".

Done.

Comment 12

[Chris H-C :chutten]

Attachment: abouttel.PNG

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c606cacdd1d
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 14

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

Backed out for build bustage:

    2c7ef95c2785 SH Backed out changeset 74b9b3f4ce25 (bug 1218576) for build bustage. r=backout on a CLOSED TREE
    d9deb379d830 SH Backed out changeset 7537a79e91a4 (bug 1218577)
    b64e58e453e1 SH Backed out changeset 2310da8e4942 (bug 1218576)
    c249998a085f SH Backed out changeset cd8049475224 (bug 1218576)
    6a4b7a63125d SH Backed out changeset 6e4a7610406a (bug 1218576)
    f327e2486a4b SH Backed out changeset 2d375de64477 (bug 1218576)
    ac368b44eb34 SH Backed out changeset 59b0fb3fa328 (bug 1218576)
    1092ce552e86 SH Backed out changeset 0397f79f86ad (bug 1218576)
    70b93415fbef SH Backed out changeset 283cee5fb8a7 (bug 1218576)
    7e6a9af6246b SH Backed out changeset 7f8e3d336e2f (bug 1218576)
    20775bc718db SH Backed out changeset aee17ede1c0e (bug 1218576)
    9470c7a31cf2 SH Backed out changeset 80b0680f916f (bug 1218576)
    452bab4b12e9 SH Backed out changeset 3c5d66ad69a0 (bug 1218576)
    53f5c4b914c4 SH Backed out changeset ce72d25d0c5b (bug 1218576)
    188b32f8df91 SH Backed out changeset 3c606cacdd1d (bug 1277504)
    15ad29361e48 SH Backed out changeset fe56ce42df42 (bug 1290027)

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fe56ce42df420209cc940b2e88b676495c933d00

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/219efbc6aadb
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4630c8a51e
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 17

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

Backed this out together with bug 1218576 and relanded it.

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5ac9c5221e
Backed out changeset 219efbc6aadb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Chris H-C :chutten]

This will make about:telemetry not work properly for child payloads until bug 1218576 lands. I'm not sure this should have been pushed independently.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out per IRC discussion.
https://hg.mozilla.org/mozilla-central/rev/b3ec8a3373e8

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9595c56c9db2
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All of the stuff from this push (including bug 1218576 and bug 1218577) backed out for ASAN bc2 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35454365&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1530103e032c

Comment 24

[Chris H-C :chutten]

Nothing wrong with this patch (that I can tell, anyway). Just isn't useful without the others.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/762e238a73d0
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/762e238a73d0