979443 - Telemetry for home banner

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Did the user click on it? Did the user dismiss it? Did the user actually set up sync?

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Deb, what info do you want?

Comment 2

[Deb Richardson [:dria] (plz NEEDINFO)]

I'd like to know how many people:
1) tap on it
2) close it without tapping on it
3) close it after tapping on it (but not actually setting up a Fx Account)
4) actually start the Fx Accounts set up process
5) actually complete the Fx Accounts set up process

I'm not sure we can do #3, and I expect #4 and #5 are metrics we can already get from the Fx Accounts team.  If so, just ignore those.

Comment 3

[:Margaret Leibovic]

Sola, we decided to assign this to you because you're our new resident UI telemetry expert, but please let me know if you need help figuring out how the sync promo banner works!

Items 1-3 could be done in JS in Snippets.js, where we add the sync promo banner, but we may need help from someone like rnewman for advice about how to do items 4-5.

Comment 4

[Sola Ogunsakin [:sola]]

awesome thanks!

Comment 5

[Richard Newman [:rnewman]]

For #3, we can say whether you had a Sync account set up at the time you closed the banner, and we can probably say whether you had one prior to the banner being shown. Those are a pretty good proxy for "the banner did/did not cause them to set up Sync". Will that work, Deb?

Comment 6

[Richard Newman [:rnewman]]

Sola: for 3-4, you're basically instrumenting calls to Accounts.jsm. 5 requires some persistent state: you want to track when the banner has been tapped, and then at some point in the future you need to check to see whether Sync was set up. It might be possible to add that to the "should I show this banner?" check -- if it's been shown before, and tapped, but now Sync is set up, don't show... but record success!

Comment 7

[Deb Richardson [:dria] (plz NEEDINFO)]

