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)

All
Android
defect

Tracking

(firefox34 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox34 --- verified

People

(Reporter: Margaret, Assigned: eedens)

References

Details

Attachments

(1 file, 4 obsolete files)

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)
(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)
Priority: -- → P1
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
Attached patch bug-1042956-wip.patch (obsolete) — Splinter Review
Attachment #8465074 - Flags: feedback?
Attached patch bug-1042956-wip.v2.patch (obsolete) — Splinter Review
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)
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+
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)
(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>.
(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
(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)
Attached patch bug-1042956-fix.v1.patch (obsolete) — Splinter Review
- 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 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+
Flags: needinfo?(mark.finkle)
(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.
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+
Attached patch bug-1042956-fix.v2.patch (obsolete) — Splinter Review
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)
(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.
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+
Changed "search_history" to "search-history".

Setting more info to margaret for landing.
Attachment #8465685 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
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)
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
(I forget to run the bug marking tool until today)
Verified as fixed in build 34.0a1 (2014-08-07);
Device: Samsung Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
Depends on: 1052731
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.