Closed Bug 1055315 Opened 10 years ago Closed 10 years ago

Add telemetry probe to measure when user changes search engine in search activity

Categories

(Firefox for Android Graveyard :: Search Activity, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: Margaret, Assigned: ramasamy.zephyr, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 1 obsolete file)

As a follow-up to bug 1042943, we should add telemetry to know when a user changes their default search engine.
Hi Margaret, Thanks for mentoring. 

I figured http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/SearchPreferenceCategory.java#52 would be a good place to add telemetry event.

For the telemetry part should I be using this Telemetry.sendUIEvent() with event being PANEL_SET_DEFAULT?
LIST_ITEM as method.
Thanks for expressing interest! That file you pointed at is a good guess, but this bug is actually about the search activity, which has its own set of preferences. The file you want is:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java

However, we develop our search activity in a separate github repo from mozilla-central, then use that to generate patches for mozilla-central, so you'll want to clone this repo and make changes there:
https://github.com/mozilla/fennec-search/blob/master/app/src/main/java/org/mozilla/search/SearchPreferenceActivity.java

This post to mobile-firefox-dev has some more details about our workflow for generating a patch/pull request:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-August/000844.html

With regards to the telemetry probe, I would do something similar to what we did in bug 1007523, but use a slightly different probe to make sure that we can differentiate the data.

I think we should use Event.SEARCH_SET_DEFAULT, and then perhaps Method.LIST_ITEM to make this distinct from the probe for the Fennec search settings.

mfinkle, what do you think about this for a probe? I feel like Method.DIALOG would also make sense in the search activity settings, but I think we need a way to keep the search activity data separate. Maybe we actually need a new event?
Flags: needinfo?(mark.finkle)
(In reply to :Margaret Leibovic from comment #2)

> I think we should use Event.SEARCH_SET_DEFAULT, and then perhaps
> Method.LIST_ITEM to make this distinct from the probe for the Fennec search
> settings.
> 
> mfinkle, what do you think about this for a probe? I feel like Method.DIALOG
> would also make sense in the search activity settings, but I think we need a
> way to keep the search activity data separate. Maybe we actually need a new
> event?

Sounds like this should really be Method.SETTING, just like Fennec. We have the SEARCH session active, so we can tell which is happening in Fennec and which is happening in Search activity.
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> 
> > I think we should use Event.SEARCH_SET_DEFAULT, and then perhaps
> > Method.LIST_ITEM to make this distinct from the probe for the Fennec search
> > settings.
> > 
> > mfinkle, what do you think about this for a probe? I feel like Method.DIALOG
> > would also make sense in the search activity settings, but I think we need a
> > way to keep the search activity data separate. Maybe we actually need a new
> > event?
> 
> Sounds like this should really be Method.SETTING, just like Fennec. We have
> the SEARCH session active, so we can tell which is happening in Fennec and
> which is happening in Search activity.

Oh, good point, I forgot about the sessions. That solves our problem! So then, we should just add a probe just like the one in the Fennec settings.

ramasamy, let me know if you need help getting a build set up or if you don't understand what I've been talking about. You can also join #mobile on irc.mozilla.org to ask questions.
(In reply to Mark Finkle (:mfinkle) from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> 
> > I think we should use Event.SEARCH_SET_DEFAULT, and then perhaps
> > Method.LIST_ITEM to make this distinct from the probe for the Fennec search
> > settings.
> > 
> > mfinkle, what do you think about this for a probe? I feel like Method.DIALOG
> > would also make sense in the search activity settings, but I think we need a
> > way to keep the search activity data separate. Maybe we actually need a new
> > event?
> 
> Sounds like this should really be Method.SETTING, just like Fennec. We have
> the SEARCH session active, so we can tell which is happening in Fennec and
> which is happening in Search activity.

Actually, that search probe landed before that Method.SETTING probe landed (that isn't in the tree yet), so it used a Method.DIALOG. I think that actually makes sense, though, since a dialog pops up when you're actually setting the different default engine (in both Fennec settings and the search activity settings).
Attached patch 001-Bug_1055315-Telemetry.patch (obsolete) — Splinter Review
I haven't verified the end results though, can you point out where to look for telemetry data. 
I confirmed with the following logcat output:
D/Telemetry(24343): SendUIEvent: event = search.setdefault.1 method = dialog timestamp = 102701338 extras = GOOGLE.
Attachment #8477795 - Flags: review?(margaret.leibovic)
(In reply to ramasamy.zephyr from comment #6)
> Created attachment 8477795 [details] [diff] [review]
> 001-Bug_1055315-Telemetry.patch
> 
> I haven't verified the end results though, can you point out where to look
> for telemetry data. 
> I confirmed with the following logcat output:
> D/Telemetry(24343): SendUIEvent: event = search.setdefault.1 method = dialog
> timestamp = 102701338 extras = GOOGLE.

Verifying via logcat is our common practice. After things land we'll start looking for it in UI Telemetry too.
Comment on attachment 8477795 [details] [diff] [review]
001-Bug_1055315-Telemetry.patch

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

Looking good, thanks! I just have one piece of feedback to make our data more consistent with future data.

Also, it would be awesome if you could make an equivalent pull request for the fennec-search repo here: https://github.com/mozilla/fennec-search

However, if you're unfamiliar with github, I can also do that myself.

::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
@@ +137,5 @@
>              final ListPreference searchEnginePref = (ListPreference) findPreference(PREF_SEARCH_ENGINE_KEY);
>              searchEnginePref.setSummary(searchEnginePref.getEntry());
> +            Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH_SET_DEFAULT,
> +                                  TelemetryContract.Method.DIALOG,
> +                                  searchEnginePref.getValue());

In the future we will want to follow the same logic we do in Fennec to get the identifier for built-in engines, but use "other" for other engines, but right now (and for the forseeable future) you can only choose built-in engines in the search activity, so this works.

Currently we use hard-coded all-caps values for the prefs, but this will change with bug 1057629 to use the lower-case identifiers that Fennec uses. To make sure the data in telemetry is consistent, could you just add a .toLowerCase() to the end of the getValue() call?
Attachment #8477795 - Flags: review?(margaret.leibovic) → review+
I will generate a pull request to github as soon as this gets +reviewed. I will also update the Telemetry contract to reflect the one that is currently in mozilla central.
Attachment #8477795 - Attachment is obsolete: true
Attachment #8478683 - Flags: review?(margaret.leibovic)
Comment on attachment 8478683 [details] [diff] [review]
001-Bug_1055315-Telemetry.patch

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

Awesome, thanks! I can land this on fx-team for you.
Attachment #8478683 - Flags: review?(margaret.leibovic) → review+
Thanks for being very welcoming and helpful. I will try to do more work, if its possible for me in fennec search. :)
https://hg.mozilla.org/mozilla-central/rev/c28054999911
Assignee: nobody → ramasamy.zephyr
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.