1401404 - Include Activity Stream user customizations in AS ping

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Activity Stream, defect, P1)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Grisha Kruglov from bug 1377288 comment #14)
> A thought: you might want to account for ability to configure the A-S panel.
> Perhaps having an ability to segment analytics (and dashboards) by which
> portions of the UI are enabled would be helpful? I'm not sure if the
> telemetry part of that was done along with other AS Settings work.

Good idea! I filed this bug.

> Also, what does desktop do to account for such user customizations?

It looks like desktop has bit-packed field "user_prefs" in their telemetry (see the description towards the bottom of the page): https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md

---

Adding to the current iteration because it sounds useful.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #0)
> It looks like desktop has bit-packed field "user_prefs" in their telemetry

Which I assume is sent for every UI event.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, I asked Maria the priority on Slack: I'll change the flags once I get a response.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And please update the docs too!

http://searchfox.org/mozilla-central/source/mobile/android/docs/activitystreamtelemetry.rst

Comment 4

[:Grisha Kruglov]

This should be easy to do as far as adding stuff to the telemetry bundle is concerned.

See https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java#88 for how we set a global "is FxA account present" flag. I reckon this will be very similar.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

From Maria: Great if it can land, but we won't hold the release for this.

(In reply to :Grisha Kruglov from comment #4)
> for how we set a global "is FxA account present" flag. I reckon this will be very
> similar.

Don't forget to update this after changing the settings (after bug 1395792) - perhaps we should put this setGlobal in onResume or similar?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

From liuche: we record when the preference changes.

---

This will let us know *how often* people toggle these preferences, but will not let us attribute the current preference state to a UI click. 

From Grisha: if [the customizations are] used enough, without segmenting A-S dashboards become potentially very misleading

---

So if this doesn't land, we can at least see how important it is that this does land. :)

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[eng triage recommendation] P1: Chenxia feels this is easy to fix.

Comment 8

[Chenxia Liu [:liuche]]

Updating the bug to make it more clear that this is to help us segment usage based on what is actually enabled (e.g., # of highlight clicks/total clicks of people who have highlights enabled).

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Bug 1401404 - Add telemetry for AS content prefs.

Review commit: https://reviewboard.mozilla.org/r/182414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182414/

Comment 10

[François Marier [:francois]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

https://reviewboard.mozilla.org/r/182414/#review187742

Category 2 data. datareview+

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

https://reviewboard.mozilla.org/r/182414/#review187750

I think the bit-packing is not quite right, unless you can explain to me otherwise. :) However, I think it'd be best to be consistent with desktop.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:64
(Diff revision 1)
>      }
>  
>      /**
> +     * AS_USER_PREFERENCES
> +     *
> +     * NB: Additional pref values to be bit-packed should be unique prime numbers.

Why do these need to be prime numbers? I'm not sure this is correct.

A conventional way to define bit-packing is with bit shift operators:

```
final int showSearch = 1 << 0; // 0001 
final int showTopSites = 1 << 1; // 0010
...
```

e.g. in your example, the bits for 2 (Pocket), 0010, conflict for those in 3 (Visited), 0011 so it'd be impossible to distinguish pocket enabled/disabled when visited is enabled.

Caveat: Java ints are signed so you'll have to make sure these values are actually 1, 2, and 4 (I think the signed bit is the first bit, so I think it's fine). Alternatively, just define them as 1, 2, and 4. :)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:66
(Diff revision 1)
>      /**
> +     * AS_USER_PREFERENCES
> +     *
> +     * NB: Additional pref values to be bit-packed should be unique prime numbers.
> +     **/
> +    private final static int POCKET_ENABLED_VALUE = 2;

Can we make these flags consistent with desktop? i.e. 

```
showSearch | 1 | | showTopSites | 2 | | showTopStories | 4 |
```

This should make analysis easier because we can use the same-ish code.

from https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:111
(Diff revision 1)
>  
>          public static Builder builder() {
>              return new Builder();
>          }
>  
> +

nit: ws

::: mobile/android/docs/activitystreamtelemetry.rst:132
(Diff revision 1)
>  
>  Full Examples
>  =============
>  Following examples of events are here to provide a better feel for the overall shape of telemetry data being recorded.
>  
> -1) User with an active Firefox Account clicked on a menu item for a third highlight ("visited"):
> +1) User with an active Firefox Account clicked on a menu item for a third highlight ("visited") and has preffed on AS top stories, bookmarks, and visited:

nit: `prefed`? I just figure since preference has one f, but I'm not sure. :)

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182414/diff/1-2/

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

https://reviewboard.mozilla.org/r/182414/#review187760

lgtm, thanks for the quick turn around! :)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:65
(Diff revision 2)
>  
>      /**
> +     * AS_USER_PREFERENCES
> +     *
> +     * NB: Additional pref values should be (unique) powers of 2
> +     * We skip 1 and 2 to be consistent with Desktop, because those prefs are not used in Firefox for Android.

Oh good call – I actually didn't notice that the prefs we use are different from those in desktop.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:78
(Diff revision 2)
> +     *
> +     * @param sharedPreferences SharedPreferences of this profile
> +     * @return bit-packed value of the user's AS preferences, which is the sum of the values of the enabled preferences.
> +     */
> +    public static int getASUserPreferencesValue(final SharedPreferences sharedPreferences, final Resources res) {
> +        int bitPackedPrefValue = 0;

Here's a wonky thought: we could start at 2 because that's the desktop preference and our top sites are always enabled. :)

I don't think we should though because that creates assumptions in the code that'd be easy to mess up in future iterations.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9909c8392ccf
Add telemetry for AS content prefs. r=francois,mcomella

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

Approval Request Comment
[Feature/Bug causing the regression]: Missing telemetry
[User impact if declined]: Won't have telemetry about the pref state on activity stream
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: local builds
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no, adding telemetry
[Why is the change risky/not risky?]: Only adding calls to telemetry 
[String changes made/needed]:

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

More telemetry, taking it.
Should be in 57b3.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9909c8392ccf

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44dc794755b4