Closed Bug 1046234 Opened 10 years ago Closed 9 years ago

Telemetry measures for tools users' systems

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Version:
30 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: canuckistani, Assigned: miker)

References

Details

Attachments

(1 file, 4 obsolete files)

extra-telemetry-1046234.patch
17.92 KB, patch
miker
: review+
Details | Diff | Splinter Review 
We should get a better idea of the systems people are using tools on. 

Initial list:

* screen resolution
* OS
* # tabs open ( peak & average )
* # pinned tabs
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
We now have:

* DEVTOOLS_SCREEN_RESOLUTION_ENUMERATED_PER_USER
  - Lower
  - 800x600
  - 1024x768
  - 1280x800
  - 1280x1024
  - 1366x768
  - 1440x900
  - 1920x1080
  - 2560×1600
  - 2880x1800
  - Higher

* DEVTOOLS_OS_ENUMERATED_PER_USER
    - Windows NT 5.1                            // Windows XP (32-bit)
    - Windows NT 5.2                            // Windows XP (64-bit)
    - Windows NT 6.0                            // Windows Vista
    - Windows NT 6.1                            // Windows 7
    - Windows NT 6.2                            // Windows 8
    - Windows NT 6.3                            // Windows 8.1
    - PPC Mac OS X                              // Mac OS X (PPC build)
    - Intel Mac OS X x.y                        // Mac OS X (i386/x64 build)
    - Output of uname -s plus "i686 on x86_64"  // Linux 64-bit (32-bit build)
    - Output of uname -sm                       // Linux
    - Other

* DEVTOOLS_TABS_OPEN_PEAK_EXPONENTIAL

* DEVTOOLS_TABS_OPEN_AVERAGE_EXPONENTIAL

* DEVTOOLS_TABS_PINNED_EXPONENTIAL
What's the status of this?
Flags: needinfo?(mratcliffe)
I just need to add the probes from comment 10 and some tests and we are done.
Flags: needinfo?(mratcliffe)
I have moved the probes from comment 10 into bug 1106262.
Attached patch extra-telemetry-1046234.patch (obsolete) — — Splinter Review 

        Attachment #8530539 -
        Flags: review?(pbrosset)

    
    

    
  
We cannot test the following telemetry stats because we cannot get that information together until the browser is closing and this is too late for the testing harness:
- DEVTOOLS_TABS_OPEN_PEAK_EXPONENTIAL
- DEVTOOLS_TABS_OPEN_AVERAGE_EXPONENTIAL
- DEVTOOLS_TABS_PINNED_PEAK_EXPONENTIAL
- DEVTOOLS_TABS_PINNED_AVERAGE_EXPONENTIAL

What I can say is that the correct data is sent to telemetry when I add telemetry logging.

    
    

    
  
I haven't reviewed the code yet, I will get to it next week.
I have a question though: none of the new probes are devtools-related. OS, screen resolution, tabs are all pretty generic things. It doesn't feel great to put the code for this in gDevTools and toolbox.

Jeff: Do we really only want to record this information for people that open the toolbox?
Mike: Shouldn't the probes have more general names and the code be in a more general browser file?
Flags: needinfo?(mratcliffe)
Flags: needinfo?(jgriffiths)

    
    

    
  
Valid points, but I have no real opinion on how these things are named or what file they go in. I do think we should track them for all pre-release users.
Flags: needinfo?(jgriffiths)

    
    

    
  
I thought the point of knowing how many tabs a devtools user has open, screen resolution etc. was to see if there was a correlation between devtools users and users that have e.g. lots of tabs open.

Jeff, are you sure you want me to change this to get that data for all pre release users as that would make it impossible to link e.g. tab usage with devtools usage?
Flags: needinfo?(mratcliffe) → needinfo?(jgriffiths)

    
    

    
  
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> I thought the point of knowing how many tabs a devtools user has open,
> screen resolution etc. was to see if there was a correlation between
> devtools users and users that have e.g. lots of tabs open.
> 
> Jeff, are you sure you want me to change this to get that data for all pre
> release users as that would make it impossible to link e.g. tab usage with
> devtools usage?

