1175519 - Expose raw ping json data on about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Mark Reid [:mreid]]

It's convenient from a data perspective to be able to investigate the raw json for telemetry in your own profile. The ".jsonlz4" file storage makes in hard to view the data locally, but we could add it to about:telemetry.

It would also be nice for data transparency to see exactly what will be submitted.

Comment 1

[Georg Fritzsche [:gfritzsche]]

* add a div with id "raw-ping-data" around here:
  https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/toolkit/content/aboutTelemetry.xhtml#87
* add a "raw-ping-data" button around here:
  https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/toolkit/content/aboutTelemetry.xhtml#85
* attach an event handler around here:
  https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/toolkit/content/aboutTelemetry.js#279
* the button label should be "&aboutTelemetry.rawPingData;", the actual text (say "Raw ping data ...") goes here (so it can be localized):
  https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd#54
* make this show the "raw-ping-data" div that shows the formatted ping JSON data, e.g.: JSON.stringify(gPingData, null, 2)

The layout for the "raw-ping-data" can be pretty simple:
* make it float on top as an overlay
* show the ping data in a read-only textarea or so
* give it a close button, e.g. an "x" in the top-right via ✖
* the styling for the overlay goes into this file: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.css

Comment 2

[simplyblue24]

Attachment: 1175519.diff

Hi, I would like to work on this bug.Can you please assign the bug to me. :)
I also need some more information to proceed.Right now I have a |div| with id |raw-ping-data| and a button with label |&aboutTelemetry.rawPingData;| and i have updated aboutTelemetry.dtd.Please have look at the attachment for the changes i have made.Can please tell what should be the |id|  of the button and for the event handler what kind of event has to be handled and what should be done when the event occurs. Thank you very much.!!

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to simplyblue24 from comment #2)
> Hi, I would like to work on this bug.Can you please assign the bug to me. :)
> I also need some more information to proceed.Right now I have a |div| with
> id |raw-ping-data| and a button with label |&aboutTelemetry.rawPingData;|
> and i have updated aboutTelemetry.dtd.Please have look at the attachment for
> the changes i have made.Can please tell what should be the |id|  of the
> button and for the event handler what kind of event has to be handled and
> what should be done when the event occurs. Thank you very much.!!

Hi, i assigned it to you :)
The changes look like the right direction.
I would suggest "show-raw-ping" as the button id.

The event listener would be for "click", you can see other button handlers being attached for e.g. the "newer-ping" button.
As mentioned in comment 1, that handler would:
* make the "raw-ping-data" div visible and
* update the textarea or similar in it with the raw ping data as text, from JSON.stringify(gPingData, null, 2)

Comment 4

[simplyblue24]

Attachment: 1175519.diff

Hi, can you please provide feedback on the changes. Can you please provide more info on how to style the element.Thanks :)

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8682500 [details] [diff] [review]
1175519.diff

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

This is coming along well!

::: toolkit/content/aboutTelemetry.js
@@ +449,5 @@
>    },
> +
> +  _showRawPingData: function() {
> +    let pingData = document.getElementById("raw-ping-data");
> +    let textArea = pingData.firstChild;

I think it's better to give the textarea an id ("raw-ping-text"?) and use getElementById instead.

@@ +450,5 @@
> +
> +  _showRawPingData: function() {
> +    let pingData = document.getElementById("raw-ping-data");
> +    let textArea = pingData.firstChild;
> +    textArea.readOnly = false;

We never need to set .readOnly to false, setting .textContent from still works on a readonly textarea.

Comment 6

[Georg Fritzsche [:gfritzsche]]

For styling i think we can keep it rather simple.
What we want to for the raw-ping-data div is:
* it should start out hidden
* it should show on clicking the raw ping data button
* it should float on top and fill out all or most of the page
* have a close button in the top-right (✖)
* on clicking that button it should hide again

We can show and hide the raw ping data by adding and removing the CSS class "hidden" similar to this code:
https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/toolkit/content/aboutTelemetry.js#312

The CSS styling goes into aboutTelemetry.css:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.css

We can make the raw-ping-data div go over the rest of the page using "position: absolute;" there.

Comment 7

[simplyblue24]

Attachment: tel.diff

Hi, you please review the changes.Thanks :)

Comment 8

[Georg Fritzsche [:gfritzsche]]

Sure, thank you - today was too busy, but i will definitely look tomorrow.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8683062 [details] [diff] [review]
tel.diff

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

Thanks, this looks pretty functional already :)
The comments below are mostly about styling & layout.

