Last Comment Bug 721020 - [New Tab Page] Add telemetry probes
: [New Tab Page] Add telemetry probes
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 671038 455553
  Show dependency treegraph
 
Reported: 2012-01-25 07:24 PST by Tim Taubert [:ttaubert]
Modified: 2012-04-29 13:55 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.67 KB, patch)
2012-04-18 03:17 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v2 (3.53 KB, patch)
2012-04-23 00:05 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Review
Patch for checkin (3.77 KB, patch)
2012-04-24 20:01 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-01-25 07:24:08 PST
Add telemetry probes that track:

* whether the New Tab Page is enabled
* number of pinned tabs
* number of blocked tabs
* ...
Comment 1 Raymond Lee [:raymondlee] 2012-04-18 03:17:14 PDT
Created attachment 616069 [details] [diff] [review]
v1

I am not sure what to set for HISTOGRAM() in TelemetryHistograms.h.  Please advise.
Comment 2 Tim Taubert [:ttaubert] 2012-04-20 15:14:29 PDT
Comment on attachment 616069 [details] [diff] [review]
v1

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

This looks really good. We need to correct the histograms but the rest is great.

::: browser/modules/NewTabUtils.jsm
@@ +588,5 @@
>                                           Ci.nsISupportsWeakReference])
>  };
>  
>  /**
> + * Singleton that uses to collect telemetry data.

Nit: Singleton used to collect telemetry data.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +397,5 @@
>  HISTOGRAM_BOOLEAN(MULTIPART_XHR_RESPONSE, "XMLHttpRequest response was of type multipart/x-mixed-replace.")
>  
>  /**
> + * New Tab Page telemetry.
> + */

Let's call those NEWTAB_PAGE_* to be a tad more specific. about:telemetry shows the histogram IDs so the label don't need a common prefix.

@@ +398,5 @@
>  
>  /**
> + * New Tab Page telemetry.
> + */
> +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")

The label should say: "New tab page is enabled."

@@ +399,5 @@
>  /**
> + * New Tab Page telemetry.
> + */
> +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")
> +HISTOGRAM(NEWTAB_PINNED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Pinned Tab Count")

The ID should rather be NEWTAB_PAGE_PINNED_SITES_COUNT. There can't be more than 9 pinned tabs so we can reduce '200' to '9'. The minimum is '0'. The label should say: "Number of pinned sites on the new tab page."

@@ +400,5 @@
> + * New Tab Page telemetry.
> + */
> +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")
> +HISTOGRAM(NEWTAB_PINNED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Pinned Tab Count")
> +HISTOGRAM(NEWTAB_BLOCKED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Blocked Tab Count")

The ID should be NEWTAB_PAGE_BLOCKED_SITES_COUNT. The minimum value for this should be '0'. Let's keep the maximum at '100'. The label should say: "Number of sites blocked from the new tab page."
Comment 3 Tim Taubert [:ttaubert] 2012-04-20 15:16:16 PDT
Chris, we currently plan to track whether the newtab page is enabled, the number of pinned sites and the number of blocked sites. Can you think of any other metric that we should include?
Comment 4 Raymond Lee [:raymondlee] 2012-04-23 00:05:35 PDT
Created attachment 617402 [details] [diff] [review]
v2

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Comment on attachment 616069 [details] [diff] [review]
> v1
> 
> Review of attachment 616069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good. We need to correct the histograms but the rest is
> great.
> 
> ::: browser/modules/NewTabUtils.jsm
> @@ +588,5 @@
> >                                           Ci.nsISupportsWeakReference])
> >  };
> >  
> >  /**
> > + * Singleton that uses to collect telemetry data.
> 
> Nit: Singleton used to collect telemetry data.

Updated.

> 
> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +397,5 @@
> >  HISTOGRAM_BOOLEAN(MULTIPART_XHR_RESPONSE, "XMLHttpRequest response was of type multipart/x-mixed-replace.")
> >  
> >  /**
> > + * New Tab Page telemetry.
> > + */
> 
> Let's call those NEWTAB_PAGE_* to be a tad more specific. about:telemetry
> shows the histogram IDs so the label don't need a common prefix.
> 
> @@ +398,5 @@
> >  
> >  /**
> > + * New Tab Page telemetry.
> > + */
> > +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")
> 
> The label should say: "New tab page is enabled."

