Closed
Bug 1089670
Opened 10 years ago
Closed 10 years ago
Record searches in Telemetry
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 36
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
Details
Attachments
(2 files, 2 obsolete files)
8.12 KB,
patch
|
gfritzsche
:
review+
benjamin
:
feedback+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
7.87 KB,
patch
|
gfritzsche
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We want to record searches in Telemetry, not only FHR.
Assignee | ||
Comment 1•10 years ago
|
||
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?
Flags: needinfo?(gps)
Flags: needinfo?(bwinton)
Comment 2•10 years ago
|
||
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…)
Flags: needinfo?(bwinton)
Assignee | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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 :/
Updated•10 years ago
|
Flags: needinfo?(gps)
Comment 6•10 years ago
|
||
(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...
Assignee | ||
Comment 7•10 years ago
|
||
Bug 1069874 looks good now, so i'll be on this tomorrow - unless someone wants to / can get here today.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → georg.fritzsche
Assignee | ||
Comment 8•10 years ago
|
||
bsmedberg, SEARCH_DEFAULT_ENGINE is a little awkward as i did it. Any better suggestions? Telemetry.log()
Attachment #8515076 -
Flags: review?(bwinton)
Attachment #8515076 -
Flags: feedback?(benjamin)
Comment 9•10 years ago
|
||
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. :)
Attachment #8515076 -
Flags: review?(bwinton) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
(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".
Assignee | ||
Comment 12•10 years ago
|
||
bsmedberg, SEARCH_DEFAULT_ENGINE is a little awkward as i did it. Any better suggestions? Telemetry.log()?
Attachment #8515076 -
Attachment is obsolete: true
Attachment #8515076 -
Flags: feedback?(benjamin)
Attachment #8515183 -
Flags: review+
Attachment #8515183 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8515183 -
Attachment is obsolete: true
Attachment #8515183 -
Flags: feedback?(benjamin)
Attachment #8515214 -
Flags: review+
Attachment #8515214 -
Flags: feedback?(benjamin)
Updated•10 years ago
|
Attachment #8515214 -
Flags: feedback?(benjamin) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
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.
Attachment #8515214 -
Flags: approval-mozilla-beta?
Attachment #8515214 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify+
Flags: in-testsuite-
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8515330 -
Flags: review+
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 18•10 years ago
|
||
Comment on attachment 8515214 [details] [diff] [review]
Minor comment fix
Aurora+
Moving Beta approval request to Beta rebase patch.
Attachment #8515214 -
Flags: approval-mozilla-beta?
Attachment #8515214 -
Flags: approval-mozilla-aurora?
Attachment #8515214 -
Flags: approval-mozilla-aurora+
Comment 19•10 years ago
|
||
Comment on attachment 8515330 [details] [diff] [review]
Beta rebase - Report searches and default engine into Telemetry
Beta+
Attachment #8515330 -
Flags: approval-mozilla-beta+
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Updated•10 years ago
|
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Flags: needinfo?(georg.fritzsche)
You need to log in
before you can comment on or make changes to this bug.
Description
•