List & render custom pings in about:telemetry

RESOLVED FIXED in Firefox 47

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: penhlenh, Mentored)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

4 years ago
The high-level objectives here are:
* all ping types are selectable through the ping selector section, not only "main" and "saved-session"
* when switching to a ping type that is not "main" or "saved-session" with the structured view:
  * we keep showing the "General Data" and "Environment Data" sections
  * hide the other data sections
  * show the payload of the ping in a "Payload" section as formatted stringified JSON
* switching to the "raw ping data" view should work just the same
(Reporter)

Comment 1

3 years ago
Penh Lenh, do you want to move on to this bug in this mini project?
Flags: needinfo?(penhlenh)
(Assignee)

Comment 2

3 years ago
Yes, Please assign to me.
Flags: needinfo?(penhlenh)
(Reporter)

Updated

3 years ago
Assignee: nobody → penhlenh
(Assignee)

Comment 3

3 years ago
Posted patch bug-1232917-fix-v0.patch (obsolete) — Splinter Review
Hi, Please check this patch.
Attachment #8706019 - Flags: review?(gfritzsche)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8706019 [details] [diff] [review]
bug-1232917-fix-v0.patch

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

This doesn't apply cleanly here. I think you need to update your source and rebase on that.
There were some changes recently that you will want to build on from bug 1236529.

::: toolkit/content/aboutTelemetry.js
@@ -331,5 @@
> -
> -    let element = document.getElementById("ping-source-structured");
> -    this.viewCurrentPingData = element.checked;
> -
> -    if (this.viewCurrentPingData) {

Was there a specific reason why you moved this away from here?
Does this make things easier or does it fix anything?

@@ +355,5 @@
>        // so we have to filter out everything else.
> +
> +      // No more filter, we want to show all ping type
> +      // pingList = pingList.filter(
> +      //   (p) => ["main", "saved-session"].indexOf(p.type) != -1);

We can just remove those lines.

@@ +1731,5 @@
> +      "slow-sql-section", "chrome-hangs-section",
> +      "thread-hang-stats-section", "histograms-section",
> +      "keyed-histograms-section", "simple-measurements-section",
> +      "late-writes-section", "addon-details-section",
> +      "addon-histograms-section" ].forEach(hideSection);

I think it's cleaner and more maintainable to approach this the other way around:
Hide all "data-section" element children of the "structured-ping-data-section"... except "general-data-section" & "environment-data-section".

@@ +1744,5 @@
> +      "slow-sql-section", "chrome-hangs-section",
> +      "thread-hang-stats-section", "histograms-section",
> +      "keyed-histograms-section", "simple-measurements-section",
> +      "late-writes-section", "addon-details-section",
> +      "addon-histograms-section" ].forEach(showSection);

Here we can just show all "data-section" elements.

::: toolkit/content/aboutTelemetry.xhtml
@@ +106,5 @@
>      <div id="raw-ping-data-section" class="hidden">
>        <pre id="raw-ping-data"></pre>
>      </div>
>  
> +    <div id="data-section-wrapper">

If you update your source you will see that we already have this wrapped in the "structured-ping-data-section"
Attachment #8706019 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

3 years ago
@@ -331,5 @@
It's caused the problem of reload page. Making raw data become null. But after I pull new source, it's working fine now.
(Assignee)

Comment 6

3 years ago
Posted patch bug-1232917-fix-v1.patch (obsolete) — Splinter Review
Please review again!
Attachment #8706019 - Attachment is obsolete: true
Attachment #8707578 - Flags: review?(gfritzsche)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8707578 [details] [diff] [review]
bug-1232917-fix-v1.patch

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

This doesn't handle custom pings with the structured view.

You can open the web console on about:telemetry and enter e.g.:
TelemetryController.submitExternalPing("foo", {a: 1, b: {c: 2}})

Then reload the page, choose "Archived Ping Data", "Structured" display, make sure you have the "foo" Ping selected.
You'll see an error in the terminal or the Browser Console (found in Tools -> Web Developer).
Either of those outputs should point you to the failing code.
Attachment #8707578 - Flags: review?(gfritzsche)
(Assignee)

Comment 8

3 years ago
Posted patch bug-1232917-fix-v2.patch (obsolete) — Splinter Review
Please review again.
Attachment #8707578 - Attachment is obsolete: true
Attachment #8708836 - Flags: review?(gfritzsche)
(Reporter)

Comment 9

3 years ago
Comment on attachment 8708836 [details] [diff] [review]
bug-1232917-fix-v2.patch

With the updated patch the error is gone and i can display view both the "General Data" and "Environment Data" for a test ping.