Updated.

> 
> @@ +399,5 @@
> >  /**
> > + * New Tab Page telemetry.
> > + */
> > +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")
> > +HISTOGRAM(NEWTAB_PINNED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Pinned Tab Count")
> 
> The ID should rather be NEWTAB_PAGE_PINNED_SITES_COUNT. There can't be more
> than 9 pinned tabs so we can reduce '200' to '9'. The minimum is '0'. The
> label should say: "Number of pinned sites on the new tab page."

Only boolean can have min value '0' (see below link) so I used '1' instead.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistograms.h#48

> 
> @@ +400,5 @@
> > + * New Tab Page telemetry.
> > + */
> > +HISTOGRAM(NEWTAB_ENABLED, 0, 1, 2, BOOLEAN, "NEW TAB: Enabled")
> > +HISTOGRAM(NEWTAB_PINNED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Pinned Tab Count")
> > +HISTOGRAM(NEWTAB_BLOCKED_TAB_COUNT, 1, 200, 10, EXPONENTIAL, "NEW TAB: Blocked Tab Count")
> 
> The ID should be NEWTAB_PAGE_BLOCKED_SITES_COUNT. The minimum value for this
> should be '0'. Let's keep the maximum at '100'. The label should say:
> "Number of sites blocked from the new tab page."

Same as above.
Comment 5 Chris Lee [:clee] 2012-04-23 05:51:11 PDT
(In reply to Tim Taubert [:ttaubert] from comment #3)
> Chris, we currently plan to track whether the newtab page is enabled, the
> number of pinned sites and the number of blocked sites. Can you think of any
> other metric that we should include?

Thanks for looping me in. 

* whether the New Tab Page is enabled
* number of pinned tabs
* number of blocked tabs (what's this defined as?)

In addition to these, it'd be valuable to track:

* how many times a user/all users clicked on 'restore'
* broken thumbnails (can we detect this?)
Comment 6 Tim Taubert [:ttaubert] 2012-04-24 11:29:10 PDT
(In reply to Chris Lee [:clee] from comment #5)
> * number of blocked tabs (what's this defined as?)

That's the number of tabs the user explicitly removed from the grid by clicking the 'x' in the upper right.

> * how many times a user/all users clicked on 'restore'

Ok, we'll include that with bug 716543.

> * broken thumbnails (can we detect this?)

We already have a MISS/HIT count in the thumbnail protocol itself which tracks exactly this.
Comment 7 Tim Taubert [:ttaubert] 2012-04-24 11:33:21 PDT
(In reply to Raymond Lee [:raymondlee] from comment #4)
> Only boolean can have min value '0' (see below link) so I used '1' instead.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryHistograms.h#48

Right, I forgot about this. Ok, then we'll leave it as is.
Comment 8 Tim Taubert [:ttaubert] 2012-04-24 11:43:54 PDT
Comment on attachment 617402 [details] [diff] [review]
v2

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

Looks good, r=me. No other metrics to include for now based on Chris' comment.
Comment 9 Raymond Lee [:raymondlee] 2012-04-24 20:01:20 PDT
Created attachment 618146 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results.
Comment 10 Tim Taubert [:ttaubert] 2012-04-26 06:36:58 PDT
What are the try results? Does everything look good?
Comment 11 Raymond Lee [:raymondlee] 2012-04-26 18:38:06 PDT
(In reply to Tim Taubert [:ttaubert] from comment #10)
> What are the try results? Does everything look good?

It looks fine.  There are some oranges but they are not related to this patch.
https://tbpl.mozilla.org/?tree=Try&rev=3b378a3b07ae
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-04-29 13:55:12 PDT
http://hg.mozilla.org/mozilla-central/rev/d46ef89f0bda

Note You need to log in before you can comment on or make changes to this bug.