Before implementing bug 1303333 (and bug 1295932), we should consider implementing a new scalar probe that counts the total number of URIs opened, without any filtering or restriction. This way we can have a baseline value to compare the new counts against. The "browser.engagement.total_uri_count" [1] is restricted to http(s) protocols non-background requests. [1] - https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/components/telemetry/Scalars.yaml#127

Brendan, once we land bug 1303333 and the rest of probes for bug 1295932, we would need some baseline values to compare and validate the new measurements. "browser.engagement.total_uri_count" only accounts for a fraction of the URI loads (and doesn't count URI loads in private mode). Is it enough to compare against that probe, or should we land a new one that has less restrictions and filters?

Just commented in bug 1295932 -- agree that we need to be able to use total_uri_count as a baseline for comparison. Rather than implementing a new, less restrictive URI counting probe, perhaps we should just use the same set of filters for the probes that count URI loads triggered by specific actions. I think that probably makes sense since we already discussed the filters for total_uri_count, so they should be appropriate for use in this context as well. Thanks Alessio.

(In reply to brendan c from comment #2) > Just commented in bug 1295932 -- agree that we need to be able to use > total_uri_count as a baseline for comparison. Rather than implementing a > new, less restrictive URI counting probe, perhaps we should just use the > same set of filters for the probes that count URI loads triggered by > specific actions. I think that probably makes sense since we already > discussed the filters for total_uri_count, so they should be appropriate for > use in this context as well. Thanks Alessio. I see. I had this doubt because I'm assuming users can bookmark and visit many other non-http/https websites. This is not strictly a problem for the search probes that will be implemented by bug 1303333, as searches should be carried over HTTP(s) anyway. However, bookmarks, awesomebar selection and manual navigation could skew that results. Moreover, I'm not sure how feasible and how complicated is tracking the specific URI connection (marking them in some way) in order to break them down by the triggering action. A simpler approach, like counting at the call site which triggers the load, could be more affected by the filtering issues.

Brendan, are you going to own this probe or should :rweiss?

i can own it.

Comment on attachment 8800290 [details] [diff] [review] bug1304647.patch Gijs, flagging you for review as you already have the background from bug 1271313. This probe is needed to count all the URI loads, without being restricted to http(s). So about:* pages, data:* and so on.

Comment on attachment 8800290 [details] [diff] [review] bug1304647.patch Review of attachment 8800290 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review because of the initial pages stuff. ::: browser/modules/test/browser_UsageTelemetry.js @@ +118,5 @@ > // Remove a tab from the first window, the max shouldn't change. > yield BrowserTestUtils.removeTab(openedTabs.pop()); > + // Unlike |BrowserTestUtils.openNewForegroundTab|, opening a new Window correctly triggers > + // an "about:blank" opening. > + checkScalars(expectedMaxTabs, expectedTabOpenCount, expectedMaxWins, expectedWinOpenCount, 0, 0, 1); We shouldn't count the initial about:blank load, IMO. That will artificially inflate numbers. We should probably ignore about:newtab as well, and maybe about:home and about:privatebrowsing (use the gInitialPages set). In this case, if it only happens with a new window, isn't this already about:home that you're noticing loading? I also note that you're using openNewForegroundTab, which isn't very realistic as a lot of new tabs will be opened by clicking the new tab button or using its shortcut (accel-t) which will load or swap in a preloaded about:newtab browser. Do we count that, too? Based on this, I also expect that the new tab stuff (and maybe the new window stuff, depending on when we add observers) will be racy, as new tabs might be preloaded and then not report the initial about:blank load, but if you open a new tab before the preload of about:newtab has finished (or you just open 3 at the same time) then I don't know that that's what would happen... @@ +143,5 @@ > openedTabs.push(yield BrowserTestUtils.openNewForegroundTab(win.gBrowser, "http://www.example.com")); > > // Check that the scalars have the right values. > checkScalars(5 /*maxTabs*/, 4 /*tabOpen*/, 2 /*maxWins*/, 1 /*winOpen*/, > + 1 /* toalURIs */, 1 /* uniqueDomains */, 2 /* unfilteredURIs */); fix the comment while you're here? Why does this produce 2 unfiltered URIs? @@ +158,5 @@ > // After a subsession split, only the MAX_CONCURRENT_* scalars must be available > // and have the correct value. No tabs or windows were opened so other scalars > // must not be reported. > checkScalars(4 /*maxTabs*/, 0 /*tabOpen*/, 2 /*maxWins*/, 0 /*winOpen*/, > + 0 /* toalURIs */, 0 /* uniqueDomains */, 0 /* unfilteredURIs */); And this comment @@ +178,5 @@ > Services.telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN); > checkScalar(scalars, TOTAL_URI_COUNT, URICount, > "The URI scalar must contain the expected value."); > + checkScalar(scalars, UNFILTERED_URI_COUNT, unfilteredURIs, > + "The unfiltered URI scalar must contain the expected value."); Put this at the end, to match the order of the parameters. @@ +226,5 @@ > > // Check test.domain.com and some.domain.com are only counted once unique. > yield BrowserTestUtils.loadURI(newWin.gBrowser.selectedBrowser, "http://test1.example.com/"); > yield BrowserTestUtils.browserLoaded(newWin.gBrowser.selectedBrowser); > + checkCounts(4, 1, 5); The comment here is misleading because it lists two subdomains and we're really loading one parent and one subdomain, and we don't load another subdomain here.

Comment on attachment 8801085 [details] [diff] [review] bug1304647.patch Review of attachment 8801085 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review, Gijs! I've addressed your concerns, and I've got two questions: - Is there a better way to get the gInitialPages compared to the one I've used? - Does filtering out the initial pages fix the racing potential that you pointed out?

Comment on attachment 8801085 [details] [diff] [review] bug1304647.patch Review of attachment 8801085 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the fixes below. ::: browser/modules/test/browser_UsageTelemetry.js @@ +143,4 @@ > openedTabs.push(yield BrowserTestUtils.openNewForegroundTab(win.gBrowser, "http://www.example.com")); > > + // Check that the scalars have the right values. We expect 2 unfiltered URI loads > + // (about:mozilla and www.example.com, but no about:blank) and 1 URI loin totalURIs loin? @@ +148,2 @@ > checkScalars(5 /*maxTabs*/, 4 /*tabOpen*/, 2 /*maxWins*/, 1 /*winOpen*/, > + 1 /* toalURIs */, 1 /* uniqueDomains */, 2 /* unfilteredURIs */); This still says "toalURIs". Note that the parameter name is a bit unfortunate anyway, because now the total is less than the unfilteredURIs... Maybe we can name the new thing "totalUnfilteredURIs" or something to make it slightly less counterintuitive? @@ +202,4 @@ > yield BrowserTestUtils.removeTab(secondTab); > > + // Open a new window and set the tab to a new address. Opening a new window also > + // triggers an about:blank loading. But we're ignoring about:blank, right? So why change this comment? I'm assuming the increases are only due to the load of example.com ?

(In reply to Alessio Placitelli [:Dexter] from comment #9) > - Is there a better way to get the gInitialPages compared to the one I've > used? I don't think so, so let's use this for now. > - Does filtering out the initial pages fix the racing potential that you > pointed out? I believe so, yes. Thanks!

> @@ +148,2 @@ > > checkScalars(5 /*maxTabs*/, 4 /*tabOpen*/, 2 /*maxWins*/, 1 /*winOpen*/, > > + 1 /* toalURIs */, 1 /* uniqueDomains */, 2 /* unfilteredURIs */); Drive-by feedback: How about changing these into: checkScalars({maxTabs: 1, tabOpen: "foo", ...}); That gives the same readability and should be more flexible to use.

Thanks Gijs, I've addressed your comments and also changed checkScalars as suggested by Georg. That should make things clearer.

Comment on attachment 8802029 [details] [diff] [review] bug1304647.patch data-review?Benjamin - This patch adds a new opt-out scalar probe, set to expire in FF55, owned by Brendan Colloran. This probe counts the number of uri loads, as total_uri_count does, but it's not restricted to HTTP(s) (so we also get bookmarks, about:* pages, etc.). This will be used as a baseline to compare the data coming from the probes implemented in bug 1295932. Please see the change in the Scalars.yaml file for the documentation about the probe.

Comment on attachment 8802029 [details] [diff] [review] bug1304647.patch Moving the data-review to Rebecca as Benjamin is a bit busy at the moment. Rebecca, see comment 14 for context.

I assert the following: - Brendan is the lead analyst for this data (and Javaun is a co-sponsor, since this will be used as a baseline to compare search and navigation behaviors). - This is a temporary measurement that is a candidate for permanent measurement, pending the evaluation from Brendan and Javaun. Signing off on data review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2027674d0b299a4dba5d27911d22fdb333ab5f6 Bug 1304647 - Measure the number of total URIs visited in a "session fragment", without any filtering. r=gijs, data-review=rweiss

Comment on attachment 8802029 [details] [diff] [review] bug1304647.patch Approval Request Comment [Feature/regressing bug #]: This adds a new baseline measuerment to compare data from bug 1303333 (search probes). [User impact if declined]: Browser users won't have a problem. The colleagues trying to understand the new search probes will, as they will lack baseline data to compare against. [Describe test coverage new/current, TreeHerder]: This patch has lived for a while on m-c and adds test coverage. [Risks and why]: Low, this patch adds a new measuerment and the changes are restricted to the measurement module. The worst it could happen is that we don't gather this data. [String/UUID change made/needed]: None

Comment on attachment 8802029 [details] [diff] [review] bug1304647.patch This patch is also related to gather data for search and adds a new baseline measurement to compare data. Take it in 51 aurora.

Aurora changeset: https://hg.mozilla.org/releases/mozilla-aurora/rev/c445246757267ded8f1d71bfd4241b663193d3b2