What doesn't work is showing the raw payload.
The check for |if (ping.payload.childPayloads)| seems wrong here.
If you look at the raw display of the "foo" ping, you will see that it has its data in |ping.payload| and that it doesn't have any |childPayloads|.

With that fixed, another high-level comment:
The raw payload should show up like the other expandable sections, i.e. with white background, a "Raw Payload" caption and the "Click to toggle section" note
Attachment #8708836 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

3 years ago
Posted patch bug-1232917-fix-v3.patch (obsolete) — Splinter Review
Please review again.
Attachment #8708836 - Attachment is obsolete: true
Attachment #8711462 - Flags: review?(gfritzsche)
(Reporter)

Comment 11

3 years ago
Comment on attachment 8711462 [details] [diff] [review]
bug-1232917-fix-v3.patch

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

This does not actually work for me:
* submit ping: TelemetryController.submitExternalPing("foo", {a: 1, b: {c: 2}})
* switch to raw data display
* switch to archived pings
* make sure you have the "foo" ping selected

This does not update the page correctly and i see an error in the browser console or the terminal:
Full Message: TypeError: can't convert undefined to object
Full Stack: sortStartupMilestones@chrome://global/content/aboutTelemetry.js:1737:20
displayPingData@chrome://global/content/aboutTelemetry.js:1881:28
PingPicker._updateArchivedPingData/<@chrome://global/content/aboutTelemetry.js:360:37
...


A comment on style:
In a few places you are missing a space, e.g. "for(..." or "if(..." where it should be "for (..." or "if (...".
Also, we want to usually want to use "let" instead of "var" in new code.

