1089670 - Record searches in Telemetry

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We want to record searches in Telemetry, not only FHR.

Comment 1

[Georg Fritzsche [:gfritzsche]]

This seems to give us all the relevant locations:
http://dxr.mozilla.org/mozilla-central/search?q=org.mozilla.searches&case=true

bwinton, gps:
Should we be covered by inserting ourselves into these locations or am i missing something?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I don't know if those are all the places, but I do know that there's already some code in the tree to record searches in Telemetry:
http://dxr.mozilla.org/mozilla-central/search?q=countSearchEvent&case=true&redirect=true
so it looks like those are the correct places…

Would it make sense to move that out of the UITelemetry blob and into a more structured format?

(Adding Ilana and Cheng who are using that data to make heatmaps and stuff…)

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Blake Winton (:bwinton) from comment #2)
> Would it make sense to move that out of the UITelemetry blob and into a more
> structured format?

Hm, the requirements seem to be rather different (count all actions and don't care what engine etc.).
I would like to keep this addition pretty simple, so maybe we could revisit this for refactoring later.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

I think we can get from what we have to what you're looking for, but I'm also happy to add the extra probes here, and look into refactoring later.  :)

Would it be easier to just add the new probe in the countSearchEvent function, since that's already getting called from the correct places?

Comment 5

[Gregory Szorc [:gps]]

Yes, I think inserting in those locations is fine.

Depending on the requirements, you might be able to get away with inserting at a single location: inside the search service itself. It should ideally be done this way. IIRC FHR didn't do it because it would require rework of some of the APIs to proxy more metadata and that was considered scope bloat. As a result, I believe we missed at least 1 origin for performing searches. And, if searches are initiated via e.g. an extension, we probably miss those too. This is how technical debt is born :/

Comment 6

[:Gijs (he/him)]