Argh, you're right. Let's scope this to just devtools users, as originally discussed. Sorry for the churn, I did not remember the details of the conversation in London.
Flags: needinfo?(jgriffiths)

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> I haven't reviewed the code yet, I will get to it next week.
> I have a question though: none of the new probes are devtools-related. OS,
> screen resolution, tabs are all pretty generic things. It doesn't feel great
> to put the code for this in gDevTools and toolbox.
> 

They are generic things tested in the context of DevTools users... the code is where it needs to be.

> Jeff: Do we really only want to record this information for people that open
> the toolbox?

Jeff has answered this "Yes."

> Mike: Shouldn't the probes have more general names and the code be in a more
> general browser file?

No, the point of knowing how many tabs a devtools user has open, screen resolution etc. was to see if there was a correlation between devtools users and users that have e.g. lots of tabs open, big screens etc.

    
    

    
  
Comment on attachment 8530539 [details] [diff] [review]
extra-telemetry-1046234.patch

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

Thanks for clarifying this for me.
Code-wise, I only have 2 minor remarks. Other than that, this looks good.

::: browser/devtools/framework/gDevTools.jsm
@@ +499,5 @@
>      let deveditionThemeEnabled = Services.prefs.getBoolPref(THEME_BROWSER_PREFNAME);
>      this._telemetry.log(THEME_HISTOGRAM_BROWSER, deveditionThemeEnabled);
> +
> +    let mean = function(arr) {
> +      let total = arr.reduce(function(a, b) { return a + b; });

nit: shorter this way: arr.reduce((a, b) => a + b);

::: toolkit/components/telemetry/Histograms.json
@@ +6896,5 @@
> +  "DEVTOOLS_OS_ENUMERATED_PER_USER": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "description": "Screen resolution of DevTools user (0:Windows XP, 1:Windows Vista, 2:Windows 7, 3:Windows 8, 4:Windows 8.1, 5:OSX, 6:Linux 10:other)"

This should say "OS" instead of "Screen resolution"

        Attachment #8530539 -
        Flags: review?(pbrosset) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        extra-telemetry-1046234.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Rebased and addressed review comments.

We now have:
DEVTOOLS_OS_ENUMERATED_PER_USER
DEVTOOLS_OS_IS_64_BITS_PER_USER
DEVTOOLS_SCREEN_RESOLUTION_ENUMERATED_PER_USER
DEVTOOLS_TABS_OPEN_PEAK_EXPONENTIAL
DEVTOOLS_TABS_OPEN_AVERAGE_EXPONENTIAL
DEVTOOLS_TABS_PINNED_PEAK_EXPONENTIAL
DEVTOOLS_TABS_PINNED_AVERAGE_EXPONENTIAL

Try:
https://tbpl.mozilla.org/?tree=Try&rev=5f08fe65d48f

        Attachment #8530539 -
        Attachment is obsolete: true

        Attachment #8565899 -
        Flags: review+

    
    

    
  
Sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=73f1285748cf since one of this 2 patches caused :

https://treeherder.mozilla.org/logviewer.html#?job_id=2018723&repo=fx-team

(that failure is also in the try run)
Flags: needinfo?(mratcliffe)

    
    

    
  
We really want this so we are going to disable the tests as the error is in a completely different location that should be caused by the test changes. I am done with pulling my hair out trying to get the tests to work.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=101af11e31d8
Flags: needinfo?(mratcliffe)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        extra-telemetry-1046234.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I suppose a toolbox peer should review the changes in nsAppRunner.cpp and nsIXULRuntime.idl even if they are mind-numbingly simple.

Jaws: Do you have time?

        Attachment #8565899 -
        Attachment is obsolete: true

        Attachment #8572677 -
        Flags: review?(jaws)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        extra-telemetry-1046234.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Remembered to update UUID.

Taking timezones into account I think Gijs is best to review this.

Gijs: It is a mind numbingly simple patch and you only need to review the two files I mention in comment 26.

        Attachment #8572677 -
        Attachment is obsolete: true

        Attachment #8572677 -
        Flags: review?(jaws)

        Attachment #8573153 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8573153 [details] [diff] [review]
extra-telemetry-1046234.patch

You want an XPCOM peer for those changes.

        Attachment #8573153 -
        Flags: review?(gijskruitbosch+bugs) → review?(nfroyd)

    
    

    
  
Comment on attachment 8573153 [details] [diff] [review]
extra-telemetry-1046234.patch

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

I guess you've gotten r+ from the devtools side of things.  I have no complaints about the nsAppRunner.cpp/nsIXULRuntime.idl changes, but I do have some comments about the telemetry bits...

::: browser/devtools/framework/gDevTools.jsm
@@ +476,5 @@
>  
> +  _pingTelemetry: function() {
> +    let mean = function(arr) {
> +      let total = arr.reduce((a, b) => a + b);
> +      return Math.ceil(total / arr.length);

Is it absolutely guaranteed that arr.length is never zero?

@@ +483,5 @@
> +    let tabStats = gDevToolsBrowser._tabStats;
> +    this._telemetry.log(TABS_OPEN_PEAK_HISTOGRAM, tabStats.peakOpen);
> +    this._telemetry.log(TABS_OPEN_AVG_HISTOGRAM, mean(tabStats.histOpen));
> +    this._telemetry.log(TABS_PINNED_PEAK_HISTOGRAM, tabStats.peakPinned);
> +    this._telemetry.log(TABS_PINNED_AVG_HISTOGRAM, mean(tabStats.histPinned));

Note that if the session is open long enough to register e.g. two measurements, the *PEAK* histograms will show, say, measurements of 60 tabs and 40 tabs for a single session in a single histogram.  Is that what you want?

@@ +548,5 @@
> +  _tabStats: {
> +    peakOpen: 0,
> +    peakPinned: 0,
> +    histOpen: [],
> +    histPinned: []

I think it'd be good to have a way to measure this without having these arrays grow without bound.

::: browser/devtools/framework/toolbox.js
@@ +1616,5 @@
> +    if (dims === "1366x768")  return 5;
> +    if (dims === "1440x900")  return 6;
> +    if (dims === "1920x1080") return 7;
> +    if (dims === "2560×1600") return 8;
> +    if (dims === "2560×1600") return 9;

Duplicate dimensions here too.

@@ +1632,5 @@
> +    if (oscpu.contains("NT 6.3")) return 4;
> +    if (oscpu.contains("OS X"))   return 5;
> +    if (oscpu.contains("Linux"))  return 6;
> +
> +    return 10; // Other OS.

Note that this doesn't (I think) match up with the documentation for the histogram.

::: toolkit/components/telemetry/Histograms.json
@@ +6471,5 @@
> +  "DEVTOOLS_OS_ENUMERATED_PER_USER": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 8,
> +    "description": "OS of DevTools user (0:Windows XP, 1:Windows Vista, 2:Windows 7, 3:Windows 8, 4:Windows 8.1, 5:OSX, 6:Linux 7:other)"

Please double (at least) the number of values here, so if/when we want to add Windows 10 or differentiate between OSX versions, or whatever, that we don't have to throw away this histogram and use another one.

Though, come to think of it, why can't we just get this information from FHR?

@@ +6477,5 @@
> +  "DEVTOOLS_OS_IS_64_BITS_PER_USER": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 3,
> +    "description": "OS bit size of DevTools user (0:32bit, 1:64bit, 2:128bit)"

Also seems like something to get from FHR.

@@ +6483,5 @@
> +  "DEVTOOLS_SCREEN_RESOLUTION_ENUMERATED_PER_USER": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 13,
> +    "description": "Screen resolution of DevTools user (0:lower, 1:800x600, 2:1024x768, 3:1280x800, 4:1280x1024, 5:1366x768, 6:1440x900, 7:1920x1080, 8:2560×1600, 9:2560×1600, 10:2880x1800, 11:other, 12:higher)"

Looks like you listed 2560x1600 twice...maybe the first one is supposed to be 2560x1440?

Also seems like something to get from FHR.

@@ +6489,5 @@
> +  "DEVTOOLS_TABS_OPEN_PEAK_EXPONENTIAL": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": "101",
> +    "n_buckets": "100",

Having an exponential histogram with an upper bound this high and roughly the same number of buckets as the number of values is no better than a linear histogram.

@@ +6496,5 @@
> +  "DEVTOOLS_TABS_OPEN_AVERAGE_EXPONENTIAL": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": "101",
> +    "n_buckets": "100",

Likewise.

@@ +6497,5 @@
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": "101",
> +    "n_buckets": "100",
> +    "description": "The mean number of open tabs in all windows for a session for devtools users."

How is this mean being calculated?

@@ +6511,5 @@
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": "101",
> +    "n_buckets": "100",
> +    "description": "The mean number of pinned tabs (app tabs) in all windows for a session for devtools users."

Same question for this mean.

        Attachment #8573153 -
        Flags: review?(nfroyd) → feedback+

    
    

    
  
Commenting here because jwalker and I were chatting about it.

With regards to tracking Windows 64 bit operating systems, the variants we care about right now are Windows 7 and 8 - so detecting those would be great, and i care a lot less about things like XP, Vista, Windows 2000, etc. My suspicion is that the bulk of our Windows users are on recent versions, and if it turns out I'm wrong we can follow up later with better detection of other variants.

Mike - does this help?
Flags: needinfo?(mratcliffe)

    
    

    
  
(In reply to Jeff Griffiths (:canuckistani) from comment #30)
> Commenting here because jwalker and I were chatting about it.
> 
> With regards to tracking Windows 64 bit operating systems, the variants we
> care about right now are Windows 7 and 8 - so detecting those would be
> great, and i care a lot less about things like XP, Vista, Windows 2000, etc.
> My suspicion is that the bulk of our Windows users are on recent versions,
> and if it turns out I'm wrong we can follow up later with better detection
> of other variants.
> 
> Mike - does this help?

Yes, we give the OS and let you know if it is 64-bit so that is covered.
Flags: needinfo?(mratcliffe)

    
    

    
  
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #29)
> Comment on attachment 8573153 [details] [diff] [review]
> extra-telemetry-1046234.patch
> 
> Review of attachment 8573153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess you've gotten r+ from the devtools side of things.  I have no
> complaints about the nsAppRunner.cpp/nsIXULRuntime.idl changes, but I do
> have some comments about the telemetry bits...
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +476,5 @@
> >  
> > +  _pingTelemetry: function() {
> > +    let mean = function(arr) {
> > +      let total = arr.reduce((a, b) => a + b);
> > +      return Math.ceil(total / arr.length);
> 
> Is it absolutely guaranteed that arr.length is never zero?
> 

It shouldn't happen but let's return 0 in that case.

> @@ +483,5 @@
> > +    let tabStats = gDevToolsBrowser._tabStats;
> > +    this._telemetry.log(TABS_OPEN_PEAK_HISTOGRAM, tabStats.peakOpen);
> > +    this._telemetry.log(TABS_OPEN_AVG_HISTOGRAM, mean(tabStats.histOpen));
> > +    this._telemetry.log(TABS_PINNED_PEAK_HISTOGRAM, tabStats.peakPinned);
> > +    this._telemetry.log(TABS_PINNED_AVG_HISTOGRAM, mean(tabStats.histPinned));
> 
> Note that if the session is open long enough to register e.g. two
> measurements, the *PEAK* histograms will show, say, measurements of 60 tabs
> and 40 tabs for a single session in a single histogram.  Is that what you
> want?
> 

If they are the peak values then sure, that is what we want to show.

> @@ +548,5 @@
> > +  _tabStats: {
> > +    peakOpen: 0,
> > +    peakPinned: 0,
> > +    histOpen: [],
> > +    histPinned: []
> 
> I think it'd be good to have a way to measure this without having these
> arrays grow without bound.
> 

We need to gather stats as we go, store the numbers somewhere and then calculate the mean when the browser closes. They need to be stored somewhere and an array is the perfect candidate.

> ::: browser/devtools/framework/toolbox.js
> @@ +1616,5 @@
> > +    if (dims === "1366x768")  return 5;
> > +    if (dims === "1440x900")  return 6;
> > +    if (dims === "1920x1080") return 7;
> > +    if (dims === "2560×1600") return 8;
> > +    if (dims === "2560×1600") return 9;
> 
> Duplicate dimensions here too.
> 
> @@ +1632,5 @@
> > +    if (oscpu.contains("NT 6.3")) return 4;
> > +    if (oscpu.contains("OS X"))   return 5;
> > +    if (oscpu.contains("Linux"))  return 6;
> > +
> > +    return 10; // Other OS.
> 
> Note that this doesn't (I think) match up with the documentation for the
> histogram.
> 

Fixed.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +6471,5 @@
> > +  "DEVTOOLS_OS_ENUMERATED_PER_USER": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 8,
> > +    "description": "OS of DevTools user (0:Windows XP, 1:Windows Vista, 2:Windows 7, 3:Windows 8, 4:Windows 8.1, 5:OSX, 6:Linux 7:other)"
> 
> Please double (at least) the number of values here, so if/when we want to
> add Windows 10 or differentiate between OSX versions, or whatever, that we
> don't have to throw away this histogram and use another one.
> 

Good idea, done.

> Though, come to think of it, why can't we just get this information from FHR?
> 

That would be so much easier but the point of these stats is to test these values for devtools users only. e.g. What is a devtools users average screen size?

> @@ +6477,5 @@
> > +  "DEVTOOLS_OS_IS_64_BITS_PER_USER": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 3,
> > +    "description": "OS bit size of DevTools user (0:32bit, 1:64bit, 2:128bit)"
> 
> Also seems like something to get from FHR.
> 

Not when we want the stats just for devtools users.

> @@ +6483,5 @@
> > +  "DEVTOOLS_SCREEN_RESOLUTION_ENUMERATED_PER_USER": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 13,
> > +    "description": "Screen resolution of DevTools user (0:lower, 1:800x600, 2:1024x768, 3:1280x800, 4:1280x1024, 5:1366x768, 6:1440x900, 7:1920x1080, 8:2560×1600, 9:2560×1600, 10:2880x1800, 11:other, 12:higher)"
> 
> Looks like you listed 2560x1600 twice...maybe the first one is supposed to
> be 2560x1440?
> 

Yes, you are correct.

> Also seems like something to get from FHR.
> 

Not when we want the stats just for devtools users.

> @@ +6489,5 @@
> > +  "DEVTOOLS_TABS_OPEN_PEAK_EXPONENTIAL": {
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": "101",
> > +    "n_buckets": "100",
> 
> Having an exponential histogram with an upper bound this high and roughly
> the same number of buckets as the number of values is no better than a
> linear histogram.
> 

okay, changed to linear.

> @@ +6496,5 @@
> > +  "DEVTOOLS_TABS_OPEN_AVERAGE_EXPONENTIAL": {
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": "101",
> > +    "n_buckets": "100",
> 
> Likewise.
> 

Changed to linear.

> @@ +6497,5 @@
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": "101",
> > +    "n_buckets": "100",
> > +    "description": "The mean number of open tabs in all windows for a session for devtools users."
> 
> How is this mean being calculated?
> 

Each time a tab is opened, closed, pinned or unpinned we iterate over all browser windows counting the number of open (tabStats.histOpen) and pinned tabs (tabStats.histPinned). This way we can calculate peak and average values when the browser closes.

```
let mean = function(arr) {
  if (arr.length === 0) {
    return 0;
  }

  let total = arr.reduce((a, b) => a + b);
  return Math.ceil(total / arr.length);
};

this._telemetry.log(TABS_OPEN_AVG_HISTOGRAM, mean(tabStats.histOpen));
```

> @@ +6511,5 @@
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": "101",
> > +    "n_buckets": "100",
> > +    "description": "The mean number of pinned tabs (app tabs) in all windows for a session for devtools users."
> 
> Same question for this mean.

```
this._telemetry.log(TABS_PINNED_AVG_HISTOGRAM, mean(tabStats.histPinned));
```

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ec213bf54bcf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox39:
          --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Depends on: 1144363
No longer depends on: 1144363

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: