Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[New Tab Page] Add telemetry probes

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: raymondlee)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Add telemetry probes that track:

* whether the New Tab Page is enabled
* number of pinned tabs
* number of blocked tabs
* ...
(Reporter)

Updated

6 years ago
Blocks: 671038
(Reporter)

Updated

6 years ago
Blocks: 455553
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 616069 [details] [diff] [review]
v1

I am not sure what to set for HISTOGRAM() in TelemetryHistograms.h.  Please advise.
Attachment #616069 - Flags: review?(ttaubert)
(Reporter)

Comment 2

5 years ago
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."
Attachment #616069 - Flags: review?(ttaubert) → feedback+
(Reporter)

Comment 3

5 years ago
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?
(Assignee)

Comment 4

5 years ago
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.
Attachment #616069 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #617402 - Attachment is patch: true
Attachment #617402 - Flags: feedback?(ttaubert)

Comment 5

5 years ago
(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?)
(Reporter)

Comment 6

5 years ago
(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.
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
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.
Attachment #617402 - Flags: feedback?(ttaubert) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 618146 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results.
Attachment #617402 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
What are the try results? Does everything look good?
(Assignee)

Comment 11

5 years ago
(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
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46ef89f0bda
Target Milestone: --- → Firefox 15

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d46ef89f0bda
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.