Last Comment Bug 666538 - Use Telemetry to collect Panorama usage/perf data
: Use Telemetry to collect Panorama usage/perf data
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- enhancement
: Firefox 15
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
Depends on:
Blocks: 671038
  Show dependency treegraph
 
Reported: 2011-06-23 04:04 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (4.98 KB, patch)
2012-02-01 07:50 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
v2 (6.74 KB, patch)
2012-04-24 01:28 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 (11.09 KB, patch)
2012-04-25 01:46 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v4 (6.23 KB, patch)
2012-04-25 21:05 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (6.14 KB, patch)
2012-04-26 20:43 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (6.45 KB, patch)
2012-04-29 18:53 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Comment 1 Raymond Lee [:raymondlee] 2011-07-26 20:08:15 PDT
We need to establish what data we want to collect.

E.g. 
1. How many times does a user enter/exit Panorama
2. How does a user enter/exit Panorama? By key combo, click on a menu item, etc
3. How long does a user stay in Panorama
4. How many groups does a user have?
5. How many groups are with title
6. How many average number of tabs in all group/how many highest/lowest number of tabs among those group
Comment 2 AndreiD[QA] 2011-07-27 02:05:07 PDT
(In reply to comment #0)
> http://blog.mozilla.com/tglek/2011/06/22/developers-how-to-submit-telemetry-
> data/

Should we add a dependency to this bug on bug 585196 - telemtry infrastructure, just for a better tracking purpose?
And/or adding this bug to the wiki: https://wiki.mozilla.org/Platform/Features/Telemetry
Comment 3 Tim Taubert [:ttaubert] 2011-07-28 09:24:22 PDT
(In reply to comment #2)
> Should we add a dependency to this bug on bug 585196 - telemtry
> infrastructure, just for a better tracking purpose?
> And/or adding this bug to the wiki:
> https://wiki.mozilla.org/Platform/Features/Telemetry

We should maybe add a feature page for it and list all data we want to be tracked.

(In reply to comment #1)
> 1. How many times does a user enter/exit Panorama
> 2. How does a user enter/exit Panorama? By key combo, click on a menu item,
> 3. How long does a user stay in Panorama
> 4. How many groups does a user have?
> 5. How many groups are with title
> 6. How many average number of tabs in all group/how many highest/lowest
> number of tabs among those group

Telemetry seems not ready yet to collect all kinds of data. At the moment it's able to record a number and we can see this number develop over a period of time. So (3) seems pretty easy to do but not (1). Also (4) + (5) + (6) are feasible but no idea how to tackle (2).
Comment 4 Marco Bonardo [::mak] 2011-07-28 11:22:49 PDT
for (2) you may associate a fake enum like
let activation = {
  menu: 0,
  keys: 1,
  ...
}
and store that value

(1) is hard unless you store temporary data somewhere, you may maybe calculate time elapsed by the last time user entered it, and store that, so you can generate an avg of the time between uses. Not sure how this may help you improve the component though.  Collecting datapoints is easy, figure out what's useful to collect is not.
Comment 5 Tim Taubert [:ttaubert] 2011-11-30 04:50:58 PST
See also bug 671041.
Comment 6 Tim Taubert [:ttaubert] 2012-02-01 07:50:34 PST
Created attachment 593447 [details] [diff] [review]
patch v1
Comment 7 Dietrich Ayala (:dietrich) 2012-02-02 03:02:40 PST
Comment on attachment 593447 [details] [diff] [review]
patch v1

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

needs to be in the firefox ifdef:

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

also, can use new Date() instead of Date.now()
Comment 8 Dietrich Ayala (:dietrich) 2012-02-13 20:05:14 PST
Comment on attachment 593447 [details] [diff] [review]
patch v1

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

::: browser/base/content/browser-tabview.js
@@ +218,5 @@
>        window.removeEventListener("tabviewframeinitialized", onInit, false);
>  
> +      // Record Panorama initialization time with telemetry.
> +      Services.telemetry.getHistogramById("PANORAMA_INITIALIZATION_TIME_MS")
> +        .add(Date.now() - telemetryInitTime);

update to use TelemetryTimestamp.

::: browser/components/tabview/ui.js
@@ +276,5 @@
>  
>        if (!hasGroupItemsData)
>          this.reset();
> +      else if (!gPrivateBrowsing.privateBrowsingEnabled)
> +        this._addTelemetryValues();

does this occur every time panorama is loaded? we should probably only do this once per application load.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +324,5 @@
> + * Panorama telemetry.
> + */
> +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")

why record this?

@@ +325,5 @@
> + */
> +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")
> +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of stacked groups")

ditto

@@ +326,5 @@
> +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")
> +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of stacked groups")
> +HISTOGRAM(PANORAMA_AVG_TABS_PER_GROUP_COUNT, 1, 100, 15, EXPONENTIAL, "PANORAMA: Number of tabs per group")

hm, i think it'd be more helpful to have the median instead of an average.
Comment 9 Raymond Lee [:raymondlee] 2012-04-24 01:28:25 PDT
Created attachment 617819 [details] [diff] [review]
v2

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Comment on attachment 593447 [details] [diff] [review]
> patch v1
> 
> Review of attachment 593447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-tabview.js
> @@ +218,5 @@
> >        window.removeEventListener("tabviewframeinitialized", onInit, false);
> >  
> > +      // Record Panorama initialization time with telemetry.
> > +      Services.telemetry.getHistogramById("PANORAMA_INITIALIZATION_TIME_MS")
> > +        .add(Date.now() - telemetryInitTime);
> 
> update to use TelemetryTimestamp.

Use TelemetryTimestamps

> 
> ::: browser/components/tabview/ui.js
> @@ +276,5 @@
> >  
> >        if (!hasGroupItemsData)
> >          this.reset();
> > +      else if (!gPrivateBrowsing.privateBrowsingEnabled)
> > +        this._addTelemetryValues();
> 
> does this occur every time panorama is loaded? we should probably only do
> this once per application load.

Added a TabviewTelemetry.jsm so we add telemetry values just once per application load

> 
> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +324,5 @@
> > + * Panorama telemetry.
> > + */
> > +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> > +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> > +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")
> 
> why record this?
 Removed PANORAMA_NAMED_GROUPS_COUNT.

> 
> @@ +325,5 @@
> > + */
> > +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> > +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> > +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")
> > +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of stacked groups")
> 
> ditto
> 
> @@ +326,5 @@
> > +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "PANORAMA: Time (ms) it takes to initialize the view")
> > +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of groups")
> > +HISTOGRAM(PANORAMA_NAMED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of named groups")
> > +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "PANORAMA: Number of stacked groups")
> > +HISTOGRAM(PANORAMA_AVG_TABS_PER_GROUP_COUNT, 1, 100, 15, EXPONENTIAL, "PANORAMA: Number of tabs per group")
> 
> hm, i think it'd be more helpful to have the median instead of an average.

Updated to use median.
Comment 10 Tim Taubert [:ttaubert] 2012-04-24 12:01:46 PDT
Comment on attachment 617819 [details] [diff] [review]
v2

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

::: browser/base/content/browser-tabview.js
@@ +216,5 @@
>  
> +    let tmp = {};
> +    Cu.import("resource:///modules/TelemetryTimestamps.jsm", tmp);
> +    let TelemetryTimestamps = tmp.TelemetryTimestamps;
> +    TelemetryTimestamps.add("panoramaInitializationStarted");

We can't use TelemetryTimestamps because that tracks timestamps relative to the process startup only. Panorama is usually not in the startup path so we need to use TelemetryStopwatch for that.

::: browser/components/tabview/ui.js
@@ +249,5 @@
>  
>        if (!hasGroupItemsData)
>          this.reset();
> +      else if (!gPrivateBrowsing.privateBrowsingEnabled)
> +        this._addTelemetryValues();

We should use the "gather-telemetry" notification to track group counts like you did in bug 721020.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +438,5 @@
> + * Panorama telemetry.
> + */
> +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "Number of groups in Panorama")
> +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "Number of stacked groups in Panorama")
> +HISTOGRAM(PANORAMA_MEDIAN_TABS_IN_GROUPS_COUNT, 1, 100, 15, EXPONENTIAL, "Median of tabs in groups in Panorama")

