1163971 - Show child payload data in about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We should show the child payload data in about:telemetry.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I think it's not clear yet whether we want to aggregate the child data or not, the work here depends on that.

I can think of some display options:
* add new separate sections for all child data parts
* do some plain key-value table rendering of the child data payload(s) for now
* in sections that are present in child payloads (e.g. histograms), allow switching between "parent" and "child N" data

Comment 2

[Vladan Djeric (:vladan)]

One idea is to have a small sidebar on the left side of the page with an expanding list: Current session, Unsent pings, Archived pings. Clicking on each one expands to a list of pings. If the user clicks on an individual ping, and the ping is an e10s ping, new subitems pop out: Parent process, Child #1, Child #2, etc

It wouldn't be pretty but it would expose all the information and the sidebar wouldn't take up too much screen real estate

Comment 3

[Vladan Djeric (:vladan)]

Obviously, clicking on a "leaf node" in the sidebar would display the selected data in the main page panel

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2)
> One idea is to have a small sidebar on the left side of the page with an
> expanding list: Current session, Unsent pings, Archived pings. Clicking on
> each one expands to a list of pings. If the user clicks on an individual
> ping, and the ping is an e10s ping, new subitems pop out: Parent process,
> Child #1, Child #2, etc

Or we just extend the "ping data source" section with a selector for the parent/child1/child2/... data.
For screen real-estate, keep in mind that we currently show the same design on Firefox for Android - we probably shouldn't make it harder to use.

Comment 5

[Chris H-C :chutten]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Or we just extend the "ping data source" section with a selector for the
> parent/child1/child2/... data.
> For screen real-estate, keep in mind that we currently show the same design
> on Firefox for Android - we probably shouldn't make it harder to use.

I confess that was going to be my first approach. Just expand on the _renderPingList paradigm and put in another <select> for any child payloads that might be present. The layout switch on archived-vs-current will be a little more noisy, but I figured it'd be a decent first pass.

Thank you for pointing out that any layout needs to be responsive, I would've missed that.

Comment 6

[Chris H-C :chutten]

Attachment: 0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

I wasn't sure if all of the i18n strings ought to be duplicated across .properties/.dtd or only the ones that are used in .js/.xhtml respectively, so I erred on the side of duplication.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8680015 [details] [diff] [review]
0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

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

I put specific comments on l10n usage inline, but you can find some general background on the l10n technologies involved here:
* the html/xul entity localization: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Localization
* the l10n used for Javascript: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Property_Files

I think its a little confusing now which sections are actually updated from the child payload and which are filled from the parent data...
On the other hand it's a big improvement to display the data at all and we can worry about that later.

I think we can eliminate one source of confusion by moving the "System Information" section up in the page, right below the "Environment Data" section.
That way we are at least grouping the sections together that are always filled from the parent & common data.

::: toolkit/content/aboutTelemetry.js
@@ +329,1 @@
>      displayPingData(ping);

We should have all code updating the page from a pings data in one place instead of repeating the two same function calls in more than one place.

While i see below why it doesn't fit directly into |displayPingData()|, we could instead:
* make |displayPingData()| take an optional argument like "updatePayloadList" (preferred) or
* add a new function, say, |updatePageFromPing()| that calls both |updatePayloadList()| and |displayPingData()|

