Closed
Bug 1171382
Opened 9 years ago
Closed 9 years ago
Don't show empty keyed histograms in about:telemetry
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: gfritzsche, Assigned: robertthyberg, Mentored)
Details
(Whiteboard: [lang=js] [good first bug])
Attachments
(1 file, 4 obsolete files)
1.39 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → hitmanarky
Status: NEW → ASSIGNED
Whiteboard: [lang=js] [good first bug]
Reporter | ||
Comment 1•9 years ago
|
||
(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
Reporter | ||
Updated•9 years ago
|
Assignee: hitmanarky → nobody
Status: ASSIGNED → NEW
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → robertthyberg
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)
Reporter | ||
Comment 6•9 years ago
|
||
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)
Attachment #8651969 -
Attachment is obsolete: true
Attachment #8652539 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 8•9 years ago
|
||
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)
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)
Reporter | ||
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
forgot to hg qrefresh before i submitted
Attachment #8653028 -
Attachment is obsolete: true
Attachment #8653028 -
Flags: review?(gfritzsche)
Attachment #8653031 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 13•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d7c26735c331
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d7c26735c331
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•