The default bug view has changed. See this FAQ.

Add telemetry to search activity

VERIFIED FIXED in Firefox 34

Status

()

Firefox for Android
Search Activity
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: eedens)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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)
I think it's here: https://mobile.etherpad.mozilla.org/search-activity-workweek-july2014

under "metrics" ?
(Reporter)

Comment 2

3 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

3 years ago
Priority: -- → P1
(Reporter)

Comment 3

3 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

3 years ago
Created attachment 8465074 [details] [diff] [review]
bug-1042956-wip.patch
Attachment #8465074 - Flags: feedback?
(Assignee)

Comment 5

3 years ago
Created attachment 8465076 [details] [diff] [review]
bug-1042956-wip.v2.patch

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

3 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

3 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)
(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
(Reporter)

Comment 10

3 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

3 years ago
Created attachment 8465614 [details] [diff] [review]
bug-1042956-fix.v1.patch

- 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)
(Reporter)

Comment 13

3 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

3 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

3 years ago
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. 

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

3 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

3 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

3 years ago
Created attachment 8465775 [details] [diff] [review]
bug-1042956-fix.v3.patch

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

3 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

3 years ago
https://github.com/ericedens/FirefoxSearch/commit/6ac340eafb20a0e563b8e5b212502592b5e37b2a
(Reporter)

Comment 21

3 years ago
Somehow this changeset didn'g get linked:
http://hg.mozilla.org/mozilla-central/rev/0a1d7175bd6f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
https://hg.mozilla.org/mozilla-central/rev/0a1d7175bd6f
(I forget to run the bug marking tool until today)

Comment 24

3 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

3 years ago
status-firefox34: affected → verified
(Reporter)

Updated

3 years ago
Depends on: 1052731
You need to log in before you can comment on or make changes to this bug.