1271304 - Measure the tabs/window open events and max number of tab/window per subsession

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

This bug is about measuring the total number of opens (number of tab open events) per session fragment.

The implementation should sum the number of tab open events over all windows.

Context: https://docs.google.com/spreadsheets/d/1G33YEBL2-hSaF0-HrHPi0HVwANoTpYHsnevmMd1eZ1U/edit#gid=0

Comment 1

[Alessio Placitelli [:Dexter]]

Since we're trying to measure the user "intent", I don't think we should count the "tab opens" triggered by the SessionStore (i.e. restoring the windows/tabs when Firefox starts). The caveat there is that we could have a mismatch between the maximum number of tab opens and the total number of tab opens: in the end they are measuring two things, so there should be no problem.

Comment 2

[brendan c]

Good catch Alessio.

Rebecca, I think this could have product implications-- we'll want to know about people restoring tabs to save state between sessions as well, no? Should we request another probe to measure session restore tabs?

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to brendan c from comment #2)
> Should we request another probe to measure session restore tabs?

Indeed, if we want to know about that we should measure that with a separate probe.
Looking more closely, we already have probes for that:
FX_SESSION_RESTORE_NUMBER_OF_TABS_RESTORED
FX_SESSION_RESTORE_NUMBER_OF_WINDOWS_RESTORED

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Experimental tab measurements

This is an *experimental* patch (NOT intended for landing!) that implements the "tab" related measurements.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Experimental test coverage

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Experimental tab measurements

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Experimental test coverage

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Experimental measurements

This adds experimental total URI count.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Extends the test coverage to cover the total URI count.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Experimental measurements

This patch adds the support for counting the visited unique domains. This also prevents URI/Domain counting when in private mode, as discussed in bug 1271310 comment 6.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Updated to add test coverage for the domain count, private mode, AJAX.

