Closed Bug 981028 Opened 6 years ago Closed 6 years ago

Add telemetry probes for Top Sites

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 --- verified
firefox32 --- verified
fennec 31+ ---

People

(Reporter: deb, Assigned: oogunsakin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 10 obsolete files)

2.52 KB, patch
liuche
: review+
Details | Diff | Splinter Review
3.53 KB, patch
Details | Diff | Splinter Review
I'd like to know how/if users currently interact with our existing Top Sites implementation, including, if possible:

1) How many users see the top sites panel?
2) How often do users see the top sites panel?
3) When and how often do users see the top sites panel? (On first run? After opening a new tab? After closing all open tabs?)
4) How often do users tap on top sites thumbnails?
5) How many users have pinned a thumbnail on the Top Sites panel?
6) How many users have pinned multiple thumbnails on the Top Sites panel?
7) How many users have edited a top site?
8) How many users have permanently removed/deleted a thumbnail on the Top Sites panel?
9) How many users have changed their default home panel?
10) What built-in panels do users have set to default?

We can add more Hub-related probes later.

If we can't or don't want to do some of these for whatever reason, please comment so we can discuss it.  This list is very much off the top of my head.
Needinfo'ing ibarlow in case he has some additional ideas.
tracking-fennec: --- → ?
Flags: needinfo?(ibarlow)
The main questions I have are pretty simple:

1. To what extent are people using Top Sites (thumbnails and/or list items) to get to where they want to go?
2. For users who do use the Top Sites panel as a starting point, how many are simply letting frecency do the work for them in choosing what to display, vs how many are overriding it with their own pinned sites?

It looks like some combination of Deb's questions may be able to piece together answers to these.
Flags: needinfo?(ibarlow)
I think some of these probes are being added in bug 968308 and bug 977196. Let's make sure we don't add more than we need to answer the questions.

I don't believe we are adding any probes for the context menu actions on Home panels (or Top Sites specifically) so we should consider adding some to answer the "pinned" and "edited" questions.
tracking-fennec: ? → 31+
We should get an assignee here. Sola?
Flags: needinfo?(oogunsakin)
I'll take it
Assignee: nobody → oogunsakin
Flags: needinfo?(oogunsakin)
Attachment #8400915 - Flags: feedback?(liuche)
Attachment #8401032 - Flags: feedback?(liuche)
Attachment #8401034 - Flags: feedback?(liuche)
(In reply to Deb Richardson [:dria] from comment #0)
Status:
> 1) How many users see the top sites panel?
answered by bug 968308
> 2) How often do users see the top sites panel?
answered by bug 968308
> 3) When and how often do users see the top sites panel? (On first run? After
> opening a new tab? After closing all open tabs?)
"how do users see the top sites panel?" unanswered. not sure if our current setup can answer this.
> 4) How often do users tap on top sites thumbnails?
answered by bug 977196
> 5) How many users have pinned a thumbnail on the Top Sites panel?
answered by patch#1
> 6) How many users have pinned multiple thumbnails on the Top Sites panel?
answered by patch#1
> 7) How many users have edited a top site?
answered by patch#1
> 8) How many users have permanently removed/deleted a thumbnail on the Top
> Sites panel?
unanswered. do you mean unpinning a site?
> 9) How many users have changed their default home panel?
answered by patch#3
> 10) What built-in panels do users have set to default?
answered by patch#3
Comment on attachment 8400915 [details] [diff] [review]
Part 1: Probes for pinning & editing a top site item

In my efforts to keep Events generic and Sessions specific, I think just using "edit.1" and "pin.1" might be enough. Those events will have a ""homepanel.1:<panel_id>" Session, right?

I think that might be enough for us to analyze. Thoughts?
(In reply to Sola Ogunsakin [:sola] from comment #9)

> > 9) How many users have changed their default home panel?
> answered by patch#3
> > 10) What built-in panels do users have set to default?
> answered by patch#3

I wonder if we could find this using other means of analysis. When the Home panel is opened, we start a "home.1" Session. The first ""homepanel.1:<panel_id>" Session inside the "home.1" Session is the default panel (I think).