We need to add a histogram that tracks the time it takes to initialize Panorama.
Comment 11 Raymond Lee [:raymondlee] 2012-04-25 01:46:01 PDT
Created attachment 618187 [details] [diff] [review]
v3

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 617819 [details] [diff] [review]
> v2
> 
> Review of attachment 617819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-tabview.js
> @@ +216,5 @@
> >  
> > +    let tmp = {};
> > +    Cu.import("resource:///modules/TelemetryTimestamps.jsm", tmp);
> > +    let TelemetryTimestamps = tmp.TelemetryTimestamps;
> > +    TelemetryTimestamps.add("panoramaInitializationStarted");
> 
> We can't use TelemetryTimestamps because that tracks timestamps relative to
> the process startup only. Panorama is usually not in the startup path so we
> need to use TelemetryStopwatch for that.

Switched to use TelemetryStopwatch

> 
> ::: browser/components/tabview/ui.js
> @@ +249,5 @@
> >  
> >        if (!hasGroupItemsData)
> >          this.reset();
> > +      else if (!gPrivateBrowsing.privateBrowsingEnabled)
> > +        this._addTelemetryValues();
> 
> We should use the "gather-telemetry" notification to track group counts like
> you did in bug 721020.
> 

I have added an observer in ui.js. I have removed the TabviewTelemetry.jsm because we still want to collect telemetry data for all windows with Panorama initialized so the jsm might not be used for this.  Also, re-factored the code which for private-browsing topics and moved them into observe() method.  

> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +438,5 @@
> > + * Panorama telemetry.
> > + */
> > +HISTOGRAM(PANORAMA_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "Number of groups in Panorama")
> > +HISTOGRAM(PANORAMA_STACKED_GROUPS_COUNT, 1, 25, 15, EXPONENTIAL, "Number of stacked groups in Panorama")
> > +HISTOGRAM(PANORAMA_MEDIAN_TABS_IN_GROUPS_COUNT, 1, 100, 15, EXPONENTIAL, "Median of tabs in groups in Panorama")
> 
> We need to add a histogram that tracks the time it takes to initialize
> Panorama.

Done.
Comment 12 Tim Taubert [:ttaubert] 2012-04-25 09:42:26 PDT
Comment on attachment 618187 [details] [diff] [review]
v3

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

I'd rather not change too much of the existing code. Telemetry is an easy addition and we should implement it like that. Please create a telemetry.js and include it. That code can initialize itself, add and remove observers and finally gather telemetry data. Another benefit of doing it that way is that we'll not clutter UI more than it already is.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +436,5 @@
>  
>  /**
> + * Panorama telemetry.
> + */
> +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "Time takes to initialize the Panorama view (ms)")

Better: "Time it takes to initialize Panorama (ms)"
Comment 13 Raymond Lee [:raymondlee] 2012-04-25 21:05:35 PDT
Created attachment 618546 [details] [diff] [review]
v4

(In reply to Tim Taubert [:ttaubert] from comment #12)
> Comment on attachment 618187 [details] [diff] [review]
> v3
> 
> Review of attachment 618187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd rather not change too much of the existing code. Telemetry is an easy
> addition and we should implement it like that. Please create a telemetry.js
> and include it. That code can initialize itself, add and remove observers
> and finally gather telemetry data. Another benefit of doing it that way is
> that we'll not clutter UI more than it already is.
> 

Done.

> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +436,5 @@
> >  
> >  /**
> > + * Panorama telemetry.
> > + */
> > +HISTOGRAM(PANORAMA_INITIALIZATION_TIME_MS, 1, 10000, 15, EXPONENTIAL, "Time takes to initialize the Panorama view (ms)")
> 
> Better: "Time it takes to initialize Panorama (ms)"

Updated
Comment 14 Tim Taubert [:ttaubert] 2012-04-26 06:57:34 PDT
Comment on attachment 618546 [details] [diff] [review]
v4

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

The structure is good but this patch doesn't really submit any telemetry values. We need to fix this and some other nits.

::: browser/components/tabview/telemetry.js
@@ +5,5 @@
> +/**
> + * Collects telemetry data for Tabview.
> + */
> +let Telemetry = {
> +  _TOPIC_GATHER_TELEMETRY: "gather-telemetry",

Nit: Let's not put an underscore in front of this. This looks rather strange for a "constant".

@@ +24,5 @@
> +
> +  /**
> +   * Adds telemetry values to gather usage statistics.
> +   */
> +  _addTelemetryValues: function Telemetry_addTelemetryValues() {

How about _collect() or _collectData() or _collectTelemetryData?

@@ +25,5 @@
> +  /**
> +   * Adds telemetry values to gather usage statistics.
> +   */
> +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> +    let namedGroupsCount = 0;

We don't need this anymore.

@@ +27,5 @@
> +   */
> +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> +    let namedGroupsCount = 0;
> +    let stackedGroupsCount = 0;
> +    let nonEmptyGroupsCount = 0;

We don't need this anymore, too.

@@ +28,5 @@
> +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> +    let namedGroupsCount = 0;
> +    let stackedGroupsCount = 0;
> +    let nonEmptyGroupsCount = 0;
> +    let tabItemsInGroups = [];

Nit: it's not tabItems but rather the groups' child counts. So maybe 'childCounts'?

@@ +40,5 @@
> +      }
> +    });
> +
> +    function add(aId, aValue) {
> +      Services.telemetry.getHistogramById("PANORAMA_" + aId, aValue);

Services.telemetry.getHistogramById("PANORAMA_" + aId).add(aValue);

@@ +42,5 @@
> +
> +    function add(aId, aValue) {
> +      Services.telemetry.getHistogramById("PANORAMA_" + aId, aValue);
> +    }
> +    function median(aArray) {

Nit: aArray is not a good name. How about 'aChildCounts'?
Comment 15 Raymond Lee [:raymondlee] 2012-04-26 20:43:32 PDT
Created attachment 618912 [details] [diff] [review]
v5

(In reply to Tim Taubert [:ttaubert] from comment #14)
> Comment on attachment 618546 [details] [diff] [review]
> v4
> 
> Review of attachment 618546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The structure is good but this patch doesn't really submit any telemetry
> values. We need to fix this and some other nits.
> 
> ::: browser/components/tabview/telemetry.js
> @@ +5,5 @@
> > +/**
> > + * Collects telemetry data for Tabview.
> > + */
> > +let Telemetry = {
> > +  _TOPIC_GATHER_TELEMETRY: "gather-telemetry",
> 
> Nit: Let's not put an underscore in front of this. This looks rather strange
> for a "constant".

Removed underscore

> 
> @@ +24,5 @@
> > +
> > +  /**
> > +   * Adds telemetry values to gather usage statistics.
> > +   */
> > +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> 
> How about _collect() or _collectData() or _collectTelemetryData?

Changed to _collect().

> 
> @@ +25,5 @@
> > +  /**
> > +   * Adds telemetry values to gather usage statistics.
> > +   */
> > +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> > +    let namedGroupsCount = 0;
> 
> We don't need this anymore.

Removed

> 
> @@ +27,5 @@
> > +   */
> > +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> > +    let namedGroupsCount = 0;
> > +    let stackedGroupsCount = 0;
> > +    let nonEmptyGroupsCount = 0;
> 
> We don't need this anymore, too.

Removed

> 
> @@ +28,5 @@
> > +  _addTelemetryValues: function Telemetry_addTelemetryValues() {
> > +    let namedGroupsCount = 0;
> > +    let stackedGroupsCount = 0;
> > +    let nonEmptyGroupsCount = 0;
> > +    let tabItemsInGroups = [];
> 
> Nit: it's not tabItems but rather the groups' child counts. So maybe
> 'childCounts'?

Changed.

> 
> @@ +40,5 @@
> > +      }
> > +    });
> > +
> > +    function add(aId, aValue) {
> > +      Services.telemetry.getHistogramById("PANORAMA_" + aId, aValue);
> 
> Services.telemetry.getHistogramById("PANORAMA_" + aId).add(aValue);
> 

Updated

> @@ +42,5 @@
> > +
> > +    function add(aId, aValue) {
> > +      Services.telemetry.getHistogramById("PANORAMA_" + aId, aValue);
> > +    }
> > +    function median(aArray) {
> 
> Nit: aArray is not a good name. How about 'aChildCounts'?

Changed.
Comment 16 Tim Taubert [:ttaubert] 2012-04-27 12:44:17 PDT
Comment on attachment 618912 [details] [diff] [review]
v5

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

Thanks, looks good to me! Please push it to try before marking it as checkin-needed.

::: browser/components/tabview/telemetry.js
@@ +37,5 @@
> +          stackedGroupsCount++;
> +      }
> +    });
> +
> +    function add(aId, aValue) {

Nit: Let's call this addTelemetryValue() so it's a bit more expressive.
Comment 17 Raymond Lee [:raymondlee] 2012-04-29 18:53:54 PDT
Created attachment 619466 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Comment on attachment 618912 [details] [diff] [review]
> v5
> 
> Review of attachment 618912 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks good to me! Please push it to try before marking it as
> checkin-needed.
> 
> ::: browser/components/tabview/telemetry.js
> @@ +37,5 @@
> > +          stackedGroupsCount++;
> > +      }
> > +    });
> > +
> > +    function add(aId, aValue) {
> 
> Nit: Let's call this addTelemetryValue() so it's a bit more expressive.

Updated.


Passed try
https://tbpl.mozilla.org/?tree=Try&rev=13456b3c3383
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-04-30 17:31:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b37045fba514

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