(In reply to Gregory Szorc [:gps] from comment #5)
> Yes, I think inserting in those locations is fine.
> 
> Depending on the requirements, you might be able to get away with inserting
> at a single location: inside the search service itself. It should ideally be
> done this way. IIRC FHR didn't do it because it would require rework of some
> of the APIs to proxy more metadata and that was considered scope bloat. As a
> result, I believe we missed at least 1 origin for performing searches. And,
> if searches are initiated via e.g. an extension, we probably miss those too.
> This is how technical debt is born :/

There is a different problem with this idea, though, which is that not all search URLs requested via getSubmission will be navigated to...

Comment 7

[Georg Fritzsche [:gfritzsche]]

Bug 1069874 looks good now, so i'll be on this tomorrow - unless someone wants to / can get here today.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Record searches in Telemetry

bsmedberg, SEARCH_DEFAULT_ENGINE is a little awkward as i did it. Any better suggestions? Telemetry.log()

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8515076 [details] [diff] [review]
Record searches in Telemetry

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

So, on the one hand, I think that in the interest of not duplicating data, we should remove the BrowserUITelemetry code that counts the same searches (we should still leave the code that counts urlbar-keyword and selection searches, though).  On the other hand, cross-referencing the urlbar-keyword searches with urlbar searches seems like something we want to do, and seems easier if both pieces of data are in the same place, so perhaps we shouldn't delete the BrowserUITelemetry code.

Overall, I think it's good, though, so I'm going to say "r=me" with the things I mention below fixed (or explained).

::: browser/base/content/browser.js
@@ +415,5 @@
> +    let name;
> +
> +    if (!engine) {
> +      name = "NONE";
> +    } else if (engine.identifier) {

Why are we preferring the identifier to the name?  (The comment in the IDL for nsISearchEngine.name says "The display name of the search engine. This is a unique identifier.")

(Ditto in the code below.)

@@ +3286,5 @@
>    },
> +
> +  _getSearchEngineId: function (engine) {
> +    if (!engine) {
> +      return other;

Did you mean:
    return "other";
here?

@@ +3303,5 @@
> +    const RECORDED_SEARCHES = [
> +      "amazon.com.abouthome",
> +      "amazon.com.contextmenu",
> +      "amazon.com.searchbar",
> +      "amazon.com.urlbar",

We don't record newtab searches?
I think I'ld prefer to see this written as:

const ENGINES = [
  "amazon",
  "bing",
  "google",
  "yahoo",
  "other,
];

const SOURCES = [
  "abouthome",
  "contextmenu",
  "newtab",
  "searchbar",
  "urlbar",
];

const RECORDED_SEARCHES = [
  for (engine of ENGINES)
    for (source of SOURCES)
      engine + "." + source
];

::: browser/components/nsBrowserGlue.js
@@ +358,5 @@
>          BrowserUITelemetry.countSearchEvent("urlbar", win.gURLBar.value);
> +
> +        let engine = null;
> +        try {
> +          let engine = subject.QueryInterface(Ci.nsISearchEngine);

I don't think we want the extra "let" here…

::: toolkit/components/telemetry/Histograms.json
@@ +4503,5 @@
> +    "expires_in_version": "never",
> +    "kind": "flag",
> +    "keyed": true,
> +    "description": "Record the default search engine."
> +  },

I'm not sure about these parts, so hopefully bsmedberg will weigh in.  :)

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Blake Winton (:bwinton) from comment #9)
> So, on the one hand, I think that in the interest of not duplicating data,
> we should remove the BrowserUITelemetry code that counts the same searches
> (we should still leave the code that counts urlbar-keyword and selection
> searches, though).  On the other hand, cross-referencing the urlbar-keyword
> searches with urlbar searches seems like something we want to do, and seems
> easier if both pieces of data are in the same place, so perhaps we shouldn't
> delete the BrowserUITelemetry code.

I think a refactoring here is a good thing, but better for another bug in respect to uplift and short time-frames.

> ::: browser/base/content/browser.js
> @@ +415,5 @@
> > +    let name;
> > +
> > +    if (!engine) {
> > +      name = "NONE";
> > +    } else if (engine.identifier) {
> 
> Why are we preferring the identifier to the name?  (The comment in the IDL
> for nsISearchEngine.name says "The display name of the search engine. This
> is a unique identifier.")

At this point i'm just mirroring the "org.mozilla.searches" provider in FHR directly (for "proven approach" but also for possibly comparing data).

> @@ +3286,5 @@
> >    },
> > +
> > +  _getSearchEngineId: function (engine) {
> > +    if (!engine) {
> > +      return other;
> 
> Did you mean:
>     return "other";
> here?
> 
> @@ +3303,5 @@
> > +    const RECORDED_SEARCHES = [
> > +      "amazon.com.abouthome",
> > +      "amazon.com.contextmenu",
> > +      "amazon.com.searchbar",
> > +      "amazon.com.urlbar",
> 
> We don't record newtab searches?

Hm, good point - looks like we have to file a bug on the FHR provider there.

> I think I'ld prefer to see this written as:
> 
> const ENGINES = [
>   "amazon",
>   "bing",
>   "google",
>   "yahoo",
>   "other,
> ];
> 
> const SOURCES = [
>   "abouthome",
>   "contextmenu",
>   "newtab",
>   "searchbar",
>   "urlbar",
> ];
> 
> const RECORDED_SEARCHES = [
>   for (engine of ENGINES)
>     for (source of SOURCES)
>       engine + "." + source
> ];

Alright, seems reasonable and this shouldn't be a "hot" path perf-wise.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> > Why are we preferring the identifier to the name?  (The comment in the IDL
> > for nsISearchEngine.name says "The display name of the search engine. This
> > is a unique identifier.")
> 
> At this point i'm just mirroring the "org.mozilla.searches" provider in FHR
> directly (for "proven approach" but also for possibly comparing data).

Ah, providers.jsm has a background comment here by the way:
> * We don't use the search engine name directly, because it is shared across
> * locales; e.g., eBay-de and eBay both share the name "eBay".

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Record searches in Telemetry

bsmedberg, SEARCH_DEFAULT_ENGINE is a little awkward as i did it. Any better suggestions? Telemetry.log()?

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Minor comment fix

Comment 14

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44660ec332db

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8515214 [details] [diff] [review]
Minor comment fix

Approval Request Comment
[Feature/regressing bug #]: Search telemetry.
[User impact if declined]:
[Describe test coverage new/current, TBPL]: Did manual test-coverage.
[Risks and why]: Lowish - conservative approach chosen and not much actual browser/ code touched.
[String/UUID change made/needed]: None.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Beta rebase - Report searches and default engine into Telemetry

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/44660ec332db

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8515214 [details] [diff] [review]
Minor comment fix

Aurora+

Moving Beta approval request to Beta rebase patch.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8515330 [details] [diff] [review]
Beta rebase - Report searches and default engine into Telemetry

Beta+

Comment 20

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f38280d613a5

Comment 21

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ca39da5df9d

Comment 22

[Georg Fritzsche [:gfritzsche]]

Re qe-verify:

We have different sources for searches: "abouthome", "contextmenu", "newtab", "searchbar", "urlbar".
Then we have different search engines you can set via the dropdown in the search bar.

In about:telemetry, search activity should be counted properly in the "keyed histograms" section under "SEARCH_COUNTS".
Official search engines should show up as e.g. "google.searchbar" etc., while searches added from some other sources should show up as e.g. "other-theSearchYouAdded.urlbar".

There is also "SEARCH_DEFAULT_ENGINE", which should have a flag for the name of your current default search engine. This is only submitted on idle daily, so you should trigger it updating by running the following in the browser console:
Services.obs.notifyObservers(null, "gather-telemetry", null)

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 34.0b8
Build Id:20141110195804
OS: Windows 7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5

I've noticed that the values from about:telemetry Keyed Histograms are removed after restarting Firefox. Is this expected behavior? Beside this behavior the searches are correctly recorded.

Comment 24

[Catalin Varga [QA][:VarCat]]

Verified as fixed also on:

FF Aurora 35
Build Id: 20141113004001

FF Nightly 36
Build Id: 20141113030201

OS: Windows 7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5

Comment 25

[Georg Fritzsche [:gfritzsche]]

(In reply to Catalin Varga [QA][:VarCat] from comment #23)
> I've noticed that the values from about:telemetry Keyed Histograms are
> removed after restarting Firefox. Is this expected behavior?

Yes, that is expected. That is also the same behaviour for normal histograms.