Is this enough to determine #9 and #10. If so, drop patch #3. NI'ing Josh to help answer this one.

Since patch #2 is not used to answer any of the questions, I'd rather hold off on using it.
Flags: needinfo?(jdover)
Comment on attachment 8400915 [details] [diff] [review]
Part 1: Probes for pinning & editing a top site item

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

::: mobile/android/base/TelemetryContract.java
@@ +15,5 @@
>       * Telemetry.sendUIEvent() as the "action" parameter.
>       */
> +    public interface Event {
> +        // Fired when a user pins a top site.
> +        public static final String TOP_SITE_PIN = "topsite.pin.1";

Nit: TOP_SITES* seems more consistent with the rest of our code base.

::: mobile/android/base/home/TopSitesPanel.java
@@ +339,5 @@
>                      BrowserDB.pinSite(context.getContentResolver(), url, title, position);
>                  }
>              });
>  
> +            // Record UI Telemetry event for pin.

Doesn't seem necessary.

@@ +363,5 @@
>              // Decode "user-entered" URLs before showing them.
>              mEditPinnedSiteListener.onEditPinnedSite(info.position, decodeUserEnteredUrl(info.url));
> +
> +            // Record UI Telemetry event for the edit.
> +            Telemetry.sendUIEvent(TelemetryContract.Event.TOP_SITE_EDIT);

A single-line Telemetry.sendUIEvent doesn't need a comment because the code is pretty self-documenting.
Attachment #8400915 - Flags: feedback?(liuche) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #11)
> I wonder if we could find this using other means of analysis. When the Home
> panel is opened, we start a "home.1" Session. The first
> ""homepanel.1:<panel_id>" Session inside the "home.1" Session is the default
> panel (I think).

bug 909618 makes it so that this is not always case
Comment on attachment 8401032 [details] [diff] [review]
Part 2: Sessions for preference pages

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

I don't think this handles pre-ICS devices.

::: mobile/android/base/TelemetryContract.java
@@ +39,5 @@
>          // Started when a user enters a given home panel.
>          // Session name is dynamic, encoded as "homepanel.1:<panel_id>"
>          public static final String HOME_PANEL = "homepanel.1:";
> +
> +        // Started when a user enters a dynamic settings/preference page.

Drop dynamic here.

@@ +41,5 @@
>          public static final String HOME_PANEL = "homepanel.1:";
> +
> +        // Started when a user enters a dynamic settings/preference page.
> +        // Session name is dynamic, encoded as "settings.1:<preference_res_name>"
> +        // examples include:

For example:

@@ +42,5 @@
> +
> +        // Started when a user enters a dynamic settings/preference page.
> +        // Session name is dynamic, encoded as "settings.1:<preference_res_name>"
> +        // examples include:
> +        //  "Display" -> "settings.1:preferences_display"

How about:
Display: "settings.1:preferences_display"

@@ +44,5 @@
> +        // Session name is dynamic, encoded as "settings.1:<preference_res_name>"
> +        // examples include:
> +        //  "Display" -> "settings.1:preferences_display"
> +        //  "Privacy" -> "settings.1:preferences_privacy"
> +        //  "Developer tools" -> "settings.1:preferences_devtools"

Two examples seem plenty, drop the last one.
Attachment #8401032 - Flags: feedback?(liuche)
Comment on attachment 8401034 [details] [diff] [review]
Part 3: Probe for setting a default panel

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

::: mobile/android/base/TelemetryContract.java
@@ +20,5 @@
>  
>          // Fired when a user edits a top site.
>          public static final String TOP_SITE_EDIT = "topsite.edit.1";
> +
> +        // Occurs when a home panel is made default

I think we might want to drop the verbose verbage here - it should be clear that these events are "fired", so just describe the event.

"Set default home panel" or something like that.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +172,5 @@
>  
>          mConfigEditor.setDefault(id);
>          mConfigEditor.apply();
> +
> +        // Record UI Telemetry event for "set default" action.

