Update about:telemetry to content process Telemetry changes

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: chutten)

Tracking

(Blocks 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1281795
(Reporter)

Updated

3 years ago
Blocks: 1218576
No longer depends on: 1218576
(Assignee)

Comment 1

3 years ago
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Attachment #8783657 - Flags: review?(gfritzsche)
(Reporter)

Comment 2

3 years ago
Look of about:telemetry with the patch applied.
(Reporter)

Comment 3

3 years ago
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)
Attachment #8783657 - Flags: review?(gfritzsche)
(Reporter)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
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.
(Reporter)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Attachment #8783657 - Attachment is obsolete: true
Attachment #8783932 - Attachment is obsolete: true
Attachment #8784350 - Flags: review?(gfritzsche)
(Assignee)

Comment 8

3 years ago
Posted image Screenshot of change (obsolete) —
(In reply to Chris H-C :chutten from comment #8)
> Created attachment 8784351 [details]
> Screenshot of change

Cool!
(Reporter)

Comment 10

3 years ago
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".
Attachment #8784350 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 11

3 years ago
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.
Attachment #8784350 - Attachment is obsolete: true
Attachment #8784351 - Attachment is obsolete: true
Attachment #8785326 - Flags: review+
(Assignee)

Comment 12

3 years ago
Posted image abouttel.PNG

Comment 13

3 years ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c606cacdd1d
Support aggregated child histograms in about:telemetry r=gfritzsche
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
Flags: needinfo?(chutten)

Comment 15

3 years ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/219efbc6aadb
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 16

3 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4630c8a51e
Support aggregated child histograms in about:telemetry r=gfritzsche
Backed this out together with bug 1218576 and relanded it.

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb4630c8a51e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 20

3 years ago
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.
Flags: needinfo?(chutten) → needinfo?(ryanvm)
Backed out per IRC discussion.
https://hg.mozilla.org/mozilla-central/rev/b3ec8a3373e8
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---

Comment 22

3 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9595c56c9db2
Support aggregated child histograms in about:telemetry r=gfritzsche
(Assignee)

Comment 24

3 years ago
Nothing wrong with this patch (that I can tell, anyway). Just isn't useful without the others.
Flags: needinfo?(chutten)

Comment 25

3 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/762e238a73d0
Support aggregated child histograms in about:telemetry r=gfritzsche

Comment 26

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/762e238a73d0
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.