1304446 - Move |recordSearchInTelemetry| to BrowserUsageTelemetry.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Alessio Placitelli [:Dexter]]

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1304446.patch

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 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1304446.patch

Comment 6

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 7

[Alessio Placitelli [:Dexter]]

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

Comment 8

[Marco Bonardo [:mak]]

(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.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: bug1304446.patch

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

Comment 10

[Alessio Placitelli [:Dexter]]

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: bug1304446.patch

Comment 13

[Alessio Placitelli [:Dexter]]

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+?

Comment 14

[Alessio Placitelli [:Dexter]]

(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.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(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)?

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Marco Bonardo [:mak]]

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
}

Comment 18

[Alessio Placitelli [:Dexter]]

(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.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: bug1304446.patch

Thanks Marco.

Comment 20

[Alessio Placitelli [:Dexter]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67f3ef959db7

Comment 22

[Alessio Placitelli [:Dexter]]

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 23

[Gerry Chang [:gchang]]

Comment on attachment 8798818 [details] [diff] [review]
bug1304446.patch

This patch helps get data for search. Take it in 51 aurora.

Comment 24

[Alessio Placitelli [:Dexter]]

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