1261617 - Add UI Telemetry probes for search.1 actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We should fire search.1 events when doing searches from the various methods we allow. Currently, searches are co mingled with other URL loads in loadurl.1 events. We should keep the loadurl.1 events since they _are_ loading URLs, but we should add search.1 events for searches so we can study them explicitly.

Yes, these are opt-in probes and can't be used as a source for real search counts. The core ping will handle that.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: search-telemetry v0.1

The main purpose of this event is to track the searches that happen in our various search UI + send the name of the search engine used to perform the search.

Not sure who'd be best for the Search places. I mimic what FHR used to do except I don't track "keyword" searches as searches, since we don't have the name of the engine.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8744466 [details] [diff] [review]
search-telemetry v0.1

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

I'm worried I may be missing places where we're doing searches, I want to dig up how we used to do this with FHR.

::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java
@@ +116,5 @@
>                      } else {
>                          Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, (String) v.getTag());
>                      }
> +                    String identifier = (mSearchEngine == null) ? "other" : mSearchEngine.getEngineIdentifier();
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.SUGGESTION, identifier);

Do we also need to add a probe as well down below in performUserEnteredSearch? Would it be better to just do this in the onSearch handler?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3978

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8744466 [details] [diff] [review]
search-telemetry v0.1

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

To get context on what FHR does, I checked out b7d00293390b [1], which is the commit before FHR was removed.

FHR appears to have used the BrowserApp.recordSearch [2] method to record its searches. Using the call hierarchy tool, it is called in the following places:
 1) loadUrlOrKeywordSearch
 BrowserApp.onSearch (called by)
   2) BrowserSearch.onSearchBarClickListener
   3) SearchEngineRow.performUserEnteredSearch
   4) SearchEngineRow.onClick

These occur for:
 1) keyword bookmark searches
 2) When the search engine bar at the bottom of browser search is clicked
 3) User, w/o suggestions enabled, types into the url bar and taps a SearchEngineRow.
 4) User, presses search suggestion chiclet in BrowserSearch

Notes:
 * All search cases are covered except when the user types in the toolbar and hits enter to make a search via the default search engine. Unintuitively, "keyword:search" is passed back for this case so Finkle's patch addresses this case
 * 3 is broken on fx-team: tapping the SearchEngineRow takes no action (so you have to press a chiclet, which is 4). I filed bug 1269478.
 * Finkle's patch covers 1 (assuming the "Keyword:Search" message occurs for all keyword searches – is this what he meant in comment 1?), 2, and 4. Depending on what we do with 3, this patch is equivalent to FHR and, if we decide we don't care about 1, should be sufficient.

That being said, I like how FHR (mostly x_x [3]) funneled all search events through a common code path – BrowserApp.onSearch (via search listeners). These listeners are still in place and I think we should re-use them here – we can pass in the Telemetry event there. fwiw, storeSearchQuery also uses these entry points.

To summarize: we should record these searches where storeSearchQuery is called from and pass in the appropriate Telemetry values.

I'm going to have to do a similar change for my java telemetry patch – should I just take care of this one too?

[1]: https://hg.mozilla.org/mozilla-central/file/b7d00293390b/mobile/android/base/java/org/mozilla/gecko
[2]: https://hg.mozilla.org/mozilla-central/file/b7d00293390b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#l2342

[3]: BrowserApp.onSearch both records search information and actually opens the search, which isn't always want we want. Ideally, we could separate the concerns – a search listener and a method to actually search – so it's clearer where we we need to add these types of probes in the future (in the search listener).

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java
@@ +728,5 @@
>      @Override
>      public void onSearchBarClickListener(final SearchEngine searchEngine) {
> +        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM, "searchenginebar");
> +
> +        String identifier = (searchEngine == null) ? "other" : searchEngine.getEngineIdentifier();

nit: final

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Just one correction (?), with regard to BrowserApp.onSearch - That is not the place where all searches are funneled. Only those that originate in the BrowserSearch UI end up there.

For example, when typing a few words "finkle is wrong" and pressing ENTER, we go to browser.js and the DocShell ultimately knows that a default search happened. It fires a notification which is resent to Java as "Search:Keyword" and was originally handled by the FHR recorder [1], which is gone now. Since we also caught the message in BrowserApp, I just handle the search there now.

We also need to try not to confuse "keywords" and "keywords". I know. In one case, "Search:Keyword" is legacy naming from Gecko docShell and is really a "search with the default search engine because the text is not a URL". On the other hand, "keywords" can also mean a special type of Bookmark, like when you add DXR search field as a bookmark and keyword it as 'dxr'. Then, when you type "dxr onSearch" we catch this in BrowserSearch and convert the string to the DXR search string. 

My patch considers the former a "search", but does not consider the latter a "search" because it does not use a First Class Search Engine. Therefore, we do not know the name of the engine and would only report null.

Just more food for thought.

[1] https://hg.mozilla.org/mozilla-central/file/b7d00293390b/mobile/android/base/java/org/mozilla/gecko/health/BrowserHealthRecorder.java#l710

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Assuming I'm taking this over with my similar work in bug 1253319.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: MozReview Request: Bug 1261617 - Add UITelemetry for search counts. r=mfinkle

I largerly kept this patch the same as Finkle's initial revision except that
I don't include the engine identifiers due to bug 1272166.

Review commit: https://reviewboard.mozilla.org/r/52063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52063/

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

r? -> redirect to someone else

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8751522 [details]
MozReview Request: Bug 1261617 - Add UITelemetry for search counts. r=mfinkle

https://reviewboard.mozilla.org/r/52063/#review49752

Comment 9

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/f15a0c2d0178fc4396ef16f3a54a0f95e82e31ad
Bug 1261617 - Add UITelemetry for search counts. r=mfinkle

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f15a0c2d0178