@@ +329,4 @@
>      displayPingData(ping);
>    },
>  
> +  _updatePayloadList: function(ping) {

Nit: This has a big wall of code below - can you throw in some newlines and (short) comments on what it is doing?

@@ +344,5 @@
> +      return;
> +    }
> +    for (let payload of ping.payload.childPayloads) {
> +      option = document.createElement("option");
> +      text = bundle.GetStringFromName("childPayload") + " - " + payloadIndex;

Different locales can have different orders (or infix, prefix, ... semantics) for the index.
You need to use |formatStringFromName()| here, with a string of |
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIStringBundle#formatStringFromName%28%29

@@ +1590,4 @@
>    AddonDetails.render(ping);
>  
>    // Show simple measurements
> +  let payloadIndex = document.getElementById("choose-payload").selectedOptions.item(0).getAttribute("value");

Nit: This line is pretty long, lets break it down over two lines.

@@ +1591,5 @@
>  
>    // Show simple measurements
> +  let payloadIndex = document.getElementById("choose-payload").selectedOptions.item(0).getAttribute("value");
> +  let payload;
> +  if (payloadIndex == 0) {

I think this belongs in its own section "// Select payload to render" or so before "//Show simple measurements".

Nit: This should also be a bit more readable as:
let payload = ping.payload;
if (payloadIndex > 0) {
  ...

@@ +1611,5 @@
>    LateWritesSingleton.renderLateWrites(payload.lateWrites);
>  
>    // Show basic system info gathered
> +  let payloadInfo = payload.info || ping.payload.info;
> +  hasData = Object.keys(payloadInfo).length > 0;

We don't add the |info| section for child payloads:
https://dxr.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d/toolkit/components/telemetry/TelemetrySession.jsm#1272
... so we should just always use |ping.payload.info|.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ +139,5 @@
> +<!ENTITY aboutTelemetry.payloadChoiceHeader "
> +  Payload
> +">
> +
> +<!ENTITY aboutTelemetry.childPayload "

This file contains the localizations for the xhtml file, |childPayload| and |parentPayload| are not needed here.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +69,5 @@
>  # Note to translators:
>  # - The %1$S will be replaced with the name of an Add-on Provider (e.g. "XPI", "Plugin")
>  addonProvider = %1$S Provider
> +
> +payloadChoiceHeader = Payload

This file contains the strings used from Javascript, this entry is not needed here.

@@ +73,5 @@
> +payloadChoiceHeader = Payload
> +
> +parentPayload = Parent Payload
> +
> +childPayload = Child Payload

This needs to have a placeholder for the index, "Child Payload %1$S".
Also add a note to translators preceding this, similar to |addonProvider| above.

Comment 8

[Chris H-C :chutten]

Attachment: 0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

Changes incorporated.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> I think we can eliminate one source of confusion by moving the "System
> Information" section up in the page, right below the "Environment Data"
> section.
> That way we are at least grouping the sections together that are always
> filled from the parent & common data.

Can we do this simple change in this bug too?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8680187 [details] [diff] [review]
0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

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

::: toolkit/content/aboutTelemetry.js
@@ +1538,3 @@
>    gPingData = ping;
>  
> +  if (updatePayloadList) {

Updating the payload list is unrelated to the rest of this function; let's keep this in a separate helper function and just call it from here.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Testing the patch out a little, i think we should make the payload selector disabled when there are no child payloads.
That way i don't have to open it first to find out if there are any child payloads for the currently displayed ping.

Comment 12

[Chris H-C :chutten]

Attachment: 0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

Ideally we'd have some sort of bound on the number of child payloads that we could expect and use a radio button group or something instead of select (because selects are evil, especially on mobile) and then we wouldn't be hiding useful information.

Out of scope for this bug, anyway.

Comment 13

[Chris H-C :chutten]

Attachment: 0002-bug-1163971-Bring-the-System-Info-section-up-just-be.patch

Here's the patch for reordering the sections

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8680604 [details] [diff] [review]
0002-bug-1163971-Bring-the-System-Info-section-up-just-be.patch

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

Note: the commit message stretches over two lines, you'll want to fix that for landing the patch.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8680603 [details] [diff] [review]
0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

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

r=me with the below addressed.

::: toolkit/content/aboutTelemetry.js
@@ +1555,5 @@
> +    return
> +  }
> +  listEl.disabled = false;
> +
> +  for (let payload of ping.payload.childPayloads) {

Instead of tracking payloadIndex somewhere in the loop body, lets make this explicit in the loop header:

for (let idx = 0; idx < ping.payload.childPayloads.length; ++idx) {
  ...
  option.setAttribute("value", idx + 1);

@@ +1565,5 @@
> +    listEl.appendChild(option);
> +  }
> +}
> +
> +function displayPingData(ping, updatePayloadList) {

Use a default argument value:
function displayPingData(ping, updatePayloadList = false)

Comment 16

[Chris H-C :chutten]

Attachment: 0001-bug-1163971-Add-a-simple-select-to-about-telemetry-t.patch

Comment 17

[Chris H-C :chutten]

Attachment: 0002-bug-1163971-Bring-the-System-Info-section-up-just-be.patch

Comment 18

[Chris H-C :chutten]

No try jobs done because about:telemetry has no test coverage or build impact.

Manual testing done:
Tested with and without child payloads
Tested a narrow window to emulate mobile
Tested with dir=rtl to ensure the layout was acceptable

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/31e72022d3ff
https://hg.mozilla.org/integration/fx-team/rev/78f8c8c84ccd

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/31e72022d3ff
https://hg.mozilla.org/mozilla-central/rev/78f8c8c84ccd

Comment 21

[Francesco Lodolo [:flod]]

Does it work?

childPayload = Child Payload %1$s

Notice the lowercase s instead of S. 
https://bugzilla.mozilla.org/show_bug.cgi?id=1133554#c1

If you need to fix the string, you will also need a new string ID.

Comment 22

[Chris H-C :chutten]

It does work insofar as it worked for my tests. But yes, lowercase s appears to take only one character of the substituted string.

Quoth Yzma: Why do we even have that lever?

Better it should fail explosively than work only by taking one character... unless there is a valid usecase that I can't see.

This will work until pings contain more than 9 child payloads. I have filed bug 1220376 to change the id and the format string.

Where should I file a bug to make lowercase s fail explosively instead of sneakily?

Comment 23

[Francesco Lodolo [:flod]]

(In reply to Chris H-C :chutten from comment #22)
> Where should I file a bug to make lowercase s fail explosively instead of
> sneakily?

Unfortunately I don't have any advice. I'm only aware of the issue because I work on l10n and strings.
I tried to follow the code a few times and got lost in the process.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/31e72022d3ff
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/78f8c8c84ccd