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)
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)
1.29 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
As a follow-up to bug 1042943, we should add telemetry to know when a user changes their default search engine.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
(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)
Reporter | ||
Comment 4•10 years ago
|
||
(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.
Reporter | ||
Comment 5•10 years ago
|
||
(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).
Assignee | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
(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.
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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)
Reporter | ||
Comment 10•10 years ago
|
||
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+
Reporter | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c28054999911
Reporter | ||
Comment 12•10 years ago
|
||
https://github.com/mozilla/fennec-search/commit/60d254adfd1cdf6eb7ca823b5a8d05d1f785eb03 Thanks again!
Assignee | ||
Comment 13•10 years ago
|
||
Thanks for being very welcoming and helpful. I will try to do more work, if its possible for me in fennec search. :)
Comment 14•10 years ago
|
||
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
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
•