721020 - [New Tab Page] Add telemetry probes

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v1

I am not sure what to set for HISTOGRAM() in TelemetryHistograms.h.  Please advise.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

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]]

Attachment: 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]]

(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] (inactive)]

(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] (inactive)]

(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] (inactive)]

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]]

Attachment: Patch for checkin

Pushed to try and waiting for results.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

What are the try results? Does everything look good?

Comment 11

[Raymond Lee [:raymondlee]]

(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 12

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d46ef89f0bda

Comment 13

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/d46ef89f0bda