Closed
Bug 1042956
Opened 10 years ago
Closed 10 years ago
Add telemetry to search activity
Categories
(Firefox for Android Graveyard :: Search Activity, defect, P1)
Tracking
(firefox34 verified)
VERIFIED
FIXED
Firefox 34
Tracking | Status | |
---|---|---|
firefox34 | --- | verified |
People
(Reporter: Margaret, Assigned: eedens)
References
Details
Attachments
(1 file, 4 obsolete files)
14.58 KB,
patch
|
Details | Diff | Splinter Review |
At some point we had a list of things we wanted to measure, but I can't find it. dria, do you know where that went? To start, I think we should just build one simple probe that lets us work out the details of how to build telemetry into the search activity. Something like recording when a user taps on a search history suggestion seems like a good start.
Flags: needinfo?(deb)
Comment 1•10 years ago
|
||
I think it's here: https://mobile.etherpad.mozilla.org/search-activity-workweek-july2014 under "metrics" ?
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #1) > I think it's here: > https://mobile.etherpad.mozilla.org/search-activity-workweek-july2014 > > under "metrics" ? Ah! Thank you antlam! I moved that out into another easier-to-find etherpad: https://mobile.etherpad.mozilla.org/search-activity-telemetry
Flags: needinfo?(deb)
Reporter | ||
Updated•10 years ago
|
Priority: -- → P1
Reporter | ||
Comment 3•10 years ago
|
||
Eric is looking into this. We should use this bug to create a few very simple probes (maybe just create a session and record that a search happened), then we can file follow-up bugs to add more probes.
Assignee: margaret.leibovic → eric.edens
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8465074 -
Flags: feedback?
Assignee | ||
Comment 5•10 years ago
|
||
Here are a few UI telemetry probes to get us started. These should match what we talked about in IRC: - Start session (onStart of MainActivity) - End session (onStop of MainActivity) - Clearing search history - Performing search with suggestion - Performing search without suggestion - Clicking on a result from the search result page.
Attachment #8465074 -
Attachment is obsolete: true
Attachment #8465074 -
Flags: feedback?
Attachment #8465076 -
Flags: feedback?(mark.finkle)
Attachment #8465076 -
Flags: feedback?(margaret.leibovic)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8465076 [details] [diff] [review] bug-1042956-wip.v2.patch Review of attachment 8465076 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! I think we should also add another Event.SEARCH probe in PreSearchFragment, where the user clicks on a search history term. ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java @@ +100,4 @@ > view.stopLoading(); > Intent i = new Intent(Intent.ACTION_VIEW); > i.setData(Uri.parse(url)); > startActivity(i); This makes me think we should use a special intent to launch Fennec, since currently the user could choose to load this URL in a different browser. ::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java @@ +62,5 @@ > dialogBuilder.setNegativeButton(android.R.string.cancel, null); > dialogBuilder.setPositiveButton(android.R.string.ok, new DialogInterface.OnClickListener() { > @Override > public void onClick(DialogInterface dialog, int which) { > + Telemetry.sendUIEvent(TelemetryContract.Event.SANITIZE, TelemetryContract.Method.MENU, "history"); s/history/search_history ::: mobile/android/search/java/org/mozilla/search/autocomplete/ClearableEditText.java @@ +66,5 @@ > @Override > public boolean onEditorAction(TextView v, int actionId, KeyEvent event) { > + > + // The user searched without using search engine suggestions. > + Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.SUGGESTION, "user"); This happens when the user submits a query through the keyboard. I don't think that we would consider that a suggestion. Also, this should go inside the if statement where we're actually calling onSubmit.
Attachment #8465076 -
Flags: feedback?(margaret.leibovic) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
Thanks! Here are some points for clarification from you and mfinkle: > ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java > @@ +100,4 @@ > > view.stopLoading(); > > Intent i = new Intent(Intent.ACTION_VIEW); > > i.setData(Uri.parse(url)); > > startActivity(i); > > This makes me think we should use a special intent to launch Fennec, since > currently the user could choose to load this URL in a different browser. Since we hadn't specced the behavior, I made the conservative choice of allowing the user to decide which browser to use. Maybe open another bug to discuss? > ::: > mobile/android/search/java/org/mozilla/search/autocomplete/ClearableEditText. > java > @@ +66,5 @@ > > @Override > > public boolean onEditorAction(TextView v, int actionId, KeyEvent event) { > > + > > + // The user searched without using search engine suggestions. > > + Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.SUGGESTION, "user"); > > This happens when the user submits a query through the keyboard. I don't > think that we would consider that a suggestion. This nomenclature is from Fennec's SearchEngineRow.java, where "user" implies that a suggestion wasn't used [1]. Zooming out, though, we have three search types to track. Ideas for the params? Search bar, without suggestion: Event: Method: Extras: Search bar, with suggestion: Event: Method: Extras: History item, user tap: Event: Method: Extras: [1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SearchEngineRow.java#144
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
Comment 8•10 years ago
|
||
(In reply to :Margaret Leibovic from comment #6) > This makes me think we should use a special intent to launch Fennec, since > currently the user could choose to load this URL in a different browser. Just specify the class in the intent. Take a look at the constructors and methods for Intent. E.g., intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS_NAME); Sync has an out-of-tree helper for this; see <mobile/android/base/sync/setup/activities/ActivityUtils.java>.
Comment 9•10 years ago
|
||
(In reply to Eric Edens [:eedens] from comment #7) > Search bar, without suggestion: > Search bar, with suggestion: > History item, user tap: The terminology we use in Fennec: sources and engines. Sources are currently bartext, barsuggest, barkeyword (infrequently used). Congruence will help here, so it sounds like you want "text", "suggest", "history" as your sources. In the future these will also end up in FHR, as "activitytext", "activitysuggest", "activityhistory". (Fennec will eventually accrue its own search history picker, I imagine.)
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #8) > (In reply to :Margaret Leibovic from comment #6) > > > This makes me think we should use a special intent to launch Fennec, since > > currently the user could choose to load this URL in a different browser. > > Just specify the class in the intent. Take a look at the constructors and > methods for Intent. E.g., > > intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME, > AppConstants.BROWSER_INTENT_CLASS_NAME); > > Sync has an out-of-tree helper for this; see > <mobile/android/base/sync/setup/activities/ActivityUtils.java>. I agree with what eedens said in comment 7, I'll open another bug for us to deal with this issue :)
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 11•10 years ago
|
||
- Addressed items from Margaret's first feedback - Adjusted extras to match rnewman's feedback
Attachment #8465076 -
Attachment is obsolete: true
Attachment #8465076 -
Flags: feedback?(mark.finkle)
Attachment #8465614 -
Flags: review?(margaret.leibovic)
Comment 12•10 years ago
|
||
Comment on attachment 8465076 [details] [diff] [review] bug-1042956-wip.v2.patch >diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java >+ // Action taken from a search engine results page. >+ RESULTS_PAGE("resultspage"), This is the only thing I am worried about. It seems a bit too specific. >diff --git a/mobile/android/search/java/org/mozilla/search/MainActivity.java b/mobile/android/search/java/org/mozilla/search/MainActivity.java >diff --git a/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java b/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java >+ Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.RESULTS_PAGE); Based on the usage here, I think we could make the method be: CONTENT("content") since it's a loadurl coming from a content page. We could use the method in Fennec too, in the future. You could also add an Extra here "search" or "results" or "search-result" would be fine with me. >diff --git a/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java b/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java >+ Telemetry.sendUIEvent(TelemetryContract.Event.SANITIZE, TelemetryContract.Method.MENU, "history"); "history" -> "search-history" ? Yes, we'll be in the "searchactivity.1" session, but I wouldn't mind the additional context in the Extra Thoughts?
Attachment #8465076 -
Flags: feedback+
Updated•10 years ago
|
Flags: needinfo?(mark.finkle)
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #12) > Comment on attachment 8465076 [details] [diff] [review] > bug-1042956-wip.v2.patch > > >diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java > > >+ // Action taken from a search engine results page. > >+ RESULTS_PAGE("resultspage"), > > This is the only thing I am worried about. It seems a bit too specific. > > >diff --git a/mobile/android/search/java/org/mozilla/search/MainActivity.java b/mobile/android/search/java/org/mozilla/search/MainActivity.java > >diff --git a/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java b/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java > > >+ Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.RESULTS_PAGE); > > Based on the usage here, I think we could make the method be: > CONTENT("content") since it's a loadurl coming from a content page. We could > use the method in Fennec too, in the future. You could also add an Extra > here "search" or "results" or "search-result" would be fine with me. I like the CONTENT suggestion. > >diff --git a/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java b/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java > > >+ Telemetry.sendUIEvent(TelemetryContract.Event.SANITIZE, TelemetryContract.Method.MENU, "history"); > > "history" -> "search-history" ? > Yes, we'll be in the "searchactivity.1" session, but I wouldn't mind the > additional context in the Extra I think you were commenting on an old version of the patch. That's fixed in the most recent version.
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8465614 [details] [diff] [review] bug-1042956-fix.v1.patch Review of attachment 8465614 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. r+ with RESULTS_PAGE probe renamed to CONTENT instead. ::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java @@ +153,5 @@ > final Rect startBounds = new Rect(); > view.getGlobalVisibleRect(startBounds); > > + // The user tapped on a suggestion from the search engine. > + Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.SUGGESTION, "suggest"); Not sure we need the "suggest" extra if we have the SUGGESTION method.
Attachment #8465614 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Changed RESULTS_PAGE to CONTENT. About the "suggest" extra -- the other telemetry logs for SEARCH use extras, so my thought was it would make it easier for analysis if the mapreduce only has to split on the extra param. Associated PR: https://github.com/ericedens/FirefoxSearch/pull/45/files
Attachment #8465614 -
Attachment is obsolete: true
Attachment #8465685 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 16•10 years ago
|
||
(In reply to Eric Edens [:eedens] from comment #15) > Created attachment 8465685 [details] [diff] [review] > bug-1042956-fix.v2.patch > > Changed RESULTS_PAGE to CONTENT. > > About the "suggest" extra -- the other telemetry logs for SEARCH use extras, > so my thought was it would make it easier for analysis if the mapreduce only > has to split on the extra param. I haven't looked into how the analysis jobs work, but the SEARCH probes you have already all use different methods. But I guess it's fine to keep the extra in there in case there is some other kind of suggestion added in the future.
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8465685 [details] [diff] [review] bug-1042956-fix.v2.patch Review of attachment 8465685 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java @@ +62,5 @@ > dialogBuilder.setNegativeButton(android.R.string.cancel, null); > dialogBuilder.setPositiveButton(android.R.string.ok, new DialogInterface.OnClickListener() { > @Override > public void onClick(DialogInterface dialog, int which) { > + Telemetry.sendUIEvent(TelemetryContract.Event.SANITIZE, TelemetryContract.Method.MENU, "search_history"); Let's update this to "search-history" to be consistent with the "search-result" extra.
Attachment #8465685 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Changed "search_history" to "search-history". Setting more info to margaret for landing.
Attachment #8465685 -
Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0a1d7175bd6f I had to update the commit message (it accidentally had a message for a different bug), so make sure it's correct when merging the PR (I'll let you handle that).
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 20•10 years ago
|
||
https://github.com/ericedens/FirefoxSearch/commit/6ac340eafb20a0e563b8e5b212502592b5e37b2a
Reporter | ||
Comment 21•10 years ago
|
||
Somehow this changeset didn'g get linked: http://hg.mozilla.org/mozilla-central/rev/0a1d7175bd6f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 23•10 years ago
|
||
(I forget to run the bug marking tool until today)
Comment 24•10 years ago
|
||
Verified as fixed in build 34.0a1 (2014-08-07); Device: Samsung Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
status-firefox34:
--- → affected
Updated•10 years ago
|
Updated•6 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•