Closed Bug 979443 Opened 7 years ago Closed 7 years ago

Telemetry for home banner

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox31 fixed, firefox32 fixed, fennec31+)

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
fennec 31+ ---

People

(Reporter: Margaret, Assigned: liuche)

References

Details

Attachments

(2 files, 2 obsolete files)

Did the user click on it? Did the user dismiss it? Did the user actually set up sync?
Deb, what info do you want?
Assignee: nobody → oogunsakin
tracking-fennec: ? → 29+
Flags: needinfo?(deb)
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.
Flags: needinfo?(deb)
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.
awesome thanks!
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?
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!
Status: NEW → ASSIGNED
(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?
(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.
tracking-fennec: 29+ → 31+
Chenxia, do you have time to knock this out?
Assignee: oogunsakin → liuche
Yep, I've got some WIP for this, will finish them up today.
Depends on: 1009315
Attached patch Patch: UI Telemetry for sync (obsolete) — Splinter Review
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.
Attachment #8425183 - Flags: feedback?(rnewman)
Attachment #8425183 - Flags: feedback?(mark.finkle)
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.
Attachment #8425183 - Flags: feedback?(rnewman) → feedback-
(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 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"
Attachment #8425183 - Flags: feedback?(mark.finkle) → feedback+
(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.
Summary: Telemetry for sync promo banner → Telemetry for home banner
Attached patch Part 1: Home banner telemetry (obsolete) — Splinter Review
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).
Attachment #8425183 - Attachment is obsolete: true
Attachment #8425921 - Flags: review?(rnewman)
Attachment #8425921 - Flags: feedback?(mark.finkle)
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.
Attachment #8425923 - Flags: review?(rnewman)
Depends on: 1013601
Attachment #8425923 - Flags: review?(rnewman) → review+
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.
Attachment #8425921 - Flags: feedback?(mark.finkle) → feedback+
Keeping the ids, updating to lazy load.
Attachment #8425921 - Attachment is obsolete: true
Attachment #8425921 - Flags: review?(rnewman)
Attachment #8426434 - Flags: review?(rnewman)
No longer depends on: 1009315
Attachment #8426434 - Flags: review?(rnewman) → review+
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.
(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.
Blocks: 1017191
(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.
(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.
Can this and bug 1017191 be uplifted?
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.
Attachment #8426434 - Flags: approval-mozilla-aurora?
Comment on attachment 8426434 [details] [diff] [review]
Part 1: Home banner telemetry v2

Aurora approval granted.
Attachment #8426434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.