No comment needed, this one line of code seems pretty self-explanatory.
Attachment #8401034 - Flags: feedback?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #12)
> ::: mobile/android/base/home/TopSitesPanel.java
> @@ +339,5 @@
> >                      BrowserDB.pinSite(context.getContentResolver(), url, title, position);
> >                  }
> >              });
> >  
> > +            // Record UI Telemetry event for pin.
> 
> Doesn't seem necessary.
> 
> @@ +363,5 @@
> >              // Decode "user-entered" URLs before showing them.
> >              mEditPinnedSiteListener.onEditPinnedSite(info.position, decodeUserEnteredUrl(info.url));
> > +
> > +            // Record UI Telemetry event for the edit.
> > +            Telemetry.sendUIEvent(TelemetryContract.Event.TOP_SITE_EDIT);
> 
> A single-line Telemetry.sendUIEvent doesn't need a comment because the code
> is pretty self-documenting.

Alright, I was following guidelines set in this review https://bugzilla.mozilla.org/show_bug.cgi?id=968308#c11
(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Sola Ogunsakin [:sola] from comment #9)
> 
> > > 9) How many users have changed their default home panel?
> > answered by patch#3
> > > 10) What built-in panels do users have set to default?
> > answered by patch#3
> 
> I wonder if we could find this using other means of analysis. When the Home
> panel is opened, we start a "home.1" Session. The first
> ""homepanel.1:<panel_id>" Session inside the "home.1" Session is the default
> panel (I think).

We should be able to discern this from the session timestamps. However, once bug 909618
is finished and lands, about:home remembers its last position so the first "homepanel.1:"
session may not be the default panel.
Flags: needinfo?(jdover)
Attached patch bug-981028-pin-edit.patch.patch (obsolete) — Splinter Review
Attachment #8400915 - Attachment is obsolete: true
Attachment #8402032 - Attachment is obsolete: true
Attachment #8402033 - Flags: review?(liuche)
-handle pre-ICS devices
-change comments
Attachment #8401032 - Attachment is obsolete: true
Attachment #8402035 - Flags: review?(liuche)
-change comments
Attachment #8401034 - Attachment is obsolete: true
Attachment #8402036 - Flags: review?(liuche)
How do we want to define first run. First time the user opens app? first X minutes or hours of usage? etc?. There is no good way to tell when the app has been minimized or closed so the first approach would be more favourable from an implementation standpoint.
Flags: needinfo?(deb)
Comment on attachment 8402033 [details] [diff] [review]
Part 1: Probes for pinning & editing a top site item

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

