Closed Bug 1007523 Opened 6 years ago Closed 5 years ago

Add UI telemetry for managing search engines

Categories

(Firefox for Android :: Settings and Preferences, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
firefox32 --- wontfix
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified
fennec 32+ ---

People

(Reporter: mfinkle, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We get feedback about people not understanding that they can manage search engines in Settings. Let's add some simple probes for:
* Setting default
* Removing/Hiding
* Add a Session for the "Search" category
I think we can model this after the "setdefault.1" Event that we use for panels. We pass the ID of the panel as the Extra. For search engines, we could pass the engine.id (or name) as the Extra if the search engine is a built-in engine. If the engine is a user-added engine, let's just use "other" as the Extra.

If we use "setdefault.1", I think we need more context since we won't easily know if the "setdefault.1" is for panels or search engines:
1. We could force a Session to wrap the "Home Panels" and "Search" sections of Settings. We could use "settings.1:home" and "settings.1:search" as Session names. This matches our convention for "homepanel.1:<panel>"
2. We could modify the Event from "setdefault.1" to "setdefault.panel.1" and "setdefault.search.1"
3. We could add a Method, since "setdefault.1" does not currently use a Method. I am trying to think of a Method name(s) that make sense, but it's not coming easy. For example, using "search" and "panel" as Methods does not fit our convention.

My current thought is to do #1, since that is how we designed the UI Telemetry system to work: Sessions provide additional context.

Thoughts?
Do we need this before we can go ahead with bug 1049108? That bug is tracking 32, which means we would need to do this ASAP.

(In reply to Mark Finkle (:mfinkle) from comment #1)
> I think we can model this after the "setdefault.1" Event that we use for
> panels. We pass the ID of the panel as the Extra. For search engines, we
> could pass the engine.id (or name) as the Extra if the search engine is a
> built-in engine. If the engine is a user-added engine, let's just use
> "other" as the Extra.
> 
> If we use "setdefault.1", I think we need more context since we won't easily
> know if the "setdefault.1" is for panels or search engines:
> 1. We could force a Session to wrap the "Home Panels" and "Search" sections
> of Settings. We could use "settings.1:home" and "settings.1:search" as
> Session names. This matches our convention for "homepanel.1:<panel>"

With the search activity telemetry we're finding that we're getting events as part of the search activity session that really belong in another session. Is this just because we're ending the session in the activity's onStop method, and that's slow to get called? Or is this because starting/ending sessions is asynchronous? If it's the latter, I would be worried about this session method for getting good data.

> 2. We could modify the Event from "setdefault.1" to "setdefault.panel.1" and
> "setdefault.search.1"

This seems like the most clear solution to me.

> 3. We could add a Method, since "setdefault.1" does not currently use a
> Method. I am trying to think of a Method name(s) that make sense, but it's
> not coming easy. For example, using "search" and "panel" as Methods does not
> fit our convention.

I agree. And in the future maybe there will be multiple ways to change the default, and if that ever happens we would want this method field.
tracking-fennec: --- → ?
liuche, do you have an opinion here?
Flags: needinfo?(liuche)
(In reply to :Margaret Leibovic from comment #2)

> With the search activity telemetry we're finding that we're getting events
> as part of the search activity session that really belong in another
> session. Is this just because we're ending the session in the activity's
> onStop method, and that's slow to get called? Or is this because
> starting/ending sessions is asynchronous? If it's the latter, I would be
> worried about this session method for getting good data.

For activity A transitioning to activity B, the lifecycle goes:

A.onStart
A.onResume
...
A.onPause
B.onStart
B.onResume **
A.onStop   **

... because otherwise there would be a gap with no frontmost activity.

We shouldn't really be ending sessions in onStop.

** These two might be the other way around; I don't remember.
Blocks: 1049108
Assignee: nobody → margaret.leibovic
(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> 
> > With the search activity telemetry we're finding that we're getting events
> > as part of the search activity session that really belong in another
> > session. Is this just because we're ending the session in the activity's
> > onStop method, and that's slow to get called? Or is this because
> > starting/ending sessions is asynchronous? If it's the latter, I would be
> > worried about this session method for getting good data.
> 
> For activity A transitioning to activity B, the lifecycle goes:
> 
> A.onStart
> A.onResume
> ...
> A.onPause
> B.onStart
> B.onResume **
> A.onStop   **
> 
> ... because otherwise there would be a gap with no frontmost activity.
> 
> We shouldn't really be ending sessions in onStop.
> 
> ** These two might be the other way around; I don't remember.

Okay, this leads me to believe this is just an issue with our search activity telemetry, so I'll explore creating a session for the search section of settings (mfinkle's approach #1).
After investigating this a bit, I think the sessions approach is too tricky, since our preferences activity code is full of different logic paths for different Android versions. I think it would be more straightforward to add a probe similar to what we have for the home panels.

(In reply to Mark Finkle (:mfinkle) from comment #0)
> We get feedback about people not understanding that they can manage search
> engines in Settings. Let's add some simple probes for:
> * Setting default
> * Removing/Hiding

Is there a reason we even need this all to be in Java UI telemetry? Is that just because it's easier to analyze with our current system? All of this is actually managed in gecko under the hood, so it feels like JS probes would be more straightforward.

> * Add a Session for the "Search" category

Is this because you want to know how often users even end up in this search pane? We could add an event for that.
(In reply to :Margaret Leibovic from comment #6)
> After investigating this a bit, I think the sessions approach is too tricky,
> since our preferences activity code is full of different logic paths for
> different Android versions. I think it would be more straightforward to add
> a probe similar to what we have for the home panels.
> 
> (In reply to Mark Finkle (:mfinkle) from comment #0)
> > We get feedback about people not understanding that they can manage search
> > engines in Settings. Let's add some simple probes for:
> > * Setting default
> > * Removing/Hiding
> 
> Is there a reason we even need this all to be in Java UI telemetry? Is that
> just because it's easier to analyze with our current system? All of this is
> actually managed in gecko under the hood, so it feels like JS probes would
> be more straightforward.

I might be easier to do in JS, but it's typically harder to get context of an action that starts in Java in JS probe. I mean, you might not know why it was triggered.

> > * Add a Session for the "Search" category
> 
> Is this because you want to know how often users even end up in this search
> pane? We could add an event for that.

We could add an Event for it. We'd trigger it when the "Search" section button is tapped. We could do the same with the rest of the sub-section items. Events aren't Sessions though and we would not get the extra Session context on any subsequent Events (Events are tagged with the active Sessions).
This patch just adds new events for setting the default, restoring the defaults, and removing a search engine.

I don't know how we feel about adding individual events for these things (maybe these are too specific), but nothing better came to mind.

I like doing it here in JS because it's all in one spot. Also, we wouldn't be able to do the "other" search engine logic if we were doing this in Java. However, I don't know if we should really store "other" for non-built-in engines, since it could be interesting to see what engines people install. We're already storing the ids of home panels that users are installing, so this doesn't seem much different.
Attachment #8471865 - Flags: feedback?(mark.finkle)
Attachment #8471865 - Flags: feedback?(liuche)
Comment on attachment 8471865 [details] [diff] [review]
Add UI telemetry for managing search engines

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

I think this looks good for getting basic telemetry on the things we're trying to collect, but I don't think it really fits into "UI" telemetry.

So, a few questions about why this is in the JS , not Java. I saw you mention that it's much easier to do this is JS than in Java - what are the reasons for that? Are we running Gecko without Fennec somehow? (Last I heard, headless Gecko was stalled on something.) Basically, one of the reasons that we have event, method, session, etc, is that these give us context on how these things are called. So it's actually a good thing to add probes in several places rather than one, because it gives us a better idea of how people are using features and how they are getting there - hence the UI part. The probes are lightweight to add for that reason.

Right now, it doesn't seem like there are multiple places where these events are triggered, so this might be okay by default, but if we add more paths in the future (more ways to remove search engines, or change the defaults), we'll probably want to add the probes in places where we can distinguish where they're being called.

Maybe we should consider making another module that is (non-UI) telemetry...?

::: mobile/android/base/TelemetryContract.java
@@ +69,5 @@
>  
> +        // Remove a search engine.
> +        SEARCH_REMOVE("search.remove.1"),
> +
> +        // Set default search engine.

Oops, comment.
Attachment #8471865 - Flags: feedback?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #9)
> Comment on attachment 8471865 [details] [diff] [review]
> Add UI telemetry for managing search engines
> 
> Review of attachment 8471865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good for getting basic telemetry on the things we're
> trying to collect, but I don't think it really fits into "UI" telemetry.
> 
> So, a few questions about why this is in the JS , not Java. I saw you
> mention that it's much easier to do this is JS than in Java - what are the
> reasons for that? Are we running Gecko without Fennec somehow? (Last I
> heard, headless Gecko was stalled on something.)

The main reason this is easier in JS than in Java is to record "other" for the non built-in search engines. Java doesn't actually know about how search engines are implemented, so we would need to pass along more information to Java somehow if we were to do this. Also I personally like the way this patch works out that all these probes end up in the same block of logic.

These search settings are a bit unique because under the hood everything really just gets passed along to JS, so I don't think we're missing out on anything if we skip the whole Java side.

> Basically, one of the
> reasons that we have event, method, session, etc, is that these give us
> context on how these things are called. So it's actually a good thing to add
> probes in several places rather than one, because it gives us a better idea
> of how people are using features and how they are getting there - hence the
> UI part. The probes are lightweight to add for that reason.

Right now there aren't actually multiple places to add these probes, since these events each only happen in one place. One reason I was feeling a bit uncertain about these event names is this fact that they're so specific.

> Right now, it doesn't seem like there are multiple places where these events
> are triggered, so this might be okay by default, but if we add more paths in
> the future (more ways to remove search engines, or change the defaults),
> we'll probably want to add the probes in places where we can distinguish
> where they're being called.

Yeah, I don't know what our future UI will look like, but right now users can only do this in the search settings. The only other way that defaults could be changed is through about:config or an add-on, but measuring changes from that would require sticking some sort of probe right in nsSearchService.js, which I don't think we want to do.

> Maybe we should consider making another module that is (non-UI) telemetry...?

I don't think the solution here is another module. I agree these probes don't really take advantage of the UI telemetry system, but in this case I think it's probably more worthwhile to just abuse the system a bit than to create another one.
Comment on attachment 8471865 [details] [diff] [review]
Add UI telemetry for managing search engines

I guess this seem OK. I wish we could share the Event with Home panels, but I won't block us. Is this easier than in Java? Without trying to set the Session, I assume it's no harder.

I guess we should change the other setdefault.1 to become home.setdefault.1 (or whatever) for some consistency and for any future work.

OK, I just saw the "easier to get the 'other' name" thing.
Attachment #8471865 - Flags: feedback?(mark.finkle) → feedback+
(In reply to :Margaret Leibovic from comment #10)
> (In reply to Chenxia Liu [:liuche] from comment #9)

> The main reason this is easier in JS than in Java is to record "other" for
> the non built-in search engines. Java doesn't actually know about how search
> engines are implemented, so we would need to pass along more information to
> Java somehow if we were to do this.

Not to disagree with JS vs Java, but we already pass the engine ID (i.e., whether this is a built-in engine or not) for recording searches:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/health/BrowserHealthRecorder.java#686


> from that would require sticking some sort of probe right in
> nsSearchService.js, which I don't think we want to do.

Or in BrowserSearch, which we're making an early-loaded persistent Java-side mirror of nsSearchService over in Bug 927692.
Status: NEW → ASSIGNED
OS: Linux → Android
Hardware: x86_64 → All
(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #12)
> (In reply to :Margaret Leibovic from comment #10)
> > (In reply to Chenxia Liu [:liuche] from comment #9)
> 
> > The main reason this is easier in JS than in Java is to record "other" for
> > the non built-in search engines. Java doesn't actually know about how search
> > engines are implemented, so we would need to pass along more information to
> > Java somehow if we were to do this.
> 
> Not to disagree with JS vs Java, but we already pass the engine ID (i.e.,
> whether this is a built-in engine or not) for recording searches:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/health/
> BrowserHealthRecorder.java#686

Oh, hm, I didn't realize that this `identifier` property was only present on built-in engines (as opposed to `name`, which is available for all engines). That makes me think I should be storing that instead of the engine name.

We do return that name in Java, so I could look into doing this in Java instead.
I take back everything I've said earlier in this bug. It looks like doing this in Java actually works pretty well (and in fact, there are 2 different ways to restore the defaults).
Attachment #8471865 - Attachment is obsolete: true
Attachment #8472683 - Flags: review?(liuche)
tracking-fennec: ? → 32+
Comment on attachment 8472683 [details] [diff] [review]
Add UI telemetry for managing search engines

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

Nice, good catches on some of the other coding nits too.
Attachment #8472683 - Flags: review?(liuche) → review+
Let's wait for this to bake on Nightly for a few days then uplift to aurora/beta once we can confirm we're getting the data we want.
Flags: needinfo?(liuche) → needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/c0b30ee8dca4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8472683 [details] [diff] [review]
Add UI telemetry for managing search engines

We may be too late for beta here, but I'm trying anyway :)

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: we don't learn about users' search engine settings behavior
[Describe test coverage new/current, TBPL]: landed on m-c 8/20 with no regressions, no automated tests
[Risks and why]: low-risk, adds some telemetry probes (doesn't change any user-facing behavior)
[String/UUID change made/needed]: none
Attachment #8472683 - Flags: approval-mozilla-beta?
Attachment #8472683 - Flags: approval-mozilla-aurora?
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8472683 [details] [diff] [review]
Add UI telemetry for managing search engines

Although this is a Telemetry probe and is fairly simple, we're really late in beta with beta10 set to go to build today and the RC ready to spin tomorrow. I'm denying this approval request for a non critical change on beta.

Approved for aurora.
Attachment #8472683 - Flags: approval-mozilla-beta?
Attachment #8472683 - Flags: approval-mozilla-beta-
Attachment #8472683 - Flags: approval-mozilla-aurora?
Attachment #8472683 - Flags: approval-mozilla-aurora+
Depends on: 1058352
Verified as fixed in builds:
- 35.0a1 (2014-09-05);
- 34.0a2 (2014-09-05);
- 33 beta 1;
Device: Google Nexus 7 (Android 4.4.4).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.