::: toolkit/content/aboutTelemetry.css
@@ +229,5 @@
>  }
> +
> +#raw-ping-data-section {
> +  position: absolute;
> +  background-color:#FFF;

To make it cover the page, we will have to use these too:
  width: 100%;
  height: 100%;
  top: 0px;

For background-color, i suggest "-moz-Dialog" for #raw-ping-data-section and "white" for #raw-ping-data.

I think the "x" at the top should be more visible, i propose to make it bigger and give it a noticeable background-color different from the surroundings (so you get a noticeable rectangle with an "x" in it).

::: toolkit/content/aboutTelemetry.xhtml
@@ +81,5 @@
>                  </td>
>              </tr>
>            </table>
>          </div>
> +        <button id="show-raw-ping" type="button">&aboutTelemetry.rawPingData;</button>

Looking at the layout now, i think this is placed better right after the "ping-source-picker" div, can you move it up there?

@@ +98,5 @@
>      </header>
>  
> +    <div id="raw-ping-data-section" class="hidden">
> +      <div id="hide-raw-ping">&#10006;</div>
> +      <textArea id="raw-ping-data" readonly="readonly"></textArea>

Sidenote: This has the wrong casing, "textArea" vs. "textarea" (this was the reason the text formatting was lost).

Testing around a bit, i notice that "<pre>" actually works better here - it is always read-only and doesn't have resize controls etc.
Lets use "<pre>" instead here.

Comment 10

[simplyblue24]

Attachment: Bug-1175519.patch

Please review the patch. Thank you :)

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8683804 [details] [diff] [review]
Bug-1175519.patch

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

This looks pretty useful now! Two smaller things below, then i think we can go ahead with this.

::: toolkit/content/aboutTelemetry.css
@@ +236,5 @@
> +  left: 0px;
> +  background-color:-moz-Dialog;
> +}
> +
> +#raw-ping-data {

Lets give this 0px margin.

::: toolkit/content/aboutTelemetry.xhtml
@@ +58,5 @@
>            <input type="radio" id="ping-source-archive" name="choose-ping-source" value="archive" />
>            &aboutTelemetry.showArchivedPingData;<br />
>          </div>
> +        <button id="show-raw-ping" type="button">&aboutTelemetry.rawPingData;</button>
> +        <div id="raw-ping-data-section" class="hidden">

I meant to only move the <button>, the raw-ping-data-section was in the right location already (just before general-data-section).

Comment 12

[simplyblue24]

Attachment: bug1175519.patch

Hi, please review the patch.Hope it looks good now :)

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8684217 [details] [diff] [review]
bug1175519.patch

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

You added the margin to #raw-ping-data-section instead of #raw-ping-data, otherwise this looks good :)

Comment 14

[simplyblue24]

Attachment: Bug1175519.patch

My bad :P. This should look good now :)

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8684220 [details] [diff] [review]
Bug1175519.patch

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

Great, thanks for getting this done! :)

Comment 16

[Georg Fritzsche [:gfritzsche]]

I took this through manual testing locally - the affected code does not have automated test coverage, so a try run is not needed.

Comment 17

[simplyblue24]

Thank you very for mentoring me :D

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/118c51d2942a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/118c51d2942a

Comment 20

[Francesco Lodolo [:flod]]

Not sure I understand this string: "Raw ping data ..."

Code should *always* use "…" instead of "..." for consistency with the rest of the product, also not sure why the blank space before the ellipsis. Did it just slip through the cracks?

Given it's a matter of consistency in en-US, you can fix this without using a new string ID.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Missed this without the needinfo here.

(In reply to Francesco Lodolo [:flod] from comment #20)
> Code should *always* use "…" instead of "..." for consistency with the rest
> of the product, also not sure why the blank space before the ellipsis. Did
> it just slip through the cracks?

I wasn't aware of this, i can file a follow-up - do we have any documentation on this?

Comment 22

[Francesco Lodolo [:flod]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #21)
> I wasn't aware of this, i can file a follow-up - do we have any
> documentation on this?

I'm not aware of any documentation about it.

There are a few instances of '...' around (~40), but either they're very old or just slipped through the cracks and were never fixed. On the other hand there are no strings with a space before ellipsis.