1058951 - Add telemetry to measure the impact of changing the maximum favicon file size

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Bernardo Rittmeyer [:rittme]]

We should create some telemetry probes to help investigate the increasing of OPTIMIZED_FAVICON_DIMENSION for high-dpi devices, in bug 798223.

The suggested probes are:
- First 6 awesomebar entries
- History sidebar "by last visited”
- Time to execute a search in the library/sidebar

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 1 - Added telemetry probes to places unified autocomplete, history sidebar and history library search

This probes should allow us to have more information about possible performance impacts caused by future changes.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8483803 [details] [diff] [review]
rev 1 - Added telemetry probes to places unified autocomplete, history sidebar and history library search

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

::: toolkit/components/places/UnifiedComplete.js
@@ +748,5 @@
>    _onResultRow: function (row) {
>      TelemetryStopwatch.finish(TELEMETRY_1ST_RESULT);
> +    this._nbResults++;
> +    if (this._nbResults > 5)
> +      TelemetryStopwatch.finish(TELEMETRY_6_FIRST_RESULTS);

rather than counting manually, you should be able to do this in _addMatch, using this._result.matchCount, just before this line
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#848

::: toolkit/components/telemetry/Histograms.json
@@ +3198,5 @@
> +  "PLACES_HISTORY_TREE_QUERY_TIME_MS": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "low": 50,
> +    "high": "1000",

I think 1s is too strict for this giant synchronous view, I'd bump it a little bit, either 2 or 3s.

Comment 3

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - Added telemetry probes to places unified autocomplete, history sidebar and history library search

Thank you for the feedback, mak. 
I made the changes you suggested. 

I used 2s for the PLACES_HISTORY_TREE_QUERY_TIME_MS max. In my local tests (40K pages database) results are somewhere around 200ms. IMMO having values over 2s grouped at the last bucket would not be damaging and would make it easier to read smaller values.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8484258 [details] [diff] [review]
rev 2 - Added telemetry probes to places unified autocomplete, history sidebar and history library search

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

::: toolkit/components/telemetry/Histograms.json
@@ +3194,5 @@
> +    "n_buckets": 30,
> +    "extended_statistics_ok": true,
> +    "description": "PLACES: Time for the 6 first autocomplete results (ms)"
> +  },
> +  "PLACES_HISTORY_TREE_QUERY_TIME_MS": {

HISTORY_LASTVISITEDTREE_ or HISTORY_TREE_LASTVISITED_... maybe, if we want to add other views to the telemetry in future, we should distinguish them, history_tree is a bit too generic.

Comment 5

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - Added telemetry probes to places unified autocomplete, history sidebar and history library search. r=mak

Thank you for the review.

I changed the histogram name to HISTORY_LASTVISITED_TREE_QUERY_TIME_MS.

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9118a628ece

Comment 8

[Kamil Jozwiak [:kjozwiak]]

Bernardo,

Could you please add some more details or steps that QE can follow so this bug can be verified? I tried looking around under about:telemetry but couldn't find any histograms that start with "HISTORY_". Let me know if I'm looking at this the wrong way! Thanks :)

Comment 9

[Bernardo Rittmeyer [:rittme]]

Hi Kamil, sure I can.

- PLACES_AUTOCOMPLETE_6_FIRST_RESULTS_TIME_MS: any search at the awesomebar that shows 6 results or more should be accounted;

- HISTORY_LASTVISITED_TREE_QUERY_TIME_MS: searches at the history sidebar using the View "Last Visited" are measured;

- PLACES_HISTORY_LIBRARY_SEARCH_TIME_MS: searches at the history library (show all history dialog) are measured;

If you do these actions at least one time you will be able to see the histograms at the about:telemetry page.

I hope I could help. 
Thank you.

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following m-c build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-01-03-02-05-mozilla-central/

OS used for each test:

- Windows 8.1 x64 (VM) - PASSED
- Ubuntu 13.10 x64 (VM) - PASSED
- OSX 10.9.5 x64 - PASSED

PLACES_AUTOCOMPLETE_6_FIRST_RESULTS_TIME_MS:

- ensured that the histogram is being updated when autocomplete results are listed under the awesomebar
- ensured that the histogram is not being updated when there's no autocomplete results listed under the awesomebar
- ensured that the histogram is not being updated when autocomplete is triggered via about:home, about:newtab or the search box

HISTORY_LASTVISITED_TREE_QUERY_TIME_MS:

- ensured that searching via "By Last Visited" correctly updates the histogram
- ensured that searching via "By Date and Site", "Date", "Site" and "By Most Visited" doesn't update the histogram
- ensured that searching via the history "Library" didn't update the histogram

PLACES_HISTORY_LIBRARY_SEARCH_TIME_MS:

- ensured that searching via the history "Library" correctly updated the histogram (using "Today", "Yesterday", "Last 7 days", "September" and "August")

Thanks for the info Bernardo, definitely helped :) I just have a quick question. When closing fx, the three histograms are always cleared, is this intended? Or should the data be saved when someone closes and re-opens fx? Other than that, everything looks good!

Comment 11

[Bernardo Rittmeyer [:rittme]]

Thank you for the verification Kamil.

AFAIK the histograms are always cleared when we close/open the browser.
The data is sent to our telemetry servers and is persisted there.

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Awesome, thanks Bernardo! Marking as verified.