WIP/DNM: Add page requests metric 209 #389

groovecoder · 2017-03-16T18:53:55Z

No description provided.

groovecoder · 2017-03-16T19:02:42Z

docs/metrics.md

@@ -49,7 +49,7 @@ whether they are actively interacting with it.
  * Average number of container tabs when new window was clicked
 * How many containers do users have hidden at the same time? (when should we measure this? each time a container is hidden?)
 * Do users pin container tabs? (do we have existing Telemetry for pinning?)
-* Do users change URLs in a container tab? (sounds like it could be a flood unless we only record the first URL change?)
+* Do users visit more pages in container tabs than non-container tabs?


To query for this, we will need to do something like:

SELECT AVG(pagerequestcount) FROM containers_testpilottest WHERE event = 'page-requests-completed-per-tab' AND totalcontainerscount = 0 GROUP BY uuid

compared to

SELECT AVG(pagerequestcount) FROM containers_testpilottest WHERE event = 'page-requests-completed-per-tab' AND totalcontainerscount > 0 GROUP BY uuid

groovecoder · 2017-03-23T20:43:34Z

@bakulf or @jonathanKingston - can I get an r? on the JS code here?

jonathanKingston · 2017-03-30T17:22:51Z

webextension/manifest.json

@@ -19,8 +19,11 @@
  "homepage_url": "https://testpilot.firefox.com/",

  "permissions": [
+    "contextualIdentitities",


Is this even required? This is only available in 53 which is an issue.

This is how I get to tab.cookieStoreId right?

Sure enough, didn't need this. Just needed the cookies permission.

jonathanKingston · 2017-03-30T17:23:21Z

webextension/manifest.json

    "cookies",
-    "tabs"
+    "tabs",
+    "webRequest",


Merge issues ahead...

jonathanKingston · 2017-03-30T17:25:09Z

webextension/background.js

@@ -72,3 +73,28 @@ function disableAddon(tabId) {
  browser.browserAction.disable(tabId);
  browser.browserAction.setTitle({ tabId, title: "Containers disabled in Private Browsing Mode" });
 }
+
+browser.tabs.onCreated.addListener(tab => {


Is it worth having these methods self contained. I did the same for the assign code as it ended up getting mental. Seems like this is all related to tracking tabPageCounters etc.

Either that or for efficiency maybe we have an that does all the browser.apiName obj call methods to other objects for both the assignment and counting... however I dunno if we have a perf issue.

Like:

const apiObj = { init() { browser.webRequest.onCompleted.addListener((details) => { tabPageCounters.request.onComplete(....) assignManager.calculateContextMenu(...) }); } }

Makes more sense to contain browser.* API calls when they may share some code and/or scope. Like the themeManager. So something more like ...

const TabPageCountManager = { init() { this.counter = {}; browser.tabs.onCreated.addListener(this.initTabPageloadCounter); browser.tabs.onRemoved.addListener(this.sendTabPageloadCountAndDelete); browser.webRequest.onCompleted.addListener(this.incrementTabPageloadCount, {urls: ["<all_urls>"], types: ["main_frame"]}); }, initTabPageloadCounter() { ... }, sendTabPageloadCountAndDelete() { ... }, incrementTabPageloadCount() { ... } }

groovecoder · 2017-03-30T18:32:57Z

Refactored a new tabPageCounter.

jonathanKingston · 2017-03-30T19:00:39Z

webextension/background.js

+const tabPageCounter = {
+  init() {
+    browser.tabs.onCreated.addListener(this.initTabCounter);
+    browser.tabs.onRemoved.addListener(this.sendTabCountAndDelete);


Does this capture window closures?

Just checked in about:telemetry and yes - it does.

jonathanKingston · 2017-03-30T19:01:10Z

webextension/background.js

+  init() {
+    browser.tabs.onCreated.addListener(this.initTabCounter);
+    browser.tabs.onRemoved.addListener(this.sendTabCountAndDelete);
+    browser.webRequest.onCompleted.addListener(this.incrementTabCount, {urls: ["<all_urls>"], types: ["main_frame"]});


Do you care about tab.tabId might be -1 for non rendered tabs?

What's an example of a non-rendered tab I can spot-check?

jonathanKingston · 2017-03-30T19:02:00Z

webextension/background.js

@@ -1,3 +1,4 @@
+const tabPageRequestCounter = {};


This may as well be internal to tabPageCounter no?

jonathanKingston · 2017-03-30T19:03:30Z

webextension/background.js

+  },
+
+  incrementTabCount(details) {
+    browser.tabs.get(details.tabId).then(tab => {


Are we (singleArg) => {} or singleArg => {}. One of us will have to change. Seems we are inconsistent in existing code anyway :(

I mildly prefer NOT to have the extra ( and ) characters in the code just to reduce character noise. But I don't care too much.

groovecoder changed the title ~~Add page requests metric 209~~ WIP/DNM: Add page requests metric 209 Mar 16, 2017

groovecoder commented Mar 16, 2017

View reviewed changes

groovecoder mentioned this pull request Mar 17, 2017

combine addon telemetry data with containers telemetry data #321

Closed

groovecoder requested a review from jonathanKingston March 28, 2017 14:41

jonathanKingston reviewed Mar 30, 2017

View reviewed changes

webextension/manifest.json Outdated

"cookies",

"tabs"

"tabs",

"webRequest",

Copy link

Contributor

jonathanKingston Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge issues ahead...

jonathanKingston reviewed Mar 30, 2017

View reviewed changes

groovecoder force-pushed the add-page-requests-metric-209 branch from 6c13b93 to 53dfbf9 Compare March 30, 2017 18:32

groovecoder force-pushed the add-page-requests-metric-209 branch 3 times, most recently from 0c0d760 to 1ab789e Compare March 30, 2017 18:44

jonathanKingston reviewed Mar 30, 2017

View reviewed changes

fix #209: page-requests-completed-per-tab metric

5e94941

groovecoder force-pushed the add-page-requests-metric-209 branch from 1ab789e to 5e94941 Compare March 30, 2017 19:40

jonathanKingston approved these changes Mar 30, 2017

View reviewed changes

groovecoder merged commit 9e1da08 into master Mar 30, 2017

groovecoder deleted the add-page-requests-metric-209 branch March 30, 2017 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP/DNM: Add page requests metric 209 #389

WIP/DNM: Add page requests metric 209 #389

groovecoder commented Mar 16, 2017

groovecoder Mar 16, 2017

groovecoder commented Mar 23, 2017

jonathanKingston Mar 30, 2017

groovecoder Mar 30, 2017

groovecoder Mar 30, 2017

jonathanKingston Mar 30, 2017

jonathanKingston Mar 30, 2017

jonathanKingston Mar 30, 2017

groovecoder Mar 30, 2017

groovecoder commented Mar 30, 2017

jonathanKingston Mar 30, 2017

groovecoder Mar 30, 2017

jonathanKingston Mar 30, 2017

groovecoder Mar 30, 2017

jonathanKingston Mar 30, 2017

groovecoder Mar 30, 2017

jonathanKingston Mar 30, 2017 •

edited

groovecoder Mar 30, 2017

WIP/DNM: Add page requests metric 209 #389

WIP/DNM: Add page requests metric 209 #389

Conversation

groovecoder commented Mar 16, 2017

Choose a reason for hiding this comment

groovecoder commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston Mar 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston Mar 30, 2017 •

edited