Since |recordSearchInTelemetry| has mostly to do with Telemetry, we should think about moving it to BrowserUsageTelemetry.jsm. [1] - https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/browser/base/content/browser.js#3780

Marco, does it make sense to consolidate this Telemetry related functions in a different module, rather than keeping them in browser.js? We're building up BrowserUsageTelemetry.jsm, that could potentially be used as a central hub for this kind of functionalities.

yes, I think it makes sense. Anything we can move out of browser.js makes sense, especially when it's not related to a specific browser.

Since bug 1303333 will take advantage of this change, I moved on with this first. We don't want to really touch "BrowserUITelemetry" for now, as it might get killed in the future. However, we could do two things: 1) Just move the histogram accumulation code to BrowserUsageTelemetry, leaving the calls to BrowserUITelemetry around in browser.js (what I did). 2) Move the calls to BrowserUITelemetry from browser.js to BrowserUsageTelemetry.jsm, and let all the callers record the Telemetry through BrowserUsageTelemetry.

Comment on attachment 8796559 [details] [diff] [review] bug1304446.patch Review of attachment 8796559 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I think for now it's ok to leave BrowserUITelemetry probes as you did, mostly cause we have Shield Studies ongoing, or about to start, that may still use it (the Unified-Urlbar shield study we are building for example). so it's not a good idea to change its API. Long term, we need a plan to deprecate those probes, so for each, notify the intent to remove to fx-team, if needed build a better alternative in toolkit telemetry and then remove the probe. That requires a dedicated plan. Btw, nothing blocking here, so r+. ::: browser/modules/BrowserUsageTelemetry.jsm @@ +62,5 @@ > + > + if (!engine || (engine.name === undefined) || !Services.telemetry.canRecordExtended) > + return "other"; > + > + return "other-" + engine.name; it may be simpler like this: if (!engine || (!engine.identifier && !engine.name) || !Services.telemetry.canRecordExtended) { return "other"; } return engine.identifier || "other-" + engine.name; @@ +213,5 @@ > + * The main entry point for recording search related Telemetry. This includes > + * search counts and engagement measurements. > + * > + * Telemetry records only search counts and nothing pertaining to the search > + * itself. I think it makes sense to update this to "...search counts per engine and action origin, but nothing pertaining to the search contents themselves.". It should be clearer. @@ +219,5 @@ > + * @param engine > + * (nsISearchEngine) The engine handling the search. > + * @param source > + * (string) Where the search originated from. See the FHR > + * SearchesProvider for allowed values. AFAIK, FHR is deprecated. This should instead point to the KNOWN_SEARCH_SOURCES const in this same file. @@ +223,5 @@ > + * SearchesProvider for allowed values. > + * @param selection [optional] > + * ({index: The selected index, kind: "key" or "mouse"}) If > + * the search was a suggested search, this indicates where the > + * item was in the suggestion list and how the user selected it. Since this is unused I'm not sure how it will be used and it's hard to review it. Maybe we should leave this out for now and add it when we will actually make use of it. For example it could end up being something more like optionalData containing things that have nothing to do with selection... @@ +225,5 @@ > + * ({index: The selected index, kind: "key" or "mouse"}) If > + * the search was a suggested search, this indicates where the > + * item was in the suggestion list and how the user selected it. > + */ > + recordSearchTelemetry(engine, source, selection) { since the module name contains already the Telemetry word, maybe this could just be BrowserUsageTelemetry.recordSearch @@ +226,5 @@ > + * the search was a suggested search, this indicates where the > + * item was in the suggestion list and how the user selected it. > + */ > + recordSearchTelemetry(engine, source, selection) { > + if (KNOWN_SEARCH_SOURCES.indexOf(source) == -1) { if (!KNOWN_SEARCH_SOURCES.includes(source)) { @@ +227,5 @@ > + * item was in the suggestion list and how the user selected it. > + */ > + recordSearchTelemetry(engine, source, selection) { > + if (KNOWN_SEARCH_SOURCES.indexOf(source) == -1) { > + Cu.reportError("Unknown source for search: " + source); I honestly think this should throw, and browser.js should catch(ex) { reportError } Since we are making a new API, let's make it proper. Passing an unknown source is a developer mistake, imo.

(In reply to Marco Bonardo [::mak] from comment #4) > Long term, we need a plan to deprecate those probes, so for each, notify the > intent to remove to fx-team, if needed build a better alternative in toolkit > telemetry and then remove the probe. That requires a dedicated plan. Unrelated to this bug, but the longer term plan is roughly: (1) build out Telemetry Events as a better alternative (2) once that ships, call out UITelemetry as deprecated (3) migrate existing prioritized UITelemetry usage (4) remove UITelemetry We are currently at (1) for that project.

(In reply to Georg Fritzsche [:gfritzsche] from comment #6) > Unrelated to this bug, but the longer term plan is roughly: > ... Makes sense, thank you for the update.

Ok, I had to restore the use of |Services.prefs..| instead of using Services.telemetry.canRecordExtended due to bug 1222070, as tests were failing. Everything is green now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d656b4821777aeae8544c1cd78e28e727564b4b0

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c63072cba76a5e6da1bdd7e37a455030890bea Bug 1304446 - Move |recordSearchInTelemetry| to BrowserUsageTelemetry.jsm. r=mak

Backed out for browser-chrome bustages, e.g. browser_aboutHome.js | histogram with key should be recorded - false == true: https://hg.mozilla.org/integration/mozilla-inbound/rev/795c761956d32e3bbdd8672454ca6592fb534134 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=82c63072cba76a5e6da1bdd7e37a455030890bea Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37109829&repo=mozilla-inbound [task 2016-10-05T17:37:48.121556Z] 17:37:48 INFO - 27 INFO Entering test bound [task 2016-10-05T17:37:48.125667Z] 17:37:48 INFO - 28 INFO Check that performing a search fires a search event and records to Telemetry. [task 2016-10-05T17:37:48.127310Z] 17:37:48 INFO - 29 INFO Waiting for engine to be added: searchSuggestionEngine.xml [task 2016-10-05T17:37:48.128883Z] 17:37:48 INFO - 30 INFO Search engine added: searchSuggestionEngine.xml [task 2016-10-05T17:37:48.130625Z] 17:37:48 INFO - 31 INFO Console message: Security Error: Content at about:home may not load or link to chrome://mozapps/skin/places/defaultFavicon.png. [task 2016-10-05T17:37:48.132831Z] 17:37:48 INFO - 32 INFO TEST-PASS | browser/base/content/test/general/browser_aboutHome.js | Engine name in DOM should match engine we just added - "browser_searchSuggestionEngine searchSuggestionEngine.xml" == "browser_searchSuggestionEngine searchSuggestionEngine.xml" - [task 2016-10-05T17:37:48.137088Z] 17:37:48 INFO - 33 INFO waitForDocLoadAndStopIt: Waiting for URL: http://mochi.test:8888/ [task 2016-10-05T17:37:48.139085Z] 17:37:48 INFO - 34 INFO Console message: Security Error: Content at about:home may not load or link to chrome://mozapps/skin/places/defaultFavicon.png. [task 2016-10-05T17:37:48.142806Z] 17:37:48 INFO - 35 INFO Perform a search. [task 2016-10-05T17:37:48.144522Z] 17:37:48 INFO - 36 INFO TEST-PASS | browser/base/content/test/general/browser_aboutHome.js | waitForDocLoadAndStopIt: The expected URL was loaded - [task 2016-10-05T17:37:48.146563Z] 17:37:48 INFO - 37 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutHome.js | histogram with key should be recorded - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js :: <TOP_LEVEL> :: line 119 [task 2016-10-05T17:37:48.147913Z] 17:37:48 INFO - Stack trace: [task 2016-10-05T17:37:48.150939Z] 17:37:48 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js:null:119 [task 2016-10-05T17:37:48.152956Z] 17:37:48 INFO - @chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js:73:9 [task 2016-10-05T17:37:48.154803Z] 17:37:48 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:322:42 [task 2016-10-05T17:37:48.160529Z] 17:37:48 INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3 [task 2016-10-05T17:37:48.162274Z] 17:37:48 INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14 [task 2016-10-05T17:37:48.163859Z] 17:37:48 INFO - Task_spawn@resource://gre/modules/Task.jsm:168:12 [task 2016-10-05T17:37:48.166883Z] 17:37:48 INFO - TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:392:16 [task 2016-10-05T17:37:48.168523Z] 17:37:48 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:330:15 [task 2016-10-05T17:37:48.170208Z] 17:37:48 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 [task 2016-10-05T17:37:48.172178Z] 17:37:48 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 [task 2016-10-05T17:37:48.180083Z] 17:37:48 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 [task 2016-10-05T17:37:48.181936Z] 17:37:48 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 [task 2016-10-05T17:37:48.184144Z] 17:37:48 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 [task 2016-10-05T17:37:48.186238Z] 17:37:48 INFO - Async*this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:705:7 [task 2016-10-05T17:37:48.189474Z] 17:37:48 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:327:15 [task 2016-10-05T17:37:48.191433Z] 17:37:48 INFO - Async*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:399:7 [task 2016-10-05T17:37:48.193936Z] 17:37:48 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:330:15 [task 2016-10-05T17:37:48.196138Z] 17:37:48 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 [task 2016-10-05T17:37:48.203007Z] 17:37:48 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 [task 2016-10-05T17:37:48.204776Z] 17:37:48 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 [task 2016-10-05T17:37:48.206494Z] 17:37:48 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 [task 2016-10-05T17:37:48.208181Z] 17:37:48 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 [task 2016-10-05T17:37:48.209742Z] 17:37:48 INFO - receiveMessage@resource://testing-common/ContentTask.jsm:113:9 [task 2016-10-05T17:37:48.212445Z] 17:37:48 INFO - ************************* [task 2016-10-05T17:37:48.214173Z] 17:37:48 INFO - A coding exception was thrown and uncaught in a Task. [task 2016-10-05T17:37:48.216190Z] 17:37:48 INFO - Full message: TypeError: hs[histogramKey] is undefined [task 2016-10-05T17:37:48.218359Z] 17:37:48 INFO - Full stack: @chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js:120:5 [task 2016-10-05T17:37:48.221269Z] 17:37:48 INFO - @chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js:73:9 [task 2016-10-05T17:37:48.223276Z] 17:37:48 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:322:42 [task 2016-10-05T17:37:48.226487Z] 17:37:48 INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3

Ok, traced down the issue: it looks like the refactoring of |getSearchEngineId| was not bulletproof. For some reason (I guess different Telemetry settings) it was only triggering a failure on Linux ASAN, which I didn't get on try. I rewrote the |getSearchEngineId| function a bit with this updated patch. Marco, does it still look sane to you, can I carry over the r+?

(In reply to Alessio Placitelli [:Dexter] from comment #13) > Ok, traced down the issue: it looks like the refactoring of > |getSearchEngineId| was not bulletproof. I forgot to mention that it was failing with extended telemetry disabled, when both the engine identifier and name were defined. It was returning "other" rather than the engine identifier.

(In reply to Alessio Placitelli [:Dexter] from comment #14) > (In reply to Alessio Placitelli [:Dexter] from comment #13) > > Ok, traced down the issue: it looks like the refactoring of > > |getSearchEngineId| was not bulletproof. > > I forgot to mention that it was failing with extended telemetry disabled, > when both the engine identifier and name were defined. It was returning > "other" rather than the engine identifier. Does this mean we have missing explicit test coverage for this case? Can we add it (here or in a future bug)?

yes it looks good! sorry, I overlooked that we wanted to report engine identifiers even if extended telemetry was disabled.

jsut one thing + if (engine.identifier) { + return engine.identifier; + } else if (engine.name && extendedTelemetry) { no else after return, it can just be if () { return } if () { return }

(In reply to Georg Fritzsche [:gfritzsche] from comment #15) > (In reply to Alessio Placitelli [:Dexter] from comment #14) > > (In reply to Alessio Placitelli [:Dexter] from comment #13) > > > Ok, traced down the issue: it looks like the refactoring of > > > |getSearchEngineId| was not bulletproof. > > > > I forgot to mention that it was failing with extended telemetry disabled, > > when both the engine identifier and name were defined. It was returning > > "other" rather than the engine identifier. > > Does this mean we have missing explicit test coverage for this case? > Can we add it (here or in a future bug)? It's lacking explicit test coverage for this case, AFAICS. Filed bug 1308452 for that.

Thanks Marco.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67f3ef959db7cb877a870af4746f1af25e6cb1ee Bug 1304446 - Move |recordSearchInTelemetry| to BrowserUsageTelemetry.jsm. r=mak

Comment on attachment 8798818 [details] [diff] [review] bug1304446.patch Approval Request Comment [Feature/regressing bug #]: This patch is a base for the implementation of bug 1303333. [User impact if declined]: We won't be able to land bug 1303333. [Describe test coverage new/current, TreeHerder]: There's existing test coverage for all the points this touches already in place on m-c. This has lived for a while on m-c with no issue. [Risks and why]: Low, for the reasons above. [String/UUID change made/needed]: None

Comment on attachment 8798818 [details] [diff] [review] bug1304446.patch This patch helps get data for search. Take it in 51 aurora.

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