996753 - Add telemetry for Settings usage

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

We currently don't know how users use settings, which would be good to know so we can optimize how settings are laid out, or where we should add more hints. Are users looking for settings and then giving up? What settings do they change?

The most basic thing we can track is what settings users are clicking, and which ones they are actually changing. This could be done by creating Telemetry histograms. Getting a baseline would be easy, but tracking where a particular setting is in different versions of the Setting hierarchy would be tricky.

If we want more behavioral information, we could use UI Telemetry with Settings sessions; we could see if users are exiting Settings without changing anything, and (possibly?) see what sequence of settings items they're traversing. 

The post-processing on UI Telemetry will be more complicated than just getting histograms, so there's a tradeoff between usefulness of data and simplicity of implementation.

Comment 2

[:Margaret Leibovic]

If we're targeting bug 965377 for 34, we should get some telemetry in place.

Comment 3

[:Margaret Leibovic]

Attachment: WIP - Telemetry probes for settings pages

There is a lot of stuff we can record in settings, so I decided to try to start simple by recording when users go to different pages within settings. We can then add more individual probes for where they toggle various settings.

I made a session around when the user enters/exits the main settings screen. This won't catch when users get to settings other ways (like the telemetry notification), but I think that's okay. I made a new SETTINGS method for entering different settings pages, but maybe this should be an event instead? I'm always uncertain about the best structure for these probes. I was thinking I could also use this method for probes that track when users toggle prefs in settings. Perhaps that could be event EDIT with action SETTINGS and then the pref name could go in the extras.

I'm also finding that exiting settings is super laggy and slow on my Nexus S running 2.3.6, but this is happening even without my patch applied, so maybe that phone is just slow and old.

Comment 4

[:Margaret Leibovic]

Attachment: Telemetry probes for changing settings and hitting back

This won't cover our custom preference items, like home panels or search settings, but this will catch the majority of preference changes. We've already started an effort to add probes for these custom items, so we can just continue to chug away at that.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8474852 [details] [diff] [review]
WIP - Telemetry probes for settings pages

I like this. I think a SETTINGS method makes sense. It's not a BUTTON or a MENU, it's a SETTING.

Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, Method.SETTINGS, resourceName);

LGTM

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8474852 [details] [diff] [review]
WIP - Telemetry probes for settings pages

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

lgtm, one comment.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +481,5 @@
>          if (mPrefsRequestId > 0) {
>              PrefsHelper.removeObserver(mPrefsRequestId);
>          }
> +
> +        if (Versions.preHC && intentExtras == null) {

Where is intentExtras coming from? Why do we care if it's null?

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8474858 [details] [diff] [review]
Telemetry probes for changing settings and hitting back

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +471,5 @@
>          if (NO_TRANSITIONS) {
>              overridePendingTransition(0, 0);
>          }
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, Method.BACK, "settings");

Does this fire for both Fragment-based preferences and pre-v11 preferences? I'm not sure if the Fragment handles backPressed (but it might), or if the enclosing activity also receives the backPressed call. I guess it probably does?

Comment 8

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Comment on attachment 8474852 [details] [diff] [review]
> WIP - Telemetry probes for settings pages
> 
> Review of attachment 8474852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, one comment.
> 
> ::: mobile/android/base/preferences/GeckoPreferences.java
> @@ +481,5 @@
> >          if (mPrefsRequestId > 0) {
> >              PrefsHelper.removeObserver(mPrefsRequestId);
> >          }
> > +
> > +        if (Versions.preHC && intentExtras == null) {
> 
> Where is intentExtras coming from? Why do we care if it's null?

That if statement deserves a comment for sure. When intentExtras is null, that means that we're at the top-level activity, so we'll only end the session when we destroy that top-level activity (as opposed to the sub-activities that launch when users drill into settings pages).

(In reply to Chenxia Liu [:liuche] from comment #7)
> Comment on attachment 8474858 [details] [diff] [review]
> WIP - Telemetry probes for changing settings and hitting back
> 
> Review of attachment 8474858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/preferences/GeckoPreferences.java
> @@ +471,5 @@
> >          if (NO_TRANSITIONS) {
> >              overridePendingTransition(0, 0);
> >          }
> > +
> > +        Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, Method.BACK, "settings");
> 
> Does this fire for both Fragment-based preferences and pre-v11 preferences?
> I'm not sure if the Fragment handles backPressed (but it might), or if the
> enclosing activity also receives the backPressed call. I guess it probably
> does?

I can double check this, but I believe that the activity also receives the back pressed call in the fragment lifecycle. I was testing these patchs on a 4.4 device and a 2.3 device.

Comment 9

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #8)
> (In reply to Chenxia Liu [:liuche] from comment #6)
> > Comment on attachment 8474852 [details] [diff] [review]
> > WIP - Telemetry probes for settings pages
> > 
> > Review of attachment 8474852 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > lgtm, one comment.
> > 
> > ::: mobile/android/base/preferences/GeckoPreferences.java
> > @@ +481,5 @@
> > >          if (mPrefsRequestId > 0) {
> > >              PrefsHelper.removeObserver(mPrefsRequestId);
> > >          }
> > > +
> > > +        if (Versions.preHC && intentExtras == null) {
> > 
> > Where is intentExtras coming from? Why do we care if it's null?
> 
> That if statement deserves a comment for sure. When intentExtras is null,
> that means that we're at the top-level activity, so we'll only end the
> session when we destroy that top-level activity (as opposed to the
> sub-activities that launch when users drill into settings pages).

Oops, I also realize this version of the patch was missing a local variable declaration!

This should be getIntent().getExtras().

Comment 10

[:Margaret Leibovic]

Attachment: Telemetry probes for settings pages

Updated to address comments.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8474858 [details] [diff] [review]
Telemetry probes for changing settings and hitting back

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

Yup, I verified the back press gets handled for fragments.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/ec7f0f0432f5
https://hg.mozilla.org/integration/fx-team/rev/a1a03dfa771a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec7f0f0432f5
https://hg.mozilla.org/mozilla-central/rev/a1a03dfa771a

Comment 14

[:Margaret Leibovic]

Comment on attachment 8474858 [details] [diff] [review]
Telemetry probes for changing settings and hitting back

We will also want to uplift bug 1058813 and bug 1063128 with the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: part of effort to improve settings
[User impact if declined]: we have less insight into what users do
[Describe test coverage new/current, TBPL]: no tests, living in aurora since the beginning of the cycle
[Risks and why]: very low risk, just adds telemetry probe
[String/UUID change made/needed]: none

Comment 15

[:Margaret Leibovic]

Comment on attachment 8477706 [details] [diff] [review]
Telemetry probes for settings pages

Approval Request Comment
[Feature/regressing bug #]: part of effort to improve settings
[User impact if declined]: we have less insight into what users do
[Describe test coverage new/current, TBPL]: no tests, living in aurora since the beginning of the cycle
[Risks and why]: very low risk, just adds telemetry probe
[String/UUID change made/needed]: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d7b3bfaf3ab

Comment 17

[Ryan VanderMeulen [:RyanVM]]

And https://hg.mozilla.org/releases/mozilla-beta/rev/3504f727e58c

Comment 18

[Mats Palmgren (inactive)]

Looks like this broke the build on Beta.

mobile/android/base/preferences/GeckoPreferences.java:490: error: cannot find symbol
        if (Versions.preHC && getIntent().getExtras() == null) {
            ^
  symbol:   variable Versions
  location: class GeckoPreferences

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Yeah, discussing w/ nalexander in #mobile.

Comment 20

[:Margaret Leibovic]

Sorry about that, I can make a rebased version for beta.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Bustage follow-up:
https://hg.mozilla.org/releases/mozilla-beta/rev/7cd3ae0255ec

Comment 22

[Flaviu Cos]

Verified as fixed in builds:
- 35.0a1 (2014-09-29);
- 34.0a2 (2014-09-29);
- 33 beta 7;
Device: LG Nexus 4 (Android 4.4.4).