Include Activity Stream user customizations in AS ping

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: liuche)

Tracking

unspecified
Firefox 58
All
Android
Points:
---

Firefox Tracking Flags

(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [mobileAS])

Attachments

(1 attachment)

(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.
(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.
Actually, I asked Maria the priority on Slack: I'll change the flags once I get a response.
tracking-fennec: --- → ?
Iteration: 1.30 → ---
Priority: P1 → --
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.
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?
tracking-fennec: ? → +
Iteration: --- → 1.30
Priority: -- → P1
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. :)
[eng triage recommendation] P1: Chenxia feels this is easy to fix.
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).
Summary: Add telemetry for Activity Stream user customization (e.g. disable Pocket panel) → Include Activity Stream user customizations in AS ping
Assignee: nobody → liuche
Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

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

Category 2 data. datareview+
Attachment #8910957 - Flags: review?(francois) → review+
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. :)
Attachment #8910957 - Flags: review?(michael.l.comella) → review-
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.
Attachment #8910957 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9909c8392ccf
Add telemetry for AS content prefs. r=francois,mcomella
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]:
Attachment #8910957 - Flags: approval-mozilla-beta?
Comment on attachment 8910957 [details]
Bug 1401404 - Add telemetry for AS content prefs.

More telemetry, taking it.
Should be in 57b3.
Attachment #8910957 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/9909c8392ccf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.