This is still lacking popup test coverage, which is clearly marked as a TODO.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8754486 [details] [diff] [review]
part 1 - Experimental measurements

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

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +75,5 @@
> +  },
> +
> +  onLocationChange: function (webProgress, request, uri, flags) {
> +    // Don't count this URI if we're refreshing or it's an error page.
> +    if ((flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) ||

I thought LOCATION_CHANGE_SAME_DOCUMENT was used for fragment navigations as well which I thought you want to count? Am I wrong?

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> Comment on attachment 8754486 [details] [diff] [review]
> part 1 - Experimental measurements
> 
> Review of attachment 8754486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/BrowserUsageTelemetry.jsm
> @@ +75,5 @@
> > +  },
> > +
> > +  onLocationChange: function (webProgress, request, uri, flags) {
> > +    // Don't count this URI if we're refreshing or it's an error page.
> > +    if ((flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) ||
> 
> I thought LOCATION_CHANGE_SAME_DOCUMENT was used for fragment navigations as
> well which I thought you want to count? Am I wrong?

Good catch, thanks for pointing this out. You're right, this is also used for fragments.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Experimental measurements

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Extended the test coverage to cover the rest of the requirements.

Comment 17

[Alessio Placitelli [:Dexter]]

Temporarily dropping this and bug 1271319 to add scalar type support (which would be used for the browser engagement measurements).

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8765489 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

Gijs, I'm flagging you for review as you have a little context already (we had a Vidyo meeting a while back with :gfritzsche). To recap, we're implementing a few new probes that would allow us to better understand how users use or engage with Firefox [1].

Instead of using histograms, we're going to test-drive the new Scalar implementation (bug 1276190, bug 1276195 and bug 1276196). The documentation for the Scalars is available at [2].

These patches implement the following probes:

** OPEN TABS EVENT **

- Number of "tab open" events across all windows.
- No session store

** OPEN WINDOWS EVENT **

- Number of "window open" events.
- No session store
- No fullscreen windows
- Possibly no popups

** MAX TABS/WINDOWS **

- The maximum number of tabs/windows within a session fragment

** UNIQUE DOMAINS **

- Only top-level domains
- No private browsing
- maximum of 100 domains per fragment

** VISITED URIs **

- Count page fragments
- Count reloads (if possible)
- Don't count background AJAX
- Besically try our best to only count stuff intentionally triggered by the user.

There are a few open points:

- Is there a way to check if the new opened window is a popup?
- Switching tabs triggers an onLocationChange event. In order to filter these out (I'm only interested in web navigation started by the user), is the check that I'm performing in the code correct, i.e. |request === null && !flags|?

[1] - https://docs.google.com/document/d/1UuztyM-fdQBR88QjOiMIbqlHhLGMVfvPF_xbyhKWmOQ/edit?ts=56c378e3#heading=h.ygjz8s3u0xtp
[2] - https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/scalars.html

Comment 21

[:Gijs (he/him)]

Comment on attachment 8765489 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

Why do we need a new module? Can't this just live in BrowserUITelemetry? That already has tab/window counts, which we should probably get rid of if we're implementing new ones to avoid confusion about which data is better/right.

I reached my comment limit for 1 review, so I haven't quite looked at everything, but there's enough fodder here for improvement so I'm going to leave it for now.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +15,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
> +                                  "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +var Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);

Services.telemetry ?

@@ +37,5 @@
> +
> +  return {
> +    tabCount: tabCount,
> +    winCount: winCount
> +  };

Nit: {winCount, tabCount};

@@ +40,5 @@
> +    winCount: winCount
> +  };
> +};
> +
> +var URICountListener = {

nit: let

@@ +69,5 @@
> +    }
> +
> +    // We will be notified with |onLocationChange| when switching tab. In that case,
> +    // the request will be null and no flags will be passed.
> +    if (request === null && !flags) {

Nit: if (!request && !flags)

instead, but we really don't need this, so see below.

@@ +74,5 @@
> +      this._log.trace("onLocationChange - Skipping tab switch.");
> +      return;
> +    }
> +
> +    // |uri.spec| can throw, so account for that.

Uh, can it? If uri is null, presumably? I don't see why else it would throw. uri.host can throw, and makeURI can throw, but if you already have an nsIURI I don't know of any protocol that can subsequently throw if you ask it for its .spec.

@@ +79,5 @@
> +    let uriSpec;
> +    try {
> +      uriSpec = uri.spec;
> +    } catch (ex if (typeof ex.result !== 'undefined' &&
> +                    ex.result === Cr.NS_ERROR_MALFORMED_URI)) {

This is non-standard JS and we're trying to get rid of it, so please don't add any. In fact, I'm a bit surprised - did you run eslint? I don't think it can pass with this stuff in the file.

@@ +85,5 @@
> +      return;
> +    }
> +
> +    // Don't count "about:*" pages.
> +    if (/^about:\S+/.test(uriSpec)) {

Shudder. uri.schemeIs("about") or uri.scheme == "about". Don't regex-test specs ever.

@@ +86,5 @@
> +    }
> +
> +    // Don't count "about:*" pages.
> +    if (/^about:\S+/.test(uriSpec)) {
> +      this._log.trace("onLocationChange - Skipping about:* URIs.");

What about file URIs? resource? chrome? data: ? jar:file ? feed: ? moz-icon ? javascript: ?

Can we just only allow http(s) and bail for everything else? That seems saner if we're looking at domains anyway.

@@ +172,5 @@
> +    URICountListener.reset();
> +  },
> +
> +  uninit: function() {
> +    Services.obs.removeObserver(this, "sessionstore-windows-restored", false);

Seems like this should be removed immediately when it has run.

@@ +177,5 @@
> +    Services.obs.removeObserver(this, "domwindowopened", false);
> +    Services.obs.removeObserver(this, "domwindowclosed", false);
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {

Nit: please use method shorthands throughout, ie

observe(subject, topic, data)

Note also that in most places in frontend JS we've stopped using aFoo.

@@ +192,5 @@
> +    }
> +  },
> +
> +  handleEvent: function(aEvent) {
> +    this._log.trace("handleEvent - event " + aEvent);

Is all the tracing actually useful? It makes the code pretty hard to read.

@@ +212,5 @@
> +    // Make sure to catch new chrome windows.
> +    Services.obs.addObserver(this, "domwindowopened", false);
> +    Services.obs.addObserver(this, "domwindowclosed", false);
> +
> +  	// Attach the tabopen handlers to the existing Windows.

Indenting?

@@ +222,5 @@
> +
> +    // Get the initial tab and windows max counts.
> +    const counts = getOpenTabsAndWinsCounts();
> +    Telemetry.scalarSetMaximum("browser.engagement.max_concurrent_tab_count",
> +                               counts.tabCount);

The documentation says "look at the code", but searchfox has no hits for 'scalarsetmaximum'.

@@ +233,5 @@
> +   */
> +  _registerWindow: function(aWin) {
> +    this._log.trace("_registerWindow");
> +    aWin.addEventListener("TabOpen", this, true);
> +    // Add a progress listener to track location changes on non-private windows.

Why can we listen to tabopen events in private windows? Seems like we should just not collect this telemetry for private windows at all.

@@ +238,5 @@
> +    if (PrivateBrowsingUtils.isWindowPrivate(aWin)) {
> +      this._log.trace("_registerWindow - Skipping private window.");
> +      return;
> +    }
> +    aWin.gBrowser.addProgressListener(URICountListener);

If you use addTabsProgressListener, AFAICT you don't get called for tab switches.

@@ +272,5 @@
> +  /**
> +   * Tracks the window count and registers the listeners for the tab count.
> +   * @param{Object} aWindow The window object.
> +   */
> +  _onWindowOpen: function(aWindow) {

This code is assuming that all the windows for which domwindowopened gets called are browser windows, which is not the case.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1275,5 @@
> +    if (!Utils.isContentProcess && isSubsession && clearSubsession) {
> +      protect(() => {
> +        let scope = {};
> +        Cu.import("resource:///modules/BrowserUsageTelemetry.jsm", scope);
> +        scope.BrowserUsageTelemetry.onSubsession();

This type of tight coupling / dependency from toolkit to browser is not a good idea. Can you use an observer notification instead?

Comment 22

[:Gijs (he/him)]

Comment on attachment 8765490 [details] [diff] [review]
part 2 - Test coverage

Clearing test review while we hammer out details on the code.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8765489 [details] [diff] [review]
> part 1 - Implement the browser engagement measurements
> 
> Review of attachment 8765489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we need a new module? Can't this just live in BrowserUITelemetry?
> That already has tab/window counts, which we should probably get rid of if
> we're implementing new ones to avoid confusion about which data is
> better/right.

Thank you for reviewing this.

AFAICT BrowserUITelemetry measures the tabs/win counts at startup and doesn't keep track of the count changes throughout the browser session. This is a different behaviour than the one we want here. Moreover, UITelemetry is opt-in while all the usage probes we're adding are opt-out.

That said, since this is only the first batch of usage probes (more coming in Q3+!), I'd rather keep that as a separate module. If you feel strongly about moving that to BrowserUITelemetry, we can do that and discuss this again when/if the number of usage metrics grow.

Also, but a bit unrelated, documentation will be coming as a separate patch in this bug.

> 
> @@ +177,5 @@
> > +    Services.obs.removeObserver(this, "domwindowopened", false);
> > +    Services.obs.removeObserver(this, "domwindowclosed", false);
> > +  },
> > +
> > +  observe: function(aSubject, aTopic, aData) {
> 
> Nit: please use method shorthands throughout, ie
> 
> observe(subject, topic, data)
> 
> Note also that in most places in frontend JS we've stopped using aFoo.

Ah, good to know, thanks for pointing this out. I guess it would be useful to update
the coding style guide at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects . 

> @@ +192,5 @@
> > +    }
> > +  },
> > +
> > +  handleEvent: function(aEvent) {
> > +    this._log.trace("handleEvent - event " + aEvent);
> 
> Is all the tracing actually useful? It makes the code pretty hard to read.

Most of the tracing code was not so useful in this case, good point. I've removed the logger
and all the tracing log lines.

> @@ +222,5 @@
> > +
> > +    // Get the initial tab and windows max counts.
> > +    const counts = getOpenTabsAndWinsCounts();
> > +    Telemetry.scalarSetMaximum("browser.engagement.max_concurrent_tab_count",
> > +                               counts.tabCount);
> 
> The documentation says "look at the code", but searchfox has no hits for
> 'scalarsetmaximum'.

I think searchfox was lagging behind.

DXR: https://dxr.mozilla.org/mozilla-central/search?q=scalarsetmaximum&redirect=true
Searchfox: http://searchfox.org/mozilla-central/search?q=scalarsetmaximum&path=

IDL: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/nsITelemetry.idl#413
Implementation: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryScalar.cpp#938

> @@ +233,5 @@
> > +   */
> > +  _registerWindow: function(aWin) {
> > +    this._log.trace("_registerWindow");
> > +    aWin.addEventListener("TabOpen", this, true);
> > +    // Add a progress listener to track location changes on non-private windows.
> 
> Why can we listen to tabopen events in private windows? Seems like we should
> just not collect this telemetry for private windows at all.

That's pending a ni? for rweiss/bcolloran on bug 1271319. I think you're right though, so
I disabled collection for private windows.

> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1275,5 @@
> > +    if (!Utils.isContentProcess && isSubsession && clearSubsession) {
> > +      protect(() => {
> > +        let scope = {};
> > +        Cu.import("resource:///modules/BrowserUsageTelemetry.jsm", scope);
> > +        scope.BrowserUsageTelemetry.onSubsession();
> 
> This type of tight coupling / dependency from toolkit to browser is not a
> good idea. Can you use an observer notification instead?

This is a good point.

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Comment 26

[Alessio Placitelli [:Dexter]]

Comment on attachment 8766378 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

There we go with the updated patch, with your comments addressed (some things were discussed in comment 23).

Comment 27

[:Gijs (he/him)]

Comment on attachment 8766378 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

Even if we don't want to put this in BrowserUITelemetry, we should still get rid of the UI Telemetry window/tab measures if we think this is more useful. This should also get a privacy review.

I'm assuming that the scalars implementation of scalarSetMaximum takes care of only setting the number if it's higher than what has been set before.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +37,5 @@
> +}
> +
> +let URICountListener = {
> +  // A map of the visited domains, see bug 1271310.
> +  _domainMap: new Map(),

So... are we interested in toplevel domains here or just in domains? That is, should "www.foo.com" and "foo.com" be counted differently? What about "mail.foo.com" ?

@@ +51,5 @@
> +      return;
> +    }
> +
> +    // Only consider http(s) and data schemas, as we just need to count domains.
> +    if (!uri || (!uri.schemeIs("http") && !uri.schemeIs("https") && !uri.schemeIs("data"))) {

data URIs don't have a domain though... why are we including them?

Under what circumstances do we get called without a uri? (ie is the first check actually necessary?)

@@ +56,5 @@
> +      return;
> +    }
> +
> +    // Update the URI counts.
> +    Services.telemetry.scalarAdd("browser.engagement.total_uri_count", 1);

Should we do this before the early return? Or are we OK only counting http(s) URIs and not local files, ftp, etc. ?

@@ +64,5 @@
> +      return;
> +    }
> +
> +    // |uri.host| can throw if "host" isn't applicable to the URI scheme (e.g. about:* and
> +    // similar pages).

If we don't include anything besides http/https, this shouldn't throw. At a minimum the comment is now wrong.

@@ +66,5 @@
> +
> +    // |uri.host| can throw if "host" isn't applicable to the URI scheme (e.g. about:* and
> +    // similar pages).
> +    try {
> +      this._domainMap.set(uri.host, 1);

Why are we using a map if all we do is set items to one? Seems like the right data type is a Set.

@@ +72,5 @@
> +      return;
> +    }
> +
> +    Services.telemetry.scalarSet("browser.engagement.unique_domains_count",
> +                                 this._domainMap.size);

Feels like we could just set this when we finish the session rather than every time. Is there a way to do this?

@@ +86,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> +                                         Ci.nsISupportsWeakReference]),
> +};
> +
> +this.BrowserUsageTelemetry = Object.freeze({

TBH I don't see much value in the forwarding implementation here. We can simplify by just having 1 object? Also not sure why we're assigning to this.Foo rather than just using 'let Foo'.

@@ +96,5 @@
> +    BrowserUsageTelemetryImpl.uninit();
> +  },
> +});
> +
> +var BrowserUsageTelemetryImpl = {

Nit: let.

@@ +97,5 @@
> +  },
> +});
> +
> +var BrowserUsageTelemetryImpl = {
> +

Nit: please remove the empty line

@@ +108,5 @@
> +   */
> +  onSubsession() {
> +    // Scalars get cleared on subsession splits. We need to set the maximum concurrent
> +    // tab and window counts so that they reflect the correct value for this subsession
> +    // fragment.

This comment is a bit unclear to me. At the point where you get told this, do the telemetry sets do something to the old or the new subsession? If the old, why does it need setting right now? If the new, can you update the comment to be clearer?

@@ +119,5 @@
> +    // Reset the URI counter.
> +    URICountListener.reset();
> +  },
> +
> +  uninit() {

Thinking about this, maybe we should still ensure we remove the sessionstore observer if it hasn't already been removed? (That is, what happens if we get init'd in nsBrowserGlue.js but we never restore any windows...)

@@ +202,5 @@
> +    }
> +
> +    win.removeEventListener("TabOpen", this, true);
> +    // We didn't track private windows, so no need to remove the progress listener there.
> +    if (PrivateBrowsingUtils.isWindowPrivate(win)) {

You've already filtered this out earlier, so this is now redundant.

@@ +228,5 @@
> +    let onLoad = () => {
> +      win.removeEventListener("load", onLoad, false);
> +
> +      // Ignore non-browser or private windows.
> +      if (!(win instanceof Ci.nsIDOMWindow) ||

If the window isn't a DOMWindow, can we even attach the load event listener?

@@ +241,5 @@
> +      Services.telemetry.scalarAdd("browser.engagement.window_open_event_count", 1);
> +      Services.telemetry.scalarSetMaximum("browser.engagement.max_concurrent_window_count",
> +                                          counts.winCount);
> +
> +      // We won't receive the "TabOpen" event for the first tab within a new window.

Nor will we see window open events for windows opened before sessionrestore-windows-restored... do we need to account for that, too?

@@ +243,5 @@
> +                                          counts.winCount);
> +
> +      // We won't receive the "TabOpen" event for the first tab within a new window.
> +      // Account for that.
> +      this._onTabOpen();

Can we just manually set the values here so we avoid calling getOpenTabsAndWinsCounts a second time?

Comment 28

[Alessio Placitelli [:Dexter]]

(In reply to :Gijs Kruitbosch from comment #27)
> Comment on attachment 8766378 [details] [diff] [review]
> part 1 - Implement the browser engagement measurements
> 
> Review of attachment 8766378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Even if we don't want to put this in BrowserUITelemetry, we should still get
> rid of the UI Telemetry window/tab measures if we think this is more useful.
> This should also get a privacy review.

Yes, I know. Will flag :bsmedberg once we're settled with the implementation.

> ::: browser/modules/BrowserUsageTelemetry.jsm
> @@ +37,5 @@
> > +}
> > +
> > +let URICountListener = {
> > +  // A map of the visited domains, see bug 1271310.
> > +  _domainMap: new Map(),
> 
> So... are we interested in toplevel domains here or just in domains? That
> is, should "www.foo.com" and "foo.com" be counted differently? What about
> "mail.foo.com" ?

Good point.
I'm assuming they should be counted differently, but there's a ni? in bug 1271310 comment 17 to clear that out.

> @@ +51,5 @@
> > +      return;
> > +    }
> > +
> > +    // Only consider http(s) and data schemas, as we just need to count domains.
> > +    if (!uri || (!uri.schemeIs("http") && !uri.schemeIs("https") && !uri.schemeIs("data"))) {
> 
> data URIs don't have a domain though... why are we including them?
> [...] Or are we OK only counting http(s) URIs and not local files, ftp, etc. ?

Thinking again about this line, we should probably allow everything but the about:* pages.
We're really interested in the user interaction with the browser here. If the interaction
happens by opening file URIs or other URIs, it's fine.

The downside of this is that file (and similar) schemas don't have a domain name, so they won't contribute to the unique domains count. But I guess that's a little fraction of the URIs, compared to http(s) ones.

> @@ +72,5 @@
> > +      return;
> > +    }
> > +
> > +    Services.telemetry.scalarSet("browser.engagement.unique_domains_count",
> > +                                 this._domainMap.size);
> 
> Feels like we could just set this when we finish the session rather than
> every time. Is there a way to do this?

Doing it every time allows us to have updated values showing in about:telemetry.
Unless there's a performance concern (is there?), I guess we'd want to stick with that.

> @@ +241,5 @@
> > +      Services.telemetry.scalarAdd("browser.engagement.window_open_event_count", 1);
> > +      Services.telemetry.scalarSetMaximum("browser.engagement.max_concurrent_window_count",
> > +                                          counts.winCount);
> > +
> > +      // We won't receive the "TabOpen" event for the first tab within a new window.
> 
> Nor will we see window open events for windows opened before
> sessionrestore-windows-restored... do we need to account for that, too?

No, we're intentionally ignoring stuff happening before the session is restored. But we're
attaching our handlers to the restored windows in |_setupAfterRestore()| so we're still
catching tab creation afterwards.

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Comment 31

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

(In reply to :Gijs Kruitbosch from comment #27)
> Comment on attachment 8766378 [details] [diff] [review]
> part 1 - Implement the browser engagement measurements
> 
> Review of attachment 8766378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Even if we don't want to put this in BrowserUITelemetry, we should still get
> rid of the UI Telemetry window/tab measures if we think this is more useful.

Yes, we're going to rework UITelemetry in this quarter and we will remove the old probes.

@Gijs, I've dropped the URI/Domain probes from this patch and we'll be implementing them in bug 1271313 once the points you raised will be cleared.

@gfritzsche, this patch also breaks tight coupling between the BrowserUsageTelemetry.jsm module and TelemetrySessio.jsm by introducing a new topic, as :Gijs suggested. Flagging you for feedback on that part (as mentioned, the URI/Domain probes will be landing as part of bug 1271313 and not in this bug).

Comment 32

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

I've removed the URI/Domain probe testing from this patch, as the implementation for these probes has been moved to bug 1271313.

Comment 33

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from bug 1271319 comment #7)
> (In reply to Alessio Placitelli [:Dexter] from comment #6)
> > (In reply to Alessio Placitelli [:Dexter] from comment #5)
> > > Sorry for flagging you both, but I'm not sure who should answer.
> > > 
> > > Do we want to include the number of private windows opened in this count?
> > > (ditto for the number of tabs opened).
> > 
> > My gut feeling is that we really shouldn't. We're not doing that for the
> > domain/URI count.
> 
> I think we probably need to include this. If we want to answers questions
> related to window usage etc. we should count all windows.

Cancelling reviews, we should be counting tab/windows in private mode too.

Comment 34

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Context from comment 31. Changed the patch to count private tabs and windows.

Comment 35

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Changed the test to verify that we're counting private tab/windows.

Comment 36

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8767665 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1274,5 @@
>      }
>  
> +    // Notify that there was a subsession split in the parent process.
> +    if (isSubsession && clearSubsession) {
> +      Services.obs.notifyObservers(null, "telemetry-subsession-split", "");

I'm not strictly opposed to this, but i'm worried that other code will start using the notification once it's available (like the old "gather-telemetry" that we want to remove).
From what i understand, a notification has two advantages here:
(1) logical decoupling and
(2) hanging off this notification from multiple places

We don't actually want (2) here (details below).
Is (1) important enough to not just directly call into BrowserUsageTelemetry?
Or can we make reasonably sure other users don't start using this notification?

Currently we are really trying to keep users on a "push" model (i.e. directly record your values into Telemetry, which is the storage), as opposed to a "pull" model (where Telemetry signals other modules to do work or collects data from them).
From FHR we learned that a "pull" model has some problems:
* the semantics are not centrally controlled and hard to change
* blocking and waiting on other data collection code (which will end up having bugs)
What we do here is a special case where we need to collect values after subsession splits - but so far this seems to be very rare.
I'd like to not offer a public notification to do this and see what other  use-cases we have (if any) so we can properly design this.

If we end up doing it this way:
* This could change payload contents, so it would have to be notified after all the payload collection. getSessionPayload() is the more fitting location for this.
* This should also be named more clearly, e.g. "telemetry-after-subsession-split" and needs proper documentation.

Comment 37

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8767665 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

::: toolkit/components/telemetry/Scalars.yaml
@@ +68,5 @@
> +    bug_numbers:
> +      - 1271304
> +    description: >
> +      The count of maximum number of tabs open during a subsession,
> +      across all windows.

The document for bug 1281813 has useful info on what is and is not recorded for each probe:
https://docs.google.com/document/d/1Y8IZP85mg_cbaOud30WamjUh6EIhOu-Re3LZ1mWxwLs/edit

We should add this here.

Comment 40

[Alessio Placitelli [:Dexter]]

Sorry for flagging you both, but I'm not sure who should answer.

Do we want to include the number of private windows opened in this count? (ditto for the number of tabs opened).

Comment 41

[:Gijs (he/him)]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #36)
> Comment on attachment 8767665 [details] [diff] [review]
> part 1 - Implement the browser engagement measurements
> 
> Review of attachment 8767665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1274,5 @@
> >      }
> >  
> > +    // Notify that there was a subsession split in the parent process.
> > +    if (isSubsession && clearSubsession) {
> > +      Services.obs.notifyObservers(null, "telemetry-subsession-split", "");
> 
> I'm not strictly opposed to this, but i'm worried that other code will start
> using the notification once it's available (like the old "gather-telemetry"
> that we want to remove).
> From what i understand, a notification has two advantages here:
> (1) logical decoupling and
> (2) hanging off this notification from multiple places
> 
> We don't actually want (2) here (details below).
> Is (1) important enough to not just directly call into BrowserUsageTelemetry?
> Or can we make reasonably sure other users don't start using this
> notification?

Toolkit code can never assume that browser/ modules exist or call into them. So yes, it's important enough not to "just" directly call into BrowserUsageTelemetry.

We could use a different pattern, like code having to directly register some kind of listener with TelemetrySession.jsm to get notified?

Alternatively, we could avoid all of this and just register a TabClose event listener as well as a TabOpen one and update the tab count there, which would guarantee the correct count would be present in the new subsession as well.

Comment 42

[:Gijs (he/him)]

Or better yet, we could have some kind of annotation on telemetry measures (scalar or otherwise) indicating they should persist when we swap out subsessions while the browser stays up...

Comment 43

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gijs Kruitbosch from comment #42)
> Or better yet, we could have some kind of annotation on telemetry measures
> (scalar or otherwise) indicating they should persist when we swap out
> subsessions while the browser stays up...

That would get more involved:
In this case we want to reset it to e.g. the last "no. of tabs currently open", but we are actually storing the maximum for the whole last subsession in it.

Comment 44

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gijs Kruitbosch from comment #41)
> > ::: toolkit/components/telemetry/TelemetrySession.jsm
> > @@ +1274,5 @@
> > >      }
> > >  
> > > +    // Notify that there was a subsession split in the parent process.
> > > +    if (isSubsession && clearSubsession) {
> > > +      Services.obs.notifyObservers(null, "telemetry-subsession-split", "");
> > 
> > I'm not strictly opposed to this, but i'm worried that other code will start
> > using the notification once it's available (like the old "gather-telemetry"
> > that we want to remove).
> > From what i understand, a notification has two advantages here:
> > (1) logical decoupling and
> > (2) hanging off this notification from multiple places
> > 
> > We don't actually want (2) here (details below).
> > Is (1) important enough to not just directly call into BrowserUsageTelemetry?
> > Or can we make reasonably sure other users don't start using this
> > notification?
> 
> Toolkit code can never assume that browser/ modules exist or call into them.
> So yes, it's important enough not to "just" directly call into
> BrowserUsageTelemetry.

Sure, not directly and expecting it to be available, but a "lazy" call - import & call if available?
Sounds like a bad pattern though.

> We could use a different pattern, like code having to directly register some
> kind of listener with TelemetrySession.jsm to get notified?

Hm, that sounds more reasonable. Then we can at least put disclaimer comments on the registration function.

> Alternatively, we could avoid all of this and just register a TabClose event
> listener as well as a TabOpen one and update the tab count there, which
> would guarantee the correct count would be present in the new subsession as
> well.

That would mean that the measurement is out of date until the next tab open/close event.

Comment 45

[:Gijs (he/him)]

Comment on attachment 8767665 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

Clearing review pending discussion about how to deal with the subsession issue.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +126,5 @@
> +   * Updates the tab counts.
> +   * @param {Number} [newTabCount=0] The count of the opened tabs across all windows. This
> +   *        is computed manually if not provided.
> +   */
> +  _onTabOpen(newTabCount=0) {

Nit: spaces around operators.

Can we call this "tabCount" ? "newTabCount" in conjunction with "onTabOpen" makes it sound like we're dealing with multiple tabs being opened.

@@ +128,5 @@
> +   *        is computed manually if not provided.
> +   */
> +  _onTabOpen(newTabCount=0) {
> +    // Use the provided tab count if available. Otherwise, go on and compute it.
> +    const tabCount = (!newTabCount) ? getOpenTabsAndWinsCounts().tabCount : newTabCount;

just reassign to the argument:

tabCount = tabCount || getOpenTabsAndWinsCounts().tabCount;

Comment 46

[:Gijs (he/him)]

Comment on attachment 8767666 [details] [diff] [review]
part 2 - Test coverage

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

Bunch of minor issues listed below. Seems OK, but apparently there's no consensus on the private window stuff so I should probably review this again when the consensus is there.

::: browser/modules/test/browser_UsageTelemetry.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

Nit: don't actually need to add this.

@@ +4,5 @@
> +"use strict";
> +
> +var BrowserUsageTelemetry =
> +  Cu.import("resource:///modules/BrowserUsageTelemetry.jsm", {}).BrowserUsageTelemetry;
> +var Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);

Nits: no 'var', anywhere.

Instead of the XPCOM usage, please just use Services.telemetry (which you don't need to import because the code runs in a browser window scope so it's available already).

@@ +23,5 @@
> + *
> + * @return {Promise}
> + * @resolves When navigating to a non-error page.
> + */
> +function browserLocationChanged(browser) {

You never call this.

@@ +63,5 @@
> +
> +add_task(function* test_tabsAndWindows() {
> +  // Let's reset the counts.
> +  Telemetry.clearScalars();
> +  Services.obs.notifyObservers(null, TELEMETRY_SUBSESSION_TOPIC, "");

Feels like you should call into the observe() method yourself manually - if there were other consumers, this would break. But seems we're rearching this anyway.

@@ +76,5 @@
> +    // Get a snapshot of the scalars and simulate a subsession split (clear the
> +    // measuremenets).
> +    const scalars =
> +      Telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);
> +    Services.obs.notifyObservers(null, TELEMETRY_SUBSESSION_TOPIC, "");

I think we only really need to do this after we've removed a tab, no? Also, this code is hard to read - you're doing:


- get values
- reset
- check values

but the reset has no effect because you've already snapshotted the values.

Can we move the reset down, or even better, can you just only trigger the reset when we need to, and not inside the same function that you're using to assert things about the current state of the world/telemetry?

@@ +107,5 @@
> +  // Add a new window and then some tabs in it. An empty new windows counts as a tab.
> +  let win = yield BrowserTestUtils.openNewBrowserWindow();
> +  openedTabs.push(yield BrowserTestUtils.openNewForegroundTab(win.gBrowser, "about:blank"));
> +  openedTabs.push(yield BrowserTestUtils.openNewForegroundTab(win.gBrowser, "about:blank"));
> +  let rougueTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");

Nit: rogueTab

... but this is fairly hard to reason about. Can we just add it to the openedTabs array and then remove it using:

yield BrowserTestUtils.removeTab(openedTabs.pop());

?

@@ +111,5 @@
> +  let rougueTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
> +  expectedTabOpenCount = 4;
> +  expectedWinOpenCount = 1;
> +  expectedMaxWins = 2;
> +  // The new window started with a new tab, so account for it.

Seems like this comment should be before the expectedTabOpenCount assignment.

Comment 47

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #44)
> (In reply to :Gijs Kruitbosch from comment #41)
> > > ::: toolkit/components/telemetry/TelemetrySession.jsm
> > > @@ +1274,5 @@
> > > >      }
> > > >  
> > > > +    // Notify that there was a subsession split in the parent process.
> > > > +    if (isSubsession && clearSubsession) {
> > > > +      Services.obs.notifyObservers(null, "telemetry-subsession-split", "");
> > > 
> > > I'm not strictly opposed to this, but i'm worried that other code will start
> > > using the notification once it's available (like the old "gather-telemetry"
> > > that we want to remove).
> > > From what i understand, a notification has two advantages here:
> > > (1) logical decoupling and
> > > (2) hanging off this notification from multiple places
> > > 
> > > We don't actually want (2) here (details below).
> > > Is (1) important enough to not just directly call into BrowserUsageTelemetry?
> > > Or can we make reasonably sure other users don't start using this
> > > notification?
> > 
> > Toolkit code can never assume that browser/ modules exist or call into them.
> > So yes, it's important enough not to "just" directly call into
> > BrowserUsageTelemetry.
> 
> Sure, not directly and expecting it to be available, but a "lazy" call -
> import & call if available?
> Sounds like a bad pattern though.
> 
> > We could use a different pattern, like code having to directly register some
> > kind of listener with TelemetrySession.jsm to get notified?
> 
> Hm, that sounds more reasonable. Then we can at least put disclaimer
> comments on the registration function.

Running through this on IRC, we will go with the observer notification for now, prefixing it with "internal-" to make clear its not for public use.

(In reply to :Gijs Kruitbosch from comment #46)
> Bunch of minor issues listed below. Seems OK, but apparently there's no
> consensus on the private window stuff so I should probably review this again
> when the consensus is there.

There were no concerns about this during the design, i think we shouldn't block on this.
If there are privacy concerns or so they will come up in the data collection review.

Comment 48

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Clearning the ni? as per comment 47.

Comment 49

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Comment 50

[Alessio Placitelli [:Dexter]]

Comment on attachment 8768052 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

@Benjamin, asking data-review? for the newly introduced tab/window opt-out probes described in the PRD document at [1].

The description of the new probes is in the Scalars.yaml file.

@Gijs, I've addressed all your suggestions.

@Gfritzsche, I've changed the notification as described in comment 37 and added the proper documentation.

[1] - https://docs.google.com/document/d/1UuztyM-fdQBR88QjOiMIbqlHhLGMVfvPF_xbyhKWmOQ/edit?ts=56c378e3#heading=h.ygjz8s3u0xtp

Comment 51

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8768052 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +32,5 @@
> +
> +  /**
> +   * Handle subsession splits in the parent process.
> +   */
> +  onSubsession() {

afterSubsessionSplit()

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1352,5 @@
>          let sessionData = this._getSessionDataObject();
>          TelemetryStorage.saveSessionData(sessionData);
> +
> +        // Notify that there was a subsession split in the parent process.
> +        if (!this._isClassicReason(reason)) {

Why is this check needed?
Do we hit this path for "saved-session" too?
If so, we can add that check at line 1349 - none of this is needed for "saved-session".

Comment 52

[Alessio Placitelli [:Dexter]]

Comment on attachment 8768053 [details] [diff] [review]
part 2 - Test coverage

(In reply to :Gijs Kruitbosch from comment #46)
> Comment on attachment 8767666 [details] [diff] [review]
> part 2 - Test coverage
> 
> Review of attachment 8767666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bunch of minor issues listed below. Seems OK, but apparently there's no
> consensus on the private window stuff so I should probably review this again
> when the consensus is there.

r? again as per comment 47.

> @@ +63,5 @@
> > +
> > +add_task(function* test_tabsAndWindows() {
> > +  // Let's reset the counts.
> > +  Telemetry.clearScalars();
> > +  Services.obs.notifyObservers(null, TELEMETRY_SUBSESSION_TOPIC, "");
> 
> Feels like you should call into the observe() method yourself manually - if
> there were other consumers, this would break. But seems we're rearching this
> anyway.

Yeah, but the topic is strictly for using by the Browser module, so it shouldn't be a problem in this case.

Moreover, I just noticed that by declaring |let BrowserUsageTelemetry =| instead of using |var| prevents me from using BrowserUsageTelemetry with |Cu.import()| from the tests, unless I'm doing something terribly stupid.

> @@ +76,5 @@
> > +    // Get a snapshot of the scalars and simulate a subsession split (clear the
> > +    // measuremenets).
> > +    const scalars =
> > +      Telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);
> > +    Services.obs.notifyObservers(null, TELEMETRY_SUBSESSION_TOPIC, "");
> 
> I think we only really need to do this after we've removed a tab, no? Also,
> this code is hard to read - you're doing:
> 
> - get values
> - reset
> - check values
> 

|Telemetry.snapshotScalars| is the instruction clearing the scalars (due to the second parameter being true). I've changed to comment to be more clear about that.

The topic notification is always needed, not only when a tab is removed. Consider the case:

- Firefox stats.
- 2 windows are opened, the maximum number of windows is set to 2.
- Subsession split (and scalars cleared)
- Check data here

If no topic is notified, the scalars are cleared but the maximum is not set to the current number of opened windows. So the scalar will be unset, but we'll have 2 opened windows.


> but the reset has no effect because you've already snapshotted the values.
> 
> Can we move the reset down, or even better, can you just only trigger the
> reset when we need to, and not inside the same function that you're using to
> assert things about the current state of the world/telemetry?

Basically, the snapshot + notification lines are simulating a subsession split. That's the reason I put the close to each other. Without the notification, the max_concurrent_windows/tabs scalars will not have the correct value. I've changed the comment to be more clear about it.

Comment 53

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #51)
> Comment on attachment 8768052 [details] [diff] [review]
> part 1 - Implement the browser engagement measurements
> 
> Review of attachment 8768052 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1352,5 @@
> >          let sessionData = this._getSessionDataObject();
> >          TelemetryStorage.saveSessionData(sessionData);
> > +
> > +        // Notify that there was a subsession split in the parent process.
> > +        if (!this._isClassicReason(reason)) {
> 
> Why is this check needed?
> Do we hit this path for "saved-session" too?
> If so, we can add that check at line 1349 - none of this is needed for
> "saved-session".

My bad, I've removed the check.

Comment 54

[:Gijs (he/him)]

Comment on attachment 8768052 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

r=me with gfritzsche's comments addressed.

Comment 55

[:Gijs (he/him)]

Comment on attachment 8768053 [details] [diff] [review]
part 2 - Test coverage

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

::: browser/modules/test/browser_UsageTelemetry.js
@@ +35,5 @@
> +    // the scalars) and notifying the TELEMETRY_SUBSESSION_TOPIC topic. The latter is needed
> +    // to have the correct values in the MAX_CONCURRENT_* scalars.
> +    const scalars =
> +      Services.telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true /*clearScalars*/);
> +    Services.obs.notifyObservers(null, TELEMETRY_SUBSESSION_TOPIC, "");

I don't understand. You snapshot the scalars before this statement. Assuming the snapshot is actually what it says on the tin, ie a snapshot, it shouldn't matter whether or not you split the subsession here or after you call checkScalar 4 times. It also doesn't seem to matter for the private browsing case in the other task, where you don't split the subsession after calling snapshotScalars.

Comment 56

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Comment 57

[Alessio Placitelli [:Dexter]]

Comment on attachment 8768381 [details] [diff] [review]
part 2 - Test coverage

As discussed over IRC, I've moved the part covering the value of the MAX_CONCURRENT_* scalars after subsession splits in a separate test.

Comment 58

[:Gijs (he/him)]

Comment on attachment 8768381 [details] [diff] [review]
part 2 - Test coverage

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

::: browser/modules/test/browser_UsageTelemetry.js
@@ +77,5 @@
> +
> +  // Remove all the extra windows and tabs.
> +  for (let tab of openedTabs) {
> +    yield BrowserTestUtils.removeTab(tab);
> +  }

Could checkScalars again here?

Comment 59

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Test coverage

Added a |checkScalars| check after clearing the windows/tabs.

Comment 60

[Benjamin Smedberg]

Comment on attachment 8768052 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

For data review of opt-out metrics, I need a clear statement of what user value is provided by collecting these metrics, and who is responsible for delivering that user value. I'm going to NEEDINFO rweiss on this because there was some draft of this in the google doc, but it needs to be public and specific. The last thing I saw clearly justified collecting this for a period as an exploratory measurement, but not as a permanent measurement. It's not clear whether collecting this data is intended to provide direct benefit, or whether this is a correlate to some other measurement. 

The notification emails should include a particular person who is responsible for each metric, not just the telemetry-client-dev mailing list.

Comment 61

[[e-mail]]

This metric is intended to be opt-out and permanent.  This is because the product team believes that capturing this measurement is fundamental to the understanding the ways our users interact with the browser.

Most immediately, we require understanding of normal tab usage in order to identify whether Tab Center has improved or harmed tab usage.  This means we require baseline measurements of tab usage in order to perform the comparison.  The decision to graduate Tab Center from Test Pilot will require data, and this measurement is aimed towards that cause.

Additionally, there is a greater need to improve our understanding of engagement with the browser beyond days of activity in a month.  We want to use measures of types of usage via these browser interactions in order to determine whether a product change has harmed or improved usage of the browser.  This is why the measure is intended to be permanent.

This measurement is slated for use as outcome measures for evaluating the success or improvement of new feature development.  Therefore the onus of responsibility for delivering user value is on each individual product manager (and affiliated data analyst(s)) who intend(s) to use usage of the browser as an outcome measure for success.  This measurement will be tracked as part of a general UI Telemetry dashboard or in individual reporting (as in the case of Tab Center and other Test Pilot tests).

Comment 62

[[e-mail]]

:bsmedberg, does the above answer your questions?

Comment 63

[Benjamin Smedberg]

Comment on attachment 8768052 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

Talked this over with Rebecca (and other data steward peers) today. There is a clear user benefit to collect this for tab center, so we're going to start this out for six months and then evaluate whether this data was valuable and what other product decisions will be made at that time.

So data-review=me with expires: 55 (I have not reviewed the code, just the scalar definitions)

Comment 64

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Comment 65

[Alessio Placitelli [:Dexter]]

Comment on attachment 8771351 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

r? for the Telemetry changes.

Comment 66

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Fix the test failures

The tests fail locally due to "sessionstore-windows-restored" being received multiple times in BrowserUsageTelemetry.

That's because of the browser_SelfSupportBackend.js notifies that multiple times, globally. Since that seems to be the only test having this wrong behaviour, I changed that instead of adding additional checks to BrowserUsageTelemetry.jsm.

Comment 67

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8771351 [details] [diff] [review]
part 1 - Implement the browser engagement measurements

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

LGTM with the below addressed.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +26,5 @@
> +}
> +
> +let BrowserUsageTelemetry = {
> +  init() {
> +    Services.obs.addObserver(this, "sessionstore-windows-restored", false);

There are a lot of repeated strings in this file that could be written as constants.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1351,5 @@
>          // Persist session data to disk (don't wait until it completes).
>          let sessionData = this._getSessionDataObject();
>          TelemetryStorage.saveSessionData(sessionData);
> +
> +        // Notify that there was a subsession split in the parent process.

Let's clearly document here that this is an internal topic.

@@ +1352,5 @@
>          let sessionData = this._getSessionDataObject();
>          TelemetryStorage.saveSessionData(sessionData);
> +
> +        // Notify that there was a subsession split in the parent process.
> +        Services.obs.notifyObservers(null, "internal-telemetry-after-subsession-split", "");

The last parameter should be `null`.

::: toolkit/components/telemetry/docs/main-ping.rst
@@ +14,5 @@
>  * ``saved-session`` - the *"classic"* Telemetry payload with measurements covering the whole browser session (only submitted for a transition period)
>  
>  Most reasons lead to a session split, initiating a new *subsession*. We reset important measurements for those subsessions.
>  
> +After a new subsession split, the ``internal-telemetry-after-subsession-split`` topic is notified to all the observers. *This is an internal topic and is not meant to be used.*

"... and is only meant for internal Telemetry usage."

Comment 68

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Implement the browser engagement measurements

Comment 69

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53c0dead7400

Comment 70

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63eeb207ec658de26ec797a9f85a9b1daa7ad158
Bug 1271304 - Measure the tabs/window open events and max number of tab/window per subsession. r=Gijs, r=gfritzsche, data-review=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/9de4826f4e6b23b1ab16aad29d3d8d6690331101
Bug 1271304 - Add test coverage for browser usage metrics. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3705099aaf367c5a7389d6ccaf2b7ef1f3eac
Bug 1271304 - Fix browser_SelfSupportBackend.js to only send "sessionstore-windows-restored" to SelfSupportBackend.jsm. r=Gijs

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63eeb207ec65
https://hg.mozilla.org/mozilla-central/rev/9de4826f4e6b
https://hg.mozilla.org/mozilla-central/rev/f5f3705099aa