Closed Bug 1171382 Opened 7 years ago Closed 7 years ago

Don't show empty keyed histograms in about:telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: gfritzsche, Assigned: robertthyberg, Mentored)

Details

(Whiteboard: [lang=js] [good first bug])

Attachments

(1 file, 4 obsolete files)

The relevant code is here:
https://hg.mozilla.org/mozilla-central/annotate/a8edd0679b57/toolkit/content/aboutTelemetry.js#l1596

We can just not show that section when all the entries in keyedHistograms are empty objects.
Assignee: nobody → hitmanarky
Status: NEW → ASSIGNED
Whiteboard: [lang=js] [good first bug]
(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> The relevant code is here:
> https://hg.mozilla.org/mozilla-central/annotate/a8edd0679b57/toolkit/content/
> aboutTelemetry.js#l1596
> 
> We can just not show that section when all the entries in keyedHistograms
> are empty objects.

To be a bit more precise, there are two issues here:
* not showing empty keyed histograms in the keyedHistogram section
* not setting hasData for the keyedHistogram section when all keyed histograms are empty
Assignee: hitmanarky → nobody
Status: ASSIGNED → NEW
Robert, would you maybe be interested in taking this?
This is about making the "Keyed Histogram" section in the about:telemetry page more usable.
Flags: needinfo?(robertthyberg)
Yeah but I am unsure of how to go about this. From my understanding, each element is a histogram and we needto check if that histogram is not empty, then if all of them are empty dont set has data. why not set hasdata as false? How would I check if a histogram is empty?
Flags: needinfo?(robertthyberg)
So, the ping data can look e.g. like this:

{
  // ...
  payload: {
    keyedHistograms: {
      "DEVTOOLS_WEBIDE_CONNECTED_RUNTIME_ID": {}, 
      "BLOCKED_ON_PLUGINASYNCSURROGATE_WAITFORINIT_MS": {}, 
      "PROCESS_CRASH_SUBMIT_SUCCESS": {}, 
      "SEARCH_COUNTS": {
        "bing.searchbar": {
          "bucket_count": 3, 
          "sum_squares_hi": 0, 
          "range": [
            1, 
            2
          ], 
          "values": {
            "1": 0, 
            "0": 1
          }, 
          "sum": 1, 
          "histogram_type": 4, 
          "sum_squares_lo": 1
        }, 
        "google.searchbar": {
          "bucket_count": 3, 
          "sum_squares_hi": 0, 
          "range": [
            1, 
            2
          ], 
          "values": {
            "1": 0, 
            "0": 1
          }, 
          "sum": 1, 
          "histogram_type": 4, 
          "sum_squares_lo": 1
        },
        ...

Here e.g. DEVTOOLS_WEBIDE_CONNECTED_RUNTIME_ID is an empty keyed histogram, while SEARCH_COUNTS is not empty.

There is one check already in the linked code: hasData = Object.keys(keyedHistograms).length > 0;

We can do two things here:
1) If a keyed histogram is empty (i.e. Object.keys().length==0), don't render it in the loop here
2) If all keyed histograms were empty, set hasData to false
Assignee: nobody → robertthyberg
Attached patch dont_show_empty_histograms (obsolete) — Splinter Review
Before the patch i would see empty keyed histograms and after I applied it, it wouldnt show empties. I didnt know how to generate data so I didnt test whether they would show up if there was data. Also should we eventually have it grayed out like the other no data portions? It looks unclear to click on something and have nothing load.
Attachment #8651969 - Flags: review?(gfritzsche)
Comment on attachment 8651969 [details] [diff] [review]
dont_show_empty_histograms

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

You can e.g. test non-empty keyed histograms with the SEARCH_COUNTS histogram (http://mzl.la/1V6LvpI).
Every time you do a search, this should add a count for the respective search engine and the context, e.g.: http://imgur.com/27JszJx

Right, we would want this section greyed out if we have nothing to show.
I think the simplest approach here is to change things a little about how we set the visibility.
We use setHasData() for that - we could just change that usage here to something like:

let hasData = false;
for (let [id, keyed] of ...
  if (Object.keys(keyed).length > 0) {
    hasData = true;
    ... render ...
  }
}

setHasData("keyed-histograms-section", hasData);

::: toolkit/content/aboutTelemetry.js
@@ +1598,5 @@
>    setHasData("keyed-histograms-section", hasData);
>  
>    if (hasData) {
>      for (let [id, keyed] of Iterator(keyedHistograms)) {
> +      if (!Object.keys(keyed).length == 0) {

We probably want |.length > 0| or so.

@@ +1603,1 @@
>        KeyedHistogram.render(keyedDiv, id, keyed, {unpacked: true});

This line is missing indentation now.
Attachment #8651969 - Flags: review?(gfritzsche)
Attached patch dont_show_empty_histograms (obsolete) — Splinter Review
Attachment #8651969 - Attachment is obsolete: true
Attachment #8652539 - Flags: review?(gfritzsche)
Comment on attachment 8652539 [details] [diff] [review]
dont_show_empty_histograms

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

This is nearly done, thanks.
Only one real issue i commented on below.

::: toolkit/content/aboutTelemetry.js
@@ +1599,3 @@
>      for (let [id, keyed] of Iterator(keyedHistograms)) {
> +      if (Object.keys(keyed).length > 0) {
> +        hasData = true;       

Nit: Whitespace at the end of the line.

@@ +1604,2 @@
>      }
> +    setHasData("keyed-histograms-section", hasData);

This should be outside of the if-block (or we could setHasData(.., false) before it).
Otherwise this doesn't update properly if you:
* first load a ping which has some non-empty keyed histograms
* then load a ping with no keyedHistograms==null
... e.g. using the "archived ping data".

We would not hide the section in this case.

@@ +1606,2 @@
>    }
> + 

Nit: Whitespace at the end of the line.
Attachment #8652539 - Flags: review?(gfritzsche)
Attached patch dont_show_empty_histograms (obsolete) — Splinter Review
I ended up adding setHasData to false before the if statement and got rid of the extra whitespace
Attachment #8652539 - Attachment is obsolete: true
Attachment #8652877 - Flags: review?(gfritzsche)
Comment on attachment 8652877 [details] [diff] [review]
dont_show_empty_histograms

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

Thanks, this looks good.
Lets fix the two nits below and land it.

This doesn't have test coverage and doesn't need a try push.

::: toolkit/content/aboutTelemetry.js
@@ +1591,5 @@
>  
>    // Show keyed histogram data
>    let keyedDiv = document.getElementById("keyed-histograms");
>    removeAllChildNodes(keyedDiv);
> + 

Nit: some trailing whitespace on this line now.
Depending on your editor you may be able to set it up to highlight this or to automatically remove it on lines that were modified.

@@ -1604,2 @@
>    }
> -

Nit: don't remove the empty line.
Attachment #8652877 - Flags: review?(gfritzsche) → review+
Attached patch dont_show_empty_histograms (obsolete) — Splinter Review
yeah I got a plugin for vim that will show me the trailing whitespace now. Did I need to flag this for review again since its a new file or could i have just added it as an attachment with r=gfritzsche in the commit message
Attachment #8652877 - Attachment is obsolete: true
Attachment #8653028 - Flags: review?(gfritzsche)
forgot to hg qrefresh before i submitted
Attachment #8653028 - Attachment is obsolete: true
Attachment #8653028 - Flags: review?(gfritzsche)
Attachment #8653031 - Flags: review?(gfritzsche)
Comment on attachment 8653031 [details] [diff] [review]
dont_show_empty_histograms

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

Thanks.
If a reviewer already sets review+ and only lists some changes needed before landing, then you can just do the changes, upload the fixed patch and set review+ yourself ("carrying over r+") to show that it was already reviewed.
Attachment #8653031 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7c26735c331
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.