(In reply to Richard Newman [:rnewman] from comment #5)
> For #3, we can say whether you had a Sync account set up at the time you
> closed the banner, and we can probably say whether you had one prior to the
> banner being shown. Those are a pretty good proxy for "the banner did/did
> not cause them to set up Sync". Will that work, Deb?

If a user already has a Fx Account set up, should we be showing them the banner at all?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Deb Richardson [:dria] from comment #7)

> If a user already has a Fx Account set up, should we be showing them the
> banner at all?

No (though I don't rule out bugs, which is why I suggest including anyway whether you have an existing account).

The edge cases that cause me to say "a pretty good proxy" are:

* You tap the banner, don't set up Sync, then set it up later via the menu or Android Settings. False negative or fairly accurate, depending on implementation details.
* You don't tap the banner, set up Sync via the menu or Android Settings, then dismiss the banner. False positive, depending on implementation details.

Comment 9

[Richard Newman [:rnewman]]

Chenxia, do you have time to knock this out?

Comment 10

[Chenxia Liu [:liuche]]

Yep, I've got some WIP for this, will finish them up today.

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Patch: UI Telemetry for sync

Man, the sync flow goes all over the place!

Richard, I didn't do what you suggested because we close the homebanner immediately, not after Sync setup returns, so checking account state on homebanner close isn't useful.

Instead, I made a SYNC_SETUP session, send a "homebanner" event if setup is launched from the homebanner, put in some events for clicks through the setup UI, and handle session closes for the various endpoints of Sync setup. This might be a pain in analysis, because you have correlate the "action/homebanner" event to other events in the sync session, as well as its session close to know how sync setup turned out from the homebanner - any thoughts, mfinkle?

Alternatively, we could run a SYNC_PROMO session in parallel to a SYNC_SETUP one, or change the telemetry API to include an extra ("promo" or none), or discard the SYNC_SETUP session entirely in favor of SYNC_PROMO if we don't care about how people use Sync setup aside from home banner.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8425183 [details] [diff] [review]
Patch: UI Telemetry for sync

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

I started to comment inline, but this warrants a more full treatment.

This is another one of those places where it might be convenient to add some coupling -- e.g., exposing the Telemetry interfaces in a-s -- between FxA and Fennec.

But we should think carefully, because adding that coupling is a little annoying, and also indicative of a certain kind of fragile intertwining.

I see two major advantages to doing so. I'm not yet convinced whether those are big enough to override the "no" from the coupling.

1. Convenience.
2. The ability to easily record telemetry when users enter FxA etc. flows without first taking an action inside Fennec.

There are ways to do the latter otherwise (e.g., broadcasts), but not so convenient.

If we don't expose Telemetry, we'd do something like this:

* Record start and return for activities from within Fennec. We can feed values into the launch intent, too. This is easy: Fennec owns the in and out.
* Recognize on entry into Fennec that the setup flow wasn't launched from Fennec. That should catch most of the external setup starts.
* Don't record detailed telemetry during the setup flow: we record what we get by returning values from the activity.


You might consider splitting out a Part 1 that just instruments the snippet and the Sync menu item, and leave the FxA activities for later. Those alone are valuable data, and will be easier to uplift.

::: mobile/android/base/TelemetryContract.java
@@ +58,5 @@
>          // Sharing content.
>          SHARE("share.1"),
>  
> +        // Logging in or creating an account; details in extras.
> +        ACCOUNT("account.1"),

Don't forget the docs. m/a/b/docs/index.rst.

::: mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java
@@ +8,5 @@
>  import java.util.concurrent.Executors;
>  
>  import org.mozilla.gecko.R;
> +import org.mozilla.gecko.Telemetry;
> +import org.mozilla.gecko.TelemetryContract;

Don't forget that this lives in android-services, so you (a) can't do this, and (b) need these changes to live upstream.

@@ +87,5 @@
>        finish();
>        return;
>      }
>  
> +    Telemetry.stopUISession(TelemetryContract.Session.SYNC_PROMO, TelemetryContract.Reason.SUCCESS);

I suggest doing this in the onActivityResult handler. You launched FxA setup from somewhere; let that place listen for the result, too.

::: mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java
@@ +254,5 @@
>              : null;
>          if (FxAccountAgeLockoutHelper.passesAgeCheck(yearEdit.getText().toString(), yearItems)) {
>            FxAccountConstants.pii(LOG_TAG, "Passed age check.");
>            createAccount(email, password, engines);
> +          Telemetry.sendUIEvent(TelemetryContract.Event.ACCOUNT, TelemetryContract.Method.BUTTON, "create");

setResult(RESULT_CREATED_BUTTON), perhaps.

::: mobile/android/base/fxa/activities/FxAccountCreateAccountNotAllowedActivity.java
@@ +33,5 @@
>      super.onCreate(icicle);
>      setContentView(R.layout.fxaccount_create_account_not_allowed);
>      TextView view = (TextView) findViewById(R.id.learn_more_link);
>      ActivityUtils.linkTextView(view, R.string.fxaccount_account_create_not_allowed_learn_more, R.string.fxaccount_link_create_not_allowed);
> +    Telemetry.stopUISession(TelemetryContract.Session.SYNC_PROMO, TelemetryContract.Reason.CANCEL);

If we got here, it'll be because we setResult(RESULT_CANCELED).

::: mobile/android/components/Snippets.js
@@ +353,5 @@
>          icon: "drawable://sync_promo",
>          onclick: function() {
>            // Remove the message, so that it won't show again for the rest of the app lifetime.
>            Home.banner.remove(id);
> +          let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;

Let's move this logic -- which has already been used in at least one place -- into UITelemetry. Call it "uptimeMillis". Then at least we have only one place to fix in Bug 1007647.

Comment 13

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

(In reply to Richard Newman [:rnewman] from comment #12)

> You might consider splitting out a Part 1 that just instruments the snippet
> and the Sync menu item, and leave the FxA activities for later. Those alone
> are valuable data, and will be easier to uplift.

I think that should be the extent of this bug, IMO. This bug is about the promo banner, not the full flow of Sync. If we still need that, given FxA Sync will have already been shipped for 3 releases by the time the telemetry lands, we can file a new bug.

Honestly, do we even need this bug anymore? We are getting data from server-side metrics about FxA sign ups.

To answer my own question, I do think getting Banner telemetry is useful. We should know if people are using or dismissing Banners. I think the Snippets part of this patch should move forward. Maybe even make it generic for all banners.

> > +          let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;
> 
> Let's move this logic -- which has already been used in at least one place
> -- into UITelemetry. Call it "uptimeMillis". Then at least we have only one
> place to fix in Bug 1007647.

Great separate bug.

Comment 14

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

Comment on attachment 8425183 [details] [diff] [review]
Patch: UI Telemetry for sync

>diff --git a/mobile/android/components/Snippets.js b/mobile/android/components/Snippets.js

>           Home.banner.remove(id);
>+          let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;
>+          UITelemetry.startSession("syncsetup.1", uptime);

No need for a Session

>+          UITelemetry.addEvent("action.1", "homebanner", uptime);

Pass the | id | along as an extra?

I'd shorten "homebanner" to just "banner".

>+          UITelemetry.addEvent("cancel.1", "homebanner", uptime);

Same: send | id | as extra and "homebanner" -> "banner"

Comment 15

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #13)

> > You might consider splitting out a Part 1 that just instruments the snippet
> > and the Sync menu item, and leave the FxA activities for later. Those alone
> > are valuable data, and will be easier to uplift.
> 
> I think that should be the extent of this bug, IMO.

WFM fo sho.

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Part 1: Home banner telemetry

Stripped out the Sync probes, and isolated this to just track home banner.

mfinkle: I'm not sure that the id extra is very useful because it's a unique alphanumeric sequence id; if we care about sync in particular, we could make that extra="sync" (just for sync).

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 2: Update docs

Also filed [good-first-bug] bug 1013672 for updating the rest of the telem docs. If no one bites this week I'll update them myself.

Comment 18

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

Comment on attachment 8425921 [details] [diff] [review]
Part 1: Home banner telemetry

>diff --git a/mobile/android/components/Snippets.js b/mobile/android/components/Snippets.js

>+Cu.import("resource://gre/modules/UITelemetry.jsm");

Looks like we could lazy load this instead:

XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry", "resource://gre/modules/UITelemetry.jsm");

I'm OK with sending the IDs. I am assuming we can convert the well known IDs to easy strings in scripts, like we do for Homepanels. If it seems like a waste after looking at the output, we can drop the IDs from the code.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 1: Home banner telemetry v2

Keeping the ids, updating to lazy load.

Comment 20

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/b63c25c3af2a
https://hg.mozilla.org/integration/fx-team/rev/ec9b96d0f62e

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b63c25c3af2a
https://hg.mozilla.org/mozilla-central/rev/ec9b96d0f62e

Comment 22

[:Margaret Leibovic]

Comment on attachment 8426434 [details] [diff] [review]
Part 1: Home banner telemetry v2

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

::: mobile/android/components/Snippets.js
@@ +203,5 @@
>        weight: message.weight,
>        onclick: function() {
>          let parentId = gChromeWin.BrowserApp.selectedTab.id;
>          gChromeWin.BrowserApp.addTab(message.url, { parentId: parentId });
> +        UITelemetry.addEvent("action.1", "banner", null, id);

I wish I had seen this before it landed, but this id is really not very useful, since it's randomly generated for each message. Using the actual snippet id (message.id) would be more useful.

@@ +359,5 @@
>            // Remove the message, so that it won't show again for the rest of the app lifetime.
>            Home.banner.remove(id);
>            Accounts.launchSetup();
> +
> +          UITelemetry.addEvent("action.1", "banner", null, id);

Same thing here, maybe it would be better to record that this is the sync banner.

Comment 23

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #22)

> I wish I had seen this before it landed, but this id is really not very
> useful, since it's randomly generated for each message. Using the actual
> snippet id (message.id) would be more useful.

My bad, that's what I expected the id to be.

I'm A-OK with landing a follow-up. We'll have one day's worth of Nightly noise.

Comment 24

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

(In reply to :Margaret Leibovic from comment #22)

> >            // Remove the message, so that it won't show again for the rest of the app lifetime.
> >            Home.banner.remove(id);
> >            Accounts.launchSetup();
> > +
> > +          UITelemetry.addEvent("action.1", "banner", null, id);
> 
> Same thing here, maybe it would be better to record that this is the sync
> banner.

If we have no | message.id | in this case, then yes, let's just hardcode a "syncpromo" Extra.

Comment 25

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #24)
> (In reply to :Margaret Leibovic from comment #22)
> 
> > >            // Remove the message, so that it won't show again for the rest of the app lifetime.
> > >            Home.banner.remove(id);
> > >            Accounts.launchSetup();
> > > +
> > > +          UITelemetry.addEvent("action.1", "banner", null, id);
> > 
> > Same thing here, maybe it would be better to record that this is the sync
> > banner.
> 
> If we have no | message.id | in this case, then yes, let's just hardcode a
> "syncpromo" Extra.

Correct, there's no message.id, since this doesn't come from a snippet.

Comment 26

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

Can this and bug 1017191 be uplifted?

Comment 27

[Chenxia Liu [:liuche]]

Comment on attachment 8426434 [details] [diff] [review]
Part 1: Home banner telemetry v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New telemetry probes
User impact if declined: No probes for home banner
Testing completed (on m-c, etc.): Baked on nightly
Risk to taking this patch (and alternatives if risky): very low, adds two probes
String or IDL/UUID changes made by this patch: none

Should be uplifted with bug 1017191.

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8426434 [details] [diff] [review]
Part 1: Home banner telemetry v2

Aurora approval granted.

Comment 29

[Chenxia Liu [:liuche]]

Only uplifted Part 1:
https://hg.mozilla.org/releases/mozilla-aurora/rev/63cd4aa566c5