::: toolkit/content/aboutTelemetry.js
@@ +956,5 @@
>      div.appendChild(document.createElement("br"));
>    }
>  };
>  
> +var RawPayoad = {

This looks like a typo, it should be "RawPayload".

@@ +964,5 @@
> +  render: function(aPing) {
> +    hideSection();
> +    showRawSection();
> +
> +    if (!isEmpty(aPing.payload)) {

I don't think we need this.
If the payload is empty JSON.stringify gives us "{}", which is fine.

@@ +1735,5 @@
>      listEl.appendChild(option);
>    }
>  }
>  
> +function showSection() {

Improve the naming: showAllSections()

@@ +1742,5 @@
> +    elements[i].classList.remove("hidden");
> +  }
> +}
> +
> +function hideSection() {

Improved naming: hideMainPingSections()
In a comment we can point out that this hides the sections that are specific to "main" pings.

@@ +1782,5 @@
>    // Show environment data.
>    EnvironmentData.render(ping);
>  
> +  let elements = document.getElementById("ping-source-raw");
> +  if (!elements.checked) {

You are only triggering actions when "ping-source-raw" is checked.
We also need to do something when it is not checked.

::: toolkit/content/aboutTelemetry.xhtml
@@ +240,5 @@
>          <div id="addon-histograms" class="data">
>          </div>
>        </section>
>  
> +      <section id="addon-raw-payload-data-section" class="data-section">

This is not about addons, "raw-payload-data-section"?
Attachment #8711462 - Flags: review?(gfritzsche)
(Assignee)

Comment 12

3 years ago
Posted patch bug-1232917-fix-v4.patch (obsolete) — Splinter Review
I think it looks not so good but I try to make functionality work. Please review.
Attachment #8711462 - Attachment is obsolete: true
Attachment #8715361 - Flags: review?(gfritzsche)
(Reporter)

Comment 13

3 years ago
I got an error in the console with this:
* select an archived ping
* reload the page
* check the browser console:
> TypeError: can't convert null to object
> filterObject() aboutTelemetry.js:107
> GeneralData.render() aboutTelemetry.js:511
> displayPingData() aboutTelemetry.js:1833
> PingPicker.update() aboutTelemetry.js:347 - displayPingData(gPingData) with gPingData==null
> onLoad/<() aboutTelemetry.js:1693
> onLoad() aboutTelemetry.js:169

The problem is really in PingPicker.update():
When we load the page, we restore the form state like that "archived ping data" is selected.
This triggers loading a ping from disk, which is asynchronous, so we can't display it yet.
Because this is just after loading the page, we don't have any ping loaded into gPingData yet and can't use it.

If i take the displayPingData() call out, the following steps show the wrong data:
* view current ping data and structured
* switch to raw json
* switch to archived ping data and a custom ping
* switch to structured view
* ... now i see the structured data from the "current ping" again, not the archived custom ping

I think we should really move the code that handles making different sections visible etc. to PingPicker.update() and fix any remaining problems from there.
I'll put up a suggested cleanup for this on top of you patch.
(Reporter)

Comment 14

3 years ago
Posted patch Suggested cleanup (obsolete) — Splinter Review
The suggested code cleanup so we have more maintainable code flow. Feel free to integrate this in your patch and test it a bit.
(Reporter)

Comment 15

3 years ago
Comment on attachment 8715361 [details] [diff] [review]
bug-1232917-fix-v4.patch

One more high-level comment:
For a custom ping "foo", submitted like this:
> TelemetryController.submitExternalPing("foo", {a: 1, b: {c: 2}})
... we show the whole raw JSON on just one line without linebreaks & indentation in the "Raw Payload" section.
Let's put the stringified JSON in a "<pre>" element so we get the proper formatting.
Attachment #8715361 - Flags: review?(gfritzsche)
(Assignee)

Comment 16

3 years ago
Posted patch bug-1232917-fix-v5.patch (obsolete) — Splinter Review
Please review again.
Attachment #8715361 - Attachment is obsolete: true
Attachment #8715890 - Attachment is obsolete: true
Attachment #8717487 - Flags: review?(gfritzsche)
(Reporter)

Comment 17

3 years ago
Comment on attachment 8717487 [details] [diff] [review]
bug-1232917-fix-v5.patch

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

This seems to work pretty well now!
I only found one scenario where i get an error logged (not 100% its a new issue):

If you launch Firefox with a new profile (or probably after emptying the cache) and go to about:telemetry, something seems to not initialize right:

> A coding exception was thrown and uncaught in a Task.
> 
> Full message: TypeError: selected is null
> Full stack: PingPicker._getSelectedPingId@chrome://global/content/aboutTelemetry.js:455:5
> PingPicker._updateArchivedPingData@chrome://global/content/aboutTelemetry.js:359:14
> PingPicker._updateArchivedPingList<@chrome://global/content/aboutTelemetry.js:402:11
...

::: toolkit/content/aboutTelemetry.js
@@ +323,5 @@
>      let currentChanged = viewCurrent !== this.viewCurrentPingData;
>      let structuredChanged = viewStructured !== this.viewStructuredPingData;
>      this.viewCurrentPingData = viewCurrent;
>      this.viewStructuredPingData = viewStructured;
> +    

Nit: trailing whitespace.

@@ +1729,5 @@
> +  }
> +}
> +
> +function togglePingSections(isMainPing) {
> +  // this hides the sections that are specific to "main" pings.

Lets change the comment a bit to match what happens here:
// We always show the sections that are "common" to all pings.
// The raw payload section is only used for pings other than "main" and "saved-session".

@@ +1732,5 @@
> +function togglePingSections(isMainPing) {
> +  // this hides the sections that are specific to "main" pings.
> +  let commonSections = new Set(["general-data-section", "environment-data-section"]);
> +  let otherPingSections = new Set(["addon-raw-payload-data-section"]);
> +  

Nit: trailing whitespace.

::: toolkit/content/aboutTelemetry.xhtml
@@ +240,5 @@
>          <div id="addon-histograms" class="data">
>          </div>
>        </section>
>  
> +      <section id="addon-raw-payload-data-section" class="data-section">

This is not related to addons, lets rename this to say "raw-payload-section".
Attachment #8717487 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 18

3 years ago
Posted patch bug-1232917-fix-v6.patch (obsolete) — Splinter Review
Please review again.
Attachment #8717487 - Attachment is obsolete: true
Attachment #8724424 - Flags: review?(gfritzsche)
(Reporter)

Comment 19

3 years ago
Comment on attachment 8724424 [details] [diff] [review]
bug-1232917-fix-v6.patch

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

This looks good, thanks.
Only two small issues below, then i think this part is done.
As discussed elsewhere, lets put the fix for the error with a new profile in a separate patch on this bug.

::: toolkit/content/aboutTelemetry.js
@@ +1722,5 @@
>  }
>  
> +function toggleElementHidden(element, isHidden) {
> +  if (isHidden) {
> +    element.classList.add("hidden")

This is missing a semicolon at the end of the line.
Ditto the one below.

@@ +1769,5 @@
>    EnvironmentData.render(ping);
>  
> +  // We only have special rendering code for the payloads from "main" pings.
> +  // For any other pings we just render the raw JSON payload.
> +  let isMainPing = (ping.type === "main" || ping.type === "saved-session");

Nit: == is enough here.
Attachment #8724424 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 20

3 years ago
Attachment #8724424 - Attachment is obsolete: true
Attachment #8724561 - Flags: review+
(Reporter)

Comment 21

3 years ago
This works well now, thank you!
Keywords: checkin-needed

Comment 23

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