::: mobile/android/base/TelemetryContract.java
@@ +14,5 @@
>       * Holds event names. Intended for use with
>       * Telemetry.sendUIEvent() as the "action" parameter.
>       */
> +    public interface Event {
> +        // Fired when a user pins a top site.

Let's move away from prefacing everything with "Fired when". How about "Pinning a Top Site"?

We should also track unpinning a top site.
Attachment #8402033 - Flags: review?(liuche) → feedback+
Attachment #8402033 - Attachment is obsolete: true
Attachment #8402087 - Flags: review?(liuche)
Attachment #8402036 - Attachment is obsolete: true
Attachment #8402036 - Flags: review?(liuche)
Attachment #8402088 - Flags: review?(liuche)
Yeah for simplicity's sake, let's say "first-run" = "when the user first opens the app".  If we need more/different information later, we can figure it out then.  Thanks.
Flags: needinfo?(deb)
(In reply to Sola Ogunsakin [:sola] from comment #22)
> How do we want to define first run. First time the user opens app? first X
> minutes or hours of usage? etc?. There is no good way to tell when the app
> has been minimized or closed so the first approach would be more favourable
> from an implementation standpoint.

Android has onPause / onResume triggers that can tell us when the app is "minimized" and "restored."
(In reply to Deb Richardson [:dria] from comment #0)

> 3) When and how often do users see the top sites panel? (On first run? After
> opening a new tab? After closing all open tabs?)

Does "first-run" mean the very first time a user starts Firefox, or does it mean when launching Firefox from the homescreen icon and about:home is opened?

It sounds to me like the latter.
(In reply to Mark Finkle (:mfinkle) from comment #27)
> Android has onPause / onResume triggers that can tell us when the app is
> "minimized" and "restored."

onPause gets called only when the main activity is no longer visible. its called when we launch another activity like settings.

similarly onResume gets called when the main activity become visible again (e.g after closing settings).
(In reply to Sola Ogunsakin [:sola] from comment #29)
> (In reply to Mark Finkle (:mfinkle) from comment #27)
> > Android has onPause / onResume triggers that can tell us when the app is
> > "minimized" and "restored."
> 
> onPause gets called only when the main activity is no longer visible. its
> called when we launch another activity like settings.
> 
> similarly onResume gets called when the main activity become visible again
> (e.g after closing settings).

Just to be clear, we need to define when to stop measuring "first run." Waiting until the app is actually killed (onDestroy called) will likely give us a longer session than we need, because apps can be backgrounded without being killed. onStop for BroweserApp gets called when the Settings are opened, because BrowserApp gets backgrounded. I don't see a simple way to track Application-level on* calls.

The question is mostly,
Comment on attachment 8402087 [details] [diff] [review]
Part 1: Probes for pinning & editing a top site item

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

Just comments.

::: mobile/android/base/TelemetryContract.java
@@ +14,5 @@
>       * Holds event names. Intended for use with
>       * Telemetry.sendUIEvent() as the "action" parameter.
>       */
> +    public interface Event {
> +        // User pinned a top site.

Sorry for all this back and forth, but here's a rule of thumb - if you're repeating the same thing many times, rethink if it's necessary. We know that all these events are going to be user-initiated events, so it's really not necessary to have that everywhere.

"Top site pinned", maybe?
Attachment #8402087 - Flags: review?(liuche)
After thinking and talking about this a little, I don't think we need to track the Settings session just yet. If we want to measure setting a default, we should not do that through the settings - do it through the API, in HomeConfig. It's okay if there's not an associated session - at the moment, the only way to set a default is through settings anyways, but we shouldn't hinge tracking that on the Setting UI.
Attachment #8402035 - Flags: review?(liuche)
Attachment #8402088 - Flags: review?(liuche)
Comment on attachment 8402035 [details] [diff] [review]
Part 2: Sessions for preference pages

dropping settings patch
Attachment #8402035 - Attachment is obsolete: true
-edit comments
Attachment #8402087 - Attachment is obsolete: true
Attachment #8403421 - Flags: review?(liuche)
Attachment #8402088 - Attachment is obsolete: true
Attachment #8403422 - Flags: review?(liuche)
These look good from my perspective. We can always add more later.
Attachment #8403421 - Flags: review?(liuche) → review+
Comment on attachment 8403422 [details] [diff] [review]
Part 2: Probe for setting a default panel

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

Looks good, except one last thing - we're not checking if we're setting a fallback default (in the case when the user disables/removes a default). I think we don't need to check that, because I don't think that's particularly useful, but we can add it later.
Attachment #8403422 - Flags: review?(liuche) → review+
-clearing "checkin-needed" due to potential bug with sessions leaking
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85e45452ebd7
https://hg.mozilla.org/mozilla-central/rev/b5fbb38d86d7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
The following probes are logged in logcat:

1. Change default home panel:
SendUIEvent: action = setdefault.1 method = null timestamp = 21774933 extras = 7f6d419a-cd6c-4e34-b26f-f68b1b551907
2. Pin site to Top Sites:
SendUIEvent: action = action.1 method = contextmenu timestamp = 21925964 extras = top_sites_pin
SendUIEvent: action = pin.1 method = null timestamp = 21925966 extras = null
3. Unpin site from Top Sites:
SendUIEvent: action = action.1 method = contextmenu timestamp = 21987369 extras = top_sites_unpin
SendUIEvent: action = unpin.1 method = null timestamp = 21987370 extras = null
4. Edit pinned site:
SendUIEvent: action = action.1 method = contextmenu timestamp = 22036460 extras = top_sites_edit
SendUIEvent: action = edit.1 method = null timestamp = 22036461 extras = null

Verified as fixed in builds:
- 32.0a1 (2014-05-19);
- 31.0a2 (2014-05-19);

Device: Google Nexus 4 (Android 4.4.2).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.