Closed Bug 1361855 Opened 6 years ago Closed 6 years ago

Collect telemetry on the "typical" number of tabs a user has open

Categories

(Firefox :: Tabbed Browser, enhancement)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: billm, Assigned: lie.1296)

References

Details

Attachments

(6 files, 3 obsolete files)

Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1361855 - Added TAB_COUNT to Histograms.json;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1361855 - Add helper function BrowserUsageTelemetry:_recordTabCount();
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1361855 - Update tab count on page load and other URL changes;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals;
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
We currently collect the maximum number of tabs that a user has opened in a given browsing session. This is useful data, but I worry that some users have that number of tabs open only rarely. I'm more interested in typical usage.

Benjamin suggested that we could create a histogram that would record the number of tabs the user had open every time they load a page. That would give us a much better sense of whether they had a given number of tabs open for a long time or just for a little while.
If this is easy/performant to do, it would be very valuable. I suggest that the best time to do this would be at the point we increment total_uri_count here:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUsageTelemetry.jsm#188

It doesn't appear that BrowserUsageTelemetry keeps a cached running total of open tabs: it has a function getOpenTabsAndWinsCounts() which can calculate that, but that involves using the window manager to enumerate windows and getting tab counts for each one, which is probably not work we can afford to do on each pageload.

Dexter or Gijs, can you suggest a performant way to cache the data we want here (or take this bug if it's a trivial thing to do)?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(alessio.placitelli)
The telemetry module could watch browser window openings and closings, and then add listeners to the tabstrip to watch tab openings / closings, and keep its own count of tabs. Not sure to what degree that risks getting out of sync for things like tab drags between windows, you'd have to check carefully. The only other thing you'd need to do then is to stick that into a histogram for every pageload. Doing this isn't entirely trivial but would be reasonably performant, I guess, maybe?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> If this is easy/performant to do, it would be very valuable. I suggest that
> the best time to do this would be at the point we increment total_uri_count
> here:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/modules/
> BrowserUsageTelemetry.jsm#188
> 
> It doesn't appear that BrowserUsageTelemetry keeps a cached running total of
> open tabs: it has a function getOpenTabsAndWinsCounts() which can calculate
> that, but that involves using the window manager to enumerate windows and
> getting tab counts for each one, which is probably not work we can afford to
> do on each pageload.
> 
> Dexter or Gijs, can you suggest a performant way to cache the data we want
> here (or take this bug if it's a trivial thing to do)?

I guess that one proxy for that could also be the number of tabs/windows restored by the session store. These measurements are currently stored in opt-in histograms (FX_SESSION_RESTORE_NUMBER_OF_* - see bug 1303278 for making them opt-out).

Another option could be to keep a running average of the number of open tabs here [1] and when tabs get closed. We could then store this value in some histogram or as a scalar (probably preferred). 

Unfortunately, I can't commit time to this right now :(

[1] - https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/browser/modules/BrowserUsageTelemetry.jsm#609
Flags: needinfo?(alessio.placitelli)
The histogram as specced is the preferred recording method.
Comment on attachment 8874155 [details]
Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();

https://reviewboard.mozilla.org/r/144020/#review149558

The code is fine but there are various style issues and nits.

::: browser/modules/test/browser/browser_UsageTelemetry_content.js:48
(Diff revision 1)
>  
>  add_task(async function test_context_menu() {
>    // Let's reset the Telemetry data.
>    Services.telemetry.clearScalars();
>    Services.telemetry.clearEvents();
> -  let search_hist = getSearchCountsHistogram();
> +  let search_hist = getHistogram("SEARCH_COUNTS", 'keyed');

Please use double quotes for any new strings.

::: browser/modules/test/browser/browser_UsageTelemetry_urlbar.js:354
(Diff revision 1)
> +  let resultMethodHist = getHistogram("FX_URLBAR_SELECTED_RESULT_METHOD");
> +  let resultIndexByTypeHist = getHistogram("FX_URLBAR_SELECTED_RESULT_INDEX_BY_TYPE", 'keyed');

Nit: Why change the order of these two probes? It makes the blame a bit more confusing.

::: browser/modules/test/browser/head.js:84
(Diff revision 1)
>  
>  /**
> - * Clear and get the SEARCH_COUNTS histogram.
> - */
> -function getSearchCountsHistogram() {
> -  let search_hist = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
> + * Clear and get the named histogram
> + * @param {String} name
> + *        The name of the histogram
> + * @param {String} type

Nit: optional parameters should be documented with square brackets:

```
@param {String} [type = 'normal']
```

::: browser/modules/test/browser/head.js:88
(Diff revision 1)
> -function getSearchCountsHistogram() {
> -  let search_hist = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
> -  search_hist.clear();
> -  return search_hist;
> + *        The name of the histogram
> + * @param {String} type
> + *        (optional) pass 'keyed' to get keyed histogram, or 'normal' to 
> + *        get normal histogram, default to 'normal'.
> + */
> +function getHistogram(name, type = 'normal') {

While we're changing this I wonder if the name should call out the fact it also clear the histogram? e.g. getAndClearHistogram?


FWIW I personally think it would be clearer with two methods instead of a second optional argument: one for keyed and the other for non-keyed.

::: browser/modules/test/browser/head.js:88
(Diff revision 1)
> +function getHistogram(name, type = 'normal') {
> +  let histogram = type == 'keyed' ? Services.telemetry.getKeyedHistogramById(name) : Services.telemetry.getHistogramById(name);

Please use double-quotes for strings in mozilla-central.
Attachment #8874155 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8874155 [details]
Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();

https://reviewboard.mozilla.org/r/144020/#review149744

I don't need to review most of these. I'll give feedback on the data/mark a data review on the appropriate patch and let Matt deal with the code reviews.
Attachment #8874155 - Flags: review?(benjamin)
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review149746

Going to mark data-review- for now to fix up the histogram description.

Here's my opinion of what would be best:

* record a value every time the user opens a new window or tab
* do not record a value on any tab close
* at startup record a single value for the number of tabs - so by default it would record "1" but with a restored session it would record a single value for the number of restored tabs

There's some wiggle room here if session restore is hard to do properly or something. Happy to provide feedback if there are tradeoffs to make.

::: toolkit/components/telemetry/Histograms.json:6
(Diff revision 1)
>  {
> +  "TAB_COUNT": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": [],
> +    "bug_numbers": [1361855],
> +    "expires_in_version": "never",

Please mark this as "releaseChannelCollection": "opt-out" and "expires_in_version": "60".

::: toolkit/components/telemetry/Histograms.json:10
(Diff revision 1)
> +    "bug_numbers": [1361855],
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 1000,
> +    "n_buckets": 100,
> +    "description": "Number of tabs opened across all windows"

This description needs to document details about *when* the value is recorded. If I saw the patch summary correctly, you're recording this on tab open. Also on window open? Are you recording on any close? How will this record at startup, especially with a restored session with many tabs?
Attachment #8874156 - Flags: review?(benjamin) → review-
Comment on attachment 8874157 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabClose event;

https://reviewboard.mozilla.org/r/144024/#review149750

::: browser/modules/BrowserUsageTelemetry.jsm:632
(Diff revision 1)
> +  _onTabClose(tabCount = 0) {
> +    // Use the provided tab count if available. Otherwise, go on and compute it.
> +    // subtract getOpenTabsAndWinsCounts() by 1 as it counts the tab that is still closing.
> +    tabCount = tabCount || (getOpenTabsAndWinsCounts().tabCount - 1);
> +
> +    Services.telemetry.getHistogramById("TAB_COUNT").add(tabCount);

I don't think we actually want to record a value on tab close. So maybe this patch isn't necessary at all?
Attachment #8874157 - Flags: review?(benjamin)
Comment on attachment 8874158 [details]
Bug 1361855 - Record TAB_COUNT histogram when a browser window is closed;

https://reviewboard.mozilla.org/r/144026/#review149752

ditto, this patch may not be necessary
Attachment #8874158 - Flags: review?(benjamin)
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review154342

Sorry, I would recommend finding another reviewer as I don't have enough context for this and I'm busy with multiple other projects.
Attachment #8874156 - Flags: review?(MattN+bmo)
Comment on attachment 8874157 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabClose event;

https://reviewboard.mozilla.org/r/144024/#review154344
Attachment #8874157 - Flags: review?(MattN+bmo)
Comment on attachment 8874158 [details]
Bug 1361855 - Record TAB_COUNT histogram when a browser window is closed;

https://reviewboard.mozilla.org/r/144026/#review154346
Attachment #8874158 - Flags: review?(MattN+bmo)
See Also: → 1373728
Attachment #8874156 - Flags: review?(gijskruitbosch+bugs)
Attachment #8874157 - Flags: review?(gijskruitbosch+bugs)
Attachment #8874158 - Flags: review?(gijskruitbosch+bugs)
Lie, gijs has volunteered to take over the review from MattN. Please flag him for review on any future patches in this bug.
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review156098

::: browser/modules/BrowserUsageTelemetry.jsm:332
(Diff revision 1)
>      Services.telemetry.scalarSetMaximum(MAX_WINDOW_COUNT_SCALAR_NAME, counts.winCount);
>  
>      // Reset the URI counter.
>      URICountListener.reset();
> +
> +    Services.telemetry.getHistogramById("TAB_COUNT").add(counts.tabCount);

If we do this on every tab open and tab close, I don't see why we need to do it once more when the subsession splits.

::: browser/modules/test/browser/browser_UsageTelemetry.js:303
(Diff revision 1)
> +  // Add two new tabs in the same window.
> +  openedTabs.push(await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank"));
> +  openedTabs.push(await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank"));
> +  checkTabCountHistogram(tabCountHist.snapshot(), [0,1,1,1,1], "TAB_COUNT telemetry - opening more tabs");
> +
> +  // Add a new window and then some tabs in it. An empty new windows counts as a tab.

Well, an "empty" new window contains 1 tab, and this comment should say that more explicitly. Can't have a window with 0 tabs. :-)

::: browser/modules/test/browser/browser_UsageTelemetry.js:305
(Diff revision 1)
> +  openedTabs.push(await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank"));
> +  checkTabCountHistogram(tabCountHist.snapshot(), [0,1,1,1,1], "TAB_COUNT telemetry - opening more tabs");
> +
> +  // Add a new window and then some tabs in it. An empty new windows counts as a tab.
> +  let win = await BrowserTestUtils.openNewBrowserWindow();
> +  let openedTabs2 = [];

This is unused.

::: toolkit/components/telemetry/Histograms.json:9
(Diff revision 1)
> +    "alert_emails": [],
> +    "bug_numbers": [1361855],
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 1000,
> +    "n_buckets": 100,

I defer to bsmedberg on the specifics here, but... this gets us buckets of size 1 all the way to 29, as far as I can tell ( https://telemetry.mozilla.org/histogram-simulator/#low=1&high=1000&n_buckets=100&kind=exponential&generate=normal - the graph is a bit naff, but if you hover over the x-axis it tells you the bucket limits). I don't think we need that granularity, and would argue that after 10 tabs there's basically no additional value for a "typical" difference of 1.

This is even more so because the repeated collecting to the same histogram means that the average of the histogram will be even more accurate/narrow than the limits of the bucket. In fact, from a privacy perspective, given that we collect this for every tab open / close, we might have exact numbers anyway, because from the number of collections we'd be able to deduce how many tabs were opened/closed. I'm not sure that is OK, either.
Attachment #8874156 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #18)
> ::: toolkit/components/telemetry/Histograms.json:9
> (Diff revision 1)
> > +    "alert_emails": [],
> > +    "bug_numbers": [1361855],
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": 1000,
> > +    "n_buckets": 100,
> 
> I defer to bsmedberg on the specifics here, but... this gets us buckets of
> size 1 all the way to 29, as far as I can tell (
> https://telemetry.mozilla.org/histogram-simulator/
> #low=1&high=1000&n_buckets=100&kind=exponential&generate=normal - the graph
> is a bit naff, but if you hover over the x-axis it tells you the bucket
> limits). I don't think we need that granularity, and would argue that after
> 10 tabs there's basically no additional value for a "typical" difference of
> 1.
> 
> This is even more so because the repeated collecting to the same histogram
> means that the average of the histogram will be even more accurate/narrow
> than the limits of the bucket. In fact, from a privacy perspective, given
> that we collect this for every tab open / close, we might have exact numbers
> anyway, because from the number of collections we'd be able to deduce how
> many tabs were opened/closed. I'm not sure that is OK, either.

--> ni Benjamin.
Flags: needinfo?(benjamin)
Comment on attachment 8874158 [details]
Bug 1361855 - Record TAB_COUNT histogram when a browser window is closed;

https://reviewboard.mozilla.org/r/144026/#review156114

Based on bsmedberg's comments, clearing review for this and the tab closing patch for now.
Attachment #8874158 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874157 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabClose event;

https://reviewboard.mozilla.org/r/144024/#review156116
Attachment #8874157 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review156120

::: browser/modules/test/browser/browser_UsageTelemetry.js:274
(Diff revision 1)
> +  // workaround for missing tab count telemetry event due to extended telemetry
> +  // recording not being enabled at the start of the test
> +  let adjustedCounts = result.counts.slice();
> +  adjustedCounts[1] += 1;

Can you elaborate on this comment and why the count for 1 or 2 tabs (which I think is what it is?) needs to be incremented to match expectations?
That histogram distribution doesn't seem terrible to me, but I think we could also do with a smaller upper bound: e.g. high: 400 n_buckets 40 gives us pretty tight distribution in the n<20 range but still a lot of coverage. 100 buckets is still pretty efficient though.
Flags: needinfo?(benjamin)
Comment on attachment 8874155 [details]
Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();

https://reviewboard.mozilla.org/r/144020/#review149558

> Nit: Why change the order of these two probes? It makes the blame a bit more confusing.

I changed the order because the original ordering was inconsistent between tests. In particular, the order in `browser/modules/test/browser/browser_UsageTelemetry_urlbar.js:test_simpleQuery()` is different than the rest. I picked the order from `test_simpleQuery()` as it groups the regular and keyed histogram together (which looks slightly more aesthetically pleasing to me). Do you prefer that I change the order for `test_simpleQuery()` instead to fit the others? or is it ok to leave `test_simpleQuery()` inconsistent with the others?

> Nit: optional parameters should be documented with square brackets:
> 
> ```
> @param {String} [type = 'normal']
> ```

Code no longer takes parameter
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review156120

> Can you elaborate on this comment and why the count for 1 or 2 tabs (which I think is what it is?) needs to be incremented to match expectations?

Issue dropped as no longer relevant because the latest code no longer attempt to capture a count when the browser just loaded or during session restore. The adjustment has now been removed.
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review156098

> This is unused.

Dropped. It's used now.
Comment on attachment 8874155 [details]
Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();

https://reviewboard.mozilla.org/r/144020/#review157560
Attachment #8874155 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8881168 [details]
Bug 1361855 - Added TAB_COUNT to Histograms.json;

https://reviewboard.mozilla.org/r/152434/#review157562

bsmedberg's review is both necessary and sufficient for this part of the patch.
Attachment #8881168 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8881169 [details]
Bug 1361855 - Add helper function BrowserUsageTelemetry:_recordTabCount();

https://reviewboard.mozilla.org/r/152436/#review157564

I'm not really sure why you split this off from the TabOpen patch, but r=me with the below addressed.

::: browser/modules/BrowserUsageTelemetry.jsm:644
(Diff revision 2)
> +  _recordTabCount(tabCount = 0) {
> +    tabCount = tabCount || getOpenTabsAndWinsCounts().tabCount;

Unlike in python, in JS default arguments are lazily evaluated ( https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Default_parameters#Evaluated_at_call_time ) , so you can do:

```js
_recordTabCount(tabCount = getOpenTabsAndWinsCounts().tabCount) {
}
```

and it will reinvoke that method every time you call it without an explicit passed tabCount.

Note that this has slightly different behaviour than your current version, in that in the previous version, explicitly passing `0` would have invoked `getOpenTabsAndWinsCounts`, whereas here it doesn't. I believe not invoking it is right (so you can explicitly call `_recordTabCount(0)` and explicitly record 0), so I prefer the version I'm suggesting.
Attachment #8881169 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review156098

> Dropped. It's used now.

I'm reopening this, because it isn't actually used. It's *written to* by using `openedTabs2.push()` but it's never read. Nothing uses the array for anything, except adding tabs to it. Just get rid of it. I don't see it being used in the subsequent patches either, but if it was, the variable should be added by the subsequent patches. As it is, it seems to serve no purpose.
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review157570

r=me with the comment fixed and `openedTabs2` removed.
Attachment #8874156 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8881171 [details]
Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals;

https://reviewboard.mozilla.org/r/152440/#review157576

On a high level - why 5 minutes? That seems like a pretty high interval - I usually don't spend quite that long on most pages, it feels like. And if we're fixing to that number anyway, shouldn't we just record every 5 minutes, not only at some point *after* 5 minutes in which a tab open / pageload happened? That would be moderately simpler to write, potentially...

Note also that this isn't the typical way to do this, in that tab changes in those 5 minutes get dropped, and only after 5 minutes (but that could be after 30 minutes, or never if the browser closes) will we write again. Normally what we do is we collapse writes within a 5 minute window. I don't know if that makes more sense here, but I'd rather have a discussion in the bug about what the desired aim of this is and if we need to do this. comment 11 seems to suggest we should record after session restore but not otherwise (besides the tab/window open which is already implemented). I would suggest discussing with bsmedberg on this bug to work out what we need to do, and then implement that. If there was already discussion on IRC or email or elsewhere, please ping me and I can re-review.

::: browser/modules/BrowserUsageTelemetry.jsm:319
(Diff revision 3)
>  
>  let BrowserUsageTelemetry = {
>    init() {
> +    let MINUTE = 60 * 1000;
> +    this._lastRecordTabCount = 0;
> +    this._tabCountIntervalMillis = 5 * MINUTE;

This is a constant, so it should be defined as a constant (in ALL_CAPS) at the top of the file.

I don't see much point in defining a constant for a minute, I would just do something like:

```js
const TYPICAL_TAB_RECORDING_INTERVAL_MS = 5 * 60 * 1000; // 5 minutes, in ms
```

::: browser/modules/BrowserUsageTelemetry.jsm:654
(Diff revision 3)
>      };
>      win.addEventListener("load", onLoad);
>    },
>  
>    _recordTabCount(tabCount = 0) {
> +    let currentTime = new Date().getTime();

Nit: `Date.now()`
Attachment #8881171 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874157 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabClose event;

https://reviewboard.mozilla.org/r/144024/#review157578

I think bsmedberg said we didn't want to do this - did this part of the patch series get submitted by mistake? You can remove them from hg locally by using `hg prune` (see `hg prune --help` for usage). If there's something I'm missing, please let me know.
Attachment #8874157 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874158 [details]
Bug 1361855 - Record TAB_COUNT histogram when a browser window is closed;

https://reviewboard.mozilla.org/r/144026/#review157582

Same.
Attachment #8874158 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8881170 [details]
Bug 1361855 - Update tab count on page load and other URL changes;

https://reviewboard.mozilla.org/r/152438/#review157584

r=me in the sense that this does what it says on the tin, but like with the 5 minute stuff, not sure this is what we want. Hopefully Benjamin can clarify.
Attachment #8881170 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8874157 - Attachment is obsolete: true
Attachment #8874157 - Flags: review?(benjamin)
Attachment #8874158 - Attachment is obsolete: true
Attachment #8874158 - Flags: review?(benjamin)
Comment on attachment 8881171 [details]
Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals;

https://reviewboard.mozilla.org/r/152440/#review157576

I chose 5 minutes because it's a round number :)

I agree that that's a bit long, though I presume that when aggregated across many users it should not affect the statistical distribution of the tab count. I would be happy to change the interval to something else.

This is my summary of my email conversation with bsmedberg:

- bsmedberg suggested that recounting tabs on every page load might not be good for performance (note: I don't notice a visible performance degradation even when there are hundreds of tab; it may be something that would show up in performance tests/benchmarks, but I don't know how to check for this)
- bsmedberg suggested to measure every 10 page loads to avoid too frequent recount
- I suggested interval recording to avoid recount as well as to normalize the tab count across various usage patterns, e.g. those who frequently used new tabs vs people who reuse tabs, and between people who browses short and long pages.
- bsmedberg mentioned that he does not want to add a timer (setTimeout) because it is more fiddly to get timer working right while avoiding memory leaks and minimizing impact to battery life (to which I wholly agree) and because it will overcount people who left their browsers inactive overnight.
- I suggested the current implementation which should avoid full recount, avoid biases due to user usage patterns, and doesn't trigger when the browser is inactive. Avoiding timer had also made the tests much easier to write.
- I implemented this commit to demonstrate my suggestion.

If you want, I can forward you the full transcript (I don't have your email?)
Comment on attachment 8881171 [details]
Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals;

https://reviewboard.mozilla.org/r/152440/#review158444

OK, if this is what we want to do, the implementation looks sane to me. Thanks!
Attachment #8881171 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Lie Ryan from comment #61)
> If you want, I can forward you the full transcript (I don't have your email?)

I don't think it's necessary, but for future reference you can click user names (email addresses) on comments on bugzilla to email folks. Sometimes there are suffixes you are better off omitting (I use "+bugs", others use "+bmo"), but other than that this usually works. :-)
Comment on attachment 8874155 [details]
Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram();

https://reviewboard.mozilla.org/r/144020/#review158464
Attachment #8874155 - Flags: review?(benjamin)
Comment on attachment 8881168 [details]
Bug 1361855 - Added TAB_COUNT to Histograms.json;

https://reviewboard.mozilla.org/r/152434/#review158466

data-r=me
Attachment #8881168 - Flags: review?(benjamin) → review+
> - bsmedberg suggested that recounting tabs on every page load might not be
> good for performance

If we *could* record the tab count on each pageload, that would be great!

My concern is about actually doing the work (enumerating the browser windows, and then counting the # of tabs) each time: that sounds like a lot of work to do on each pageload.

If however you've cached the tab count, then recording it every time sounds perfectly reasonable.

What you have here is good enough from a data/analysis perspective. I'll let Gijs decide about and review the technical details.
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

https://reviewboard.mozilla.org/r/144022/#review158508
Attachment #8874156 - Flags: review?(benjamin)
Comment on attachment 8881169 [details]
Bug 1361855 - Add helper function BrowserUsageTelemetry:_recordTabCount();

https://reviewboard.mozilla.org/r/152436/#review158510
Attachment #8881169 - Flags: review?(benjamin)
Comment on attachment 8881170 [details]
Bug 1361855 - Update tab count on page load and other URL changes;

https://reviewboard.mozilla.org/r/152438/#review158512
Attachment #8881170 - Flags: review?(benjamin)
Comment on attachment 8881171 [details]
Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals;

https://reviewboard.mozilla.org/r/152440/#review158514
Attachment #8881171 - Flags: review?(benjamin)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #69)
> If however you've cached the tab count, then recording it every time sounds
> perfectly reasonable.

FWIW, we could potentially cache it, but we don't right now. It would require dealing correctly not only with tabopen/close but also window restores from session restore, dragged tabs between windows, etc. That seems like a riskier strategy, but it would be possible in principle. I don't think the perf impact of the counting when we do it every 5 minutes or so will be sufficient to require that, so I think we should go with the implemented approach (at least for now).
(In reply to :Gijs from comment #74)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #69)
> > If however you've cached the tab count, then recording it every time sounds
> > perfectly reasonable.
> 
> FWIW, we could potentially cache it, but we don't right now. It would
> require dealing correctly not only with tabopen/close but also window
> restores from session restore, dragged tabs between windows, etc. That seems
> like a riskier strategy, but it would be possible in principle. I don't
> think the perf impact of the counting when we do it every 5 minutes or so
> will be sufficient to require that, so I think we should go with the
> implemented approach (at least for now).

Ah, hm, though... we could potentially piggyback on session restore? That's what Chris is doing in bug 1373728. Something like:


let {windows} = SessionStore.getCurrentState().windows;
let tabCount = windows.map(win => win.isPrivate ? 0 : win.tabs.length)
                      .reduce((tabcount, accumulator) => tabcount + accumulator, 0);

and that would be fairly cheap.

Sorry for not thinking of this before now. :-(

Do you think you can update your code based on this? You might need to XPCOMUtils.defineLazyModuleGetter() SessionStore in order to use it in BrowserUsageTelemetry.
Flags: needinfo?(lie.1296)
Comment on attachment 8883907 [details]
Bug 1361855 - Add helper function getTabCount() in BrowserUsageTelemetry.jsm;

https://reviewboard.mozilla.org/r/154894/#review159992

::: browser/modules/BrowserUsageTelemetry.jsm:106
(Diff revision 1)
> +  let tabCount = windows.map(win => win.isPrivate ? 0 : win.tabs.length)
> +                        .reduce((tabcount, accumulator) => tabcount + accumulator, 0);
> +  return tabCount;

Nit: you don't need a temp here, just return the result of the .map.reduce().
Attachment #8883907 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8881168 [details]
Bug 1361855 - Added TAB_COUNT to Histograms.json;

https://reviewboard.mozilla.org/r/152434/#review160002

Just Benjamin's review suffices here.
Attachment #8881168 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874156 [details]
Bug 1361855 - Record TAB_COUNT histogram during TabOpen event;

Clearing a bunch of review requests which I don't think I'm reviewing.
Attachment #8874156 - Flags: review?(benjamin)
Attachment #8881169 - Flags: review?(benjamin)
Attachment #8881170 - Flags: review?(benjamin)
Attachment #8881171 - Flags: review?(benjamin)
Attachment #8883907 - Flags: review?(benjamin)
Attachment #8881168 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8881168 [details]
Bug 1361855 - Added TAB_COUNT to Histograms.json;

Given that this is literally only the histogram definition, bsmedberg's review here is sufficient. According to mozreview, this already has his r+. Not sure why bugzilla isn't aware of it.
I think this is effectively ready to land? Lie, is that right?
As far as I am concerned, yes, I am satisfied with the code as of now.
Flags: needinfo?(lie.1296)
(In reply to Lie Ryan from comment #94)
> As far as I am concerned, yes, I am satisfied with the code as of now.

OK. trypush to check this works and the new tests pass on infra: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f8ce8213a24
(In reply to :Gijs (no reviews, mostly out until Jul 17) from comment #95)
> (In reply to Lie Ryan from comment #94)
> > As far as I am concerned, yes, I am satisfied with the code as of now.
> 
> OK. trypush to check this works and the new tests pass on infra:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f8ce8213a24

eslint found some code style issues in the test, can you update your patches for those?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f8ce8213a24dafb69eb6bf0d87c9fb57792a91b&selectedJob=113561876
Flags: needinfo?(lie.1296)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df92cb52eb4
Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram(); r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d02c0af528
Added TAB_COUNT to Histograms.json; r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/4397e17fc5fc
Add helper function getTabCount() in BrowserUsageTelemetry.jsm; r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfc9384ed64
Add helper function BrowserUsageTelemetry:_recordTabCount(); r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6877c0b84e
Record TAB_COUNT histogram during TabOpen event; r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/06dbf8fc7da3
Update tab count on page load and other URL changes; r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9ac96978ad
Filter TAB_COUNT histogram to record only on 5 minute intervals; r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/ede5862f0950
fix eslint issue, rs=eslint,me,firebot,etc
Backed out for frequently failing browser_UsageTelemetry.js | TAB_COUNT telemetry:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cde0022cc6e66802188fe1e24510df5e60598678
https://hg.mozilla.org/integration/mozilla-inbound/rev/d13067985d255619ed26d4e94a991b78c0a08072
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc6dfe5af8d014d516dab1bf964ceacfae78457
https://hg.mozilla.org/integration/mozilla-inbound/rev/4473acefd1b7116397f39f4533ae0a567b7bd072
https://hg.mozilla.org/integration/mozilla-inbound/rev/3325a300f82f2d661d01e4eb66dd18f6c1afaceb
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b3cca1c77052558bff6da996d8d7b5ee0bf034
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6588b951845b42fd8dc5ec6b5458b4fe0c40736
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e71f8196d5640e7a7d581c824b8383f14542a7

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=114884732&repo=mozilla-inbound

[task 2017-07-17T16:12:40.485985Z] 16:12:40     INFO - TEST-PASS | browser/modules/test/browser/browser_UsageTelemetry.js | TAB_COUNT telemetry - page load, recount event ignored - [0,0,1,2,1,1,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 deepEqual [0,0,1,2,1,1,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 - 
[task 2017-07-17T16:12:40.490190Z] 16:12:40     INFO - TEST-PASS | browser/modules/test/browser/browser_UsageTelemetry.js | TAB_COUNT telemetry - _lastRecordTabCount unchanged - 
[task 2017-07-17T16:12:40.495530Z] 16:12:40     INFO - Buffered messages finished
[task 2017-07-17T16:12:40.508839Z] 16:12:40     INFO - TEST-UNEXPECTED-FAIL | browser/modules/test/browser/browser_UsageTelemetry.js | TAB_COUNT telemetry - page load, recount event included - [0,0,1,2,1,1,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 deepEqual [0,0,1,2,1,1,0,2,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 - JS frame :: chrome://mochitests/content/browser/browser/modules/test/browser/browser_UsageTelemetry.js :: checkTabCountHistogram :: line 276
[task 2017-07-17T16:12:40.512972Z] 16:12:40     INFO - Stack trace:
[task 2017-07-17T16:12:40.514754Z] 16:12:40     INFO - chrome://mochitests/content/browser/browser/modules/test/browser/browser_UsageTelemetry.js:checkTabCountHistogram:276
[task 2017-07-17T16:12:40.516879Z] 16:12:40     INFO - chrome://mochitests/content/browser/browser/modules/test/browser/browser_UsageTelemetry.js:test_tabsHistogram:344
[task 2017-07-17T16:12:40.529122Z] 16:12:40     INFO - TEST-PASS | browser/modules/test/browser/browser_UsageTelemetry.js | TAB_COUNT telemetry - _lastRecordTabCount updated -
I'm kinda stumped on this one. It seems like the getTabCount() that uses SessionStore does not always match the tabCount from getOpenTabsAndWinsCounts() or what's actually shown on the UI, simply adding pauses on the test seems to cause SessionStore's tab count to exclude some tabs some of the time for reasons I don't understand.

If we want to use the tabCount from SessionStore, we would probably have to either remove most of the test or figure out how to make SessionStore's behave more consistently.
Flags: needinfo?(lie.1296)
(In reply to Lie Ryan from comment #102)
> I'm kinda stumped on this one. It seems like the getTabCount() that uses
> SessionStore does not always match the tabCount from
> getOpenTabsAndWinsCounts() or what's actually shown on the UI, simply adding
> pauses on the test seems to cause SessionStore's tab count to exclude some
> tabs some of the time for reasons I don't understand.
> 
> If we want to use the tabCount from SessionStore, we would probably have to
> either remove most of the test or figure out how to make SessionStore's
> behave more consistently.

I think effectively, session store gets called off the back of the same TabOpen event this code is listening for, but then doesn't necessarily update immediately but rather on a timeout. See:

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/browser/components/sessionstore/SessionStore.jsm#989-990

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/browser/components/sessionstore/SessionStore.jsm#1844-1846

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/browser/components/sessionstore/SessionStore.jsm#4004-4009

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/browser/components/sessionstore/SessionSaver.jsm#163-172

Does that sound like it could be an explanation for the current state of things?

While we could try to wait for the collection to have happened again, I don't think there are necessarily guarantees that those writes actually happen (e.g. if the tab gets removed again before the write timer runs and the resulting state is the same as the previous state, I kinda hope session store avoids re-writing the session file).

Perhaps we'll have to revert to the previous version of getTabCount(), given that we've kept the 5 minute backoff period I think that would still be fine, and we could potentially file a followup to sort out using session store notifications directly (rather than TabOpen events) to trigger saves, which would be lazy-er and less likely to cause perf issues anyway. How does that approach sound?
Assignee: nobody → lie.1296
Status: NEW → ASSIGNED
Flags: needinfo?(lie.1296)
Attachment #8883907 - Attachment is obsolete: true
That could be part of the reason, though it doesn't fully explain how in my machine having no artificial delay actually let SessionStore track against live count better (the test consistently passes on my machine when there is no artificial delay). If the issue was caused by delayed update, I would have expected that adding artificial delays should not have worsened SessionStore's tracking of tracking against the live count.

Given what you said that SessionStore hooks on TabOpen, I would agree that when using SessionStore, it would probably be best to hook on events generated by SessionStore instead of TabOpen and friends. Do you know if there is already a suitable SessionStore event that can be hooked into? Are there any concerns about mixing TAB_COUNT histogram data from two different data collection methods after the histogram was   released?

I've reverted and pushed the code to using the live count for now.
Flags: needinfo?(lie.1296)
Sorry, this got a bit lost in my to-do pile...

(In reply to Lie Ryan from comment #108)
> That could be part of the reason, though it doesn't fully explain how in my
> machine having no artificial delay actually let SessionStore track against
> live count better (the test consistently passes on my machine when there is
> no artificial delay). If the issue was caused by delayed update, I would
> have expected that adding artificial delays should not have worsened
> SessionStore's tracking of tracking against the live count.

Fair... have you tried tracing the updates in a run with these delayed updates to figure out why it *is* breaking?

> Given what you said that SessionStore hooks on TabOpen, I would agree that
> when using SessionStore, it would probably be best to hook on events
> generated by SessionStore instead of TabOpen and friends. Do you know if
> there is already a suitable SessionStore event that can be hooked into?

I'm not sure. Maybe Mike de Boer knows. Mike, does session restore fire an event after it updates its internal session object once a tab has been added?

> Are
> there any concerns about mixing TAB_COUNT histogram data from two different
> data collection methods after the histogram was   released?

I would not be too concerned as we're adding the 5 minute break inbetween recordings, so it seems that smaller timing differences (and it does seem these differences would be very small) would be negligible by comparison.

> I've reverted and pushed the code to using the live count for now.

OK. Is this ready to be pushed again in that case? Sorry again for losing track!
Flags: needinfo?(mdeboer)
Flags: needinfo?(lie.1296)
Yes, please push this again.

> Fair... have you tried tracing the updates in a run with these delayed updates to figure out why it *is* breaking? 

I did try adding console.log(tab count from sess manager and real count) statements in various places in the test and BrowserUsageTelemetry.jsm, and different delays in different places and delay amounts in the test. As far as I can tell, the counts are always consistent within a given delay configurations, but I was unable to discern a pattern that satisfactorily explains why different delays produces the tab count that they do.
Flags: needinfo?(lie.1296)
(In reply to :Gijs from comment #109)
> > Given what you said that SessionStore hooks on TabOpen, I would agree that
> > when using SessionStore, it would probably be best to hook on events
> > generated by SessionStore instead of TabOpen and friends. Do you know if
> > there is already a suitable SessionStore event that can be hooked into?
> 
> I'm not sure. Maybe Mike de Boer knows. Mike, does session restore fire an
> event after it updates its internal session object once a tab has been added?

It listens to the usual 'TabOpen' method, which triggers a - delayed - flush to disk of the current browser state, which'll include the newly opened tab.
Sessionstore doesn't track open windows or tabs, because that would needlessly duplicate state management data from tabbrowser and much of the data to save is not available to the parent process directly. It only tracks the closed ones. When a closed object is added to the internal object, the 'sessionstore-closed-objects-changed' notification is broadcasted.
Flags: needinfo?(mdeboer)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e9f8a718bc90 -d be21172b5b5c: rebasing 411891:e9f8a718bc90 "Bug 1361855 - Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram(); r=Gijs,MattN"
merging browser/modules/test/browser/browser_UsageTelemetry_content.js
merging browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js
merging browser/modules/test/browser/browser_UsageTelemetry_searchbar.js
merging browser/modules/test/browser/browser_UsageTelemetry_urlbar.js
merging browser/modules/test/browser/head.js
rebasing 411892:32d9c5627442 "Bug 1361855 - Added TAB_COUNT to Histograms.json; r=bsmedberg"
merging toolkit/components/telemetry/Histograms.json
rebasing 411893:0a43a889a343 "Bug 1361855 - Add helper function BrowserUsageTelemetry:_recordTabCount(); r=Gijs"
merging browser/modules/BrowserUsageTelemetry.jsm
rebasing 411894:2bb2689bf31f "Bug 1361855 - Record TAB_COUNT histogram during TabOpen event; r=Gijs"
merging browser/modules/BrowserUsageTelemetry.jsm
merging browser/modules/test/browser/browser_UsageTelemetry.js
rebasing 411895:a192b4268925 "Bug 1361855 - Update tab count on page load and other URL changes; r=Gijs"
merging browser/modules/BrowserUsageTelemetry.jsm
merging browser/modules/test/browser/browser_UsageTelemetry.js
rebasing 411896:b9be94009686 "Bug 1361855 - Filter TAB_COUNT histogram to record only on 5 minute intervals; r=Gijs" (tip)
merging browser/modules/BrowserUsageTelemetry.jsm
merging browser/modules/test/browser/browser_UsageTelemetry.js
warning: conflicts while merging browser/modules/BrowserUsageTelemetry.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like this needs rebasing by now. Sorry again. :-(
Flags: needinfo?(lie.1296)
Comment on attachment 8881168 [details]
Bug 1361855 - Added TAB_COUNT to Histograms.json;

https://reviewboard.mozilla.org/r/152434/#review160692

Given that this is literally only the data
Attachment #8881168 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c24de5ff5ebf
Refactor browser/modules/test/browser/head.js:getHistogram() and getKeyedHistogram(); r=Gijs,MattN
https://hg.mozilla.org/integration/autoland/rev/a2f8106b8fa3
Added TAB_COUNT to Histograms.json; r=bsmedberg,Gijs
https://hg.mozilla.org/integration/autoland/rev/05fab8979316
Add helper function BrowserUsageTelemetry:_recordTabCount(); r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3f15e95fa104
Record TAB_COUNT histogram during TabOpen event; r=Gijs
https://hg.mozilla.org/integration/autoland/rev/cf39a68f78b4
Update tab count on page load and other URL changes; r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f85d2c98fe2d
Filter TAB_COUNT histogram to record only on 5 minute intervals; r=Gijs
Flags: needinfo?(lie.1296)
Depends on: 1425967
Depends on: 1488945
You need to log in before you can comment on or make changes to this bug.