666538 - Use Telemetry to collect Panorama usage/perf data

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

http://blog.mozilla.com/tglek/2011/06/22/developers-how-to-submit-telemetry-data/

Comment 1

[Raymond Lee [:raymondlee]]

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

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

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

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

See also bug 671041.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 7

[Dietrich Ayala (:dietrich)]

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

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

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

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

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

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

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

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

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

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

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

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

Comment 19

[(no longer active)]

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