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

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?

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…)

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

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?

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 :/

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

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

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

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. :)

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

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

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

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 on attachment 8515214 [details] [diff] [review] Minor comment fix Aurora+ Moving Beta approval request to Beta rebase patch.

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

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)

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.

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

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