Add telemetry for Settings usage

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: liuche, Assigned: Margaret)

Tracking

(Blocks 2 bugs)

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

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox35 verified, fennec+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Assignee

Updated

5 years ago
Duplicate of this bug: 1004645
Assignee

Comment 2

5 years ago
If we're targeting bug 965377 for 34, we should get some telemetry in place.
tracking-fennec: --- → ?
Assignee

Updated

5 years ago
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
Assignee

Comment 3

5 years ago
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.
Attachment #8474852 - Flags: feedback?(mark.finkle)
Attachment #8474852 - Flags: feedback?(liuche)
Assignee

Comment 4

5 years ago
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.
Attachment #8474858 - Flags: feedback?(mark.finkle)
Attachment #8474858 - Flags: feedback?(liuche)
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
Attachment #8474852 - Flags: feedback?(mark.finkle) → feedback+
Attachment #8474858 - Flags: feedback?(mark.finkle) → feedback+
Reporter

Comment 6

5 years ago
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?
Attachment #8474852 - Flags: feedback?(liuche) → feedback+
Reporter

Comment 7

5 years ago
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?
Attachment #8474858 - Flags: feedback?(liuche) → feedback+
Assignee

Comment 8

5 years ago
(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.
Assignee

Comment 9

5 years ago
(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().
Assignee

Comment 10

5 years ago
Updated to address comments.
Attachment #8474852 - Attachment is obsolete: true
Attachment #8477706 - Flags: review?(liuche)
Assignee

Comment 11

5 years ago
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.
Attachment #8474858 - Flags: review?(liuche)
Assignee

Updated

5 years ago
Attachment #8474858 - Attachment description: WIP - Telemetry probes for changing settings and hitting back → Telemetry probes for changing settings and hitting back
Reporter

Updated

5 years ago
Attachment #8477706 - Flags: review?(liuche) → review+
Reporter

Updated

5 years ago
Attachment #8474858 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/ec7f0f0432f5
https://hg.mozilla.org/mozilla-central/rev/a1a03dfa771a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Assignee

Updated

5 years ago
Depends on: 1058813
Assignee

Updated

5 years ago
Blocks: 1063114
Assignee

Updated

5 years ago
Depends on: 1063128
Assignee

Comment 14

5 years ago
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
Attachment #8474858 - Flags: approval-mozilla-beta?
Assignee

Comment 15

5 years ago
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
Attachment #8477706 - Flags: approval-mozilla-beta?
Attachment #8477706 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8474858 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Yeah, discussing w/ nalexander in #mobile.
Assignee

Comment 20

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

Comment 22

5 years ago
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).
You need to log in before you can comment on or make changes to this bug.