UI telemetry for home panels

RESOLVED FIXED in Firefox 31

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: sola)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 31
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 31 obsolete attachments)

1.82 KB, patch
Details | Diff | Splinter Review
3.79 KB, patch
Details | Diff | Splinter Review
5.18 KB, patch
Details | Diff | Splinter Review
2.14 KB, patch
liuche
: review+
Details | Diff | Splinter Review
4.87 KB, patch
Details | Diff | Splinter Review
5.37 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

4 years ago
Priority: -- → P1
Assignee: nobody → oogunsakin
(Reporter)

Comment 1

4 years ago
Some ideas of things that would be good here:

* How often users swipe to different panels
* Whether or not users have changed their default home config
* How many default panels users have disabled
* How many dynamic panels users have installed
* Swipes until destination would be a valuable metric to correlate against usage.
* Interactions with each panel.
Given the ideas in comment 1 and comment 2, what data actually needs to be collected? Knowing that the UI Telemetry system is storing "event", "timeoffset", and "sessions" we might be able to post-analyze the telemetry if we collected:
1. start/stop home session
2. show panel event (passing the panel ID?)

Given this, we might be able to analyze the data to see:
* first "show panel" event in a "home" session to find the default panel.
* number of "show panel" events in a "home" session to find how much the user moves around before making an interaction.

I think we should use other bugs to track:
* interactions with each panel

We could probably use the old fashioned telemetry to track:
* how many default panels users have disabled
* how many dynamic panels users have installed

Thoughts?
Blocks: 942279
No longer depends on: 942279
(Assignee)

Comment 4

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)
> 2. show panel event (passing the panel ID?)
We could make this a session. That way we can track how long a user spends in there.
(Assignee)

Comment 5

4 years ago
Created attachment 8388924 [details] [diff] [review]
WIP: About Home Session

Session for about:home
Attachment #8388924 - Flags: feedback?(liuche)
(Assignee)

Comment 6

4 years ago
Created attachment 8388926 [details] [diff] [review]
WIP: Sessions for each panel
Attachment #8388926 - Flags: feedback?(liuche)
(Assignee)

Comment 7

4 years ago
Created attachment 8388927 [details] [diff] [review]
WIP: Panels settings probes

Probes for answering the following questions:
* how many default panels users have disabled
* how many dynamic panels users have installed

sends events when a panel is disabled or set as default
Attachment #8388927 - Flags: feedback?(liuche)
(Assignee)

Comment 8

4 years ago
I think we should consider making the extras field an object of some sort (Map, ContentValues or JSONObject). Its seems a little messy making comma separated, key-value pair strings and it could be hard to enforce the format.
Comment on attachment 8388924 [details] [diff] [review]
WIP: About Home Session

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

Good start, let's try to keep these probes very simple, so it's easier to post-analyze and aggregate information.

::: mobile/android/base/Telemetry.java
@@ +107,5 @@
>          }
>      }
>  
>      public static void startUISession(String sessionName) {
> +        Log.d(LOGTAG, "StartUISession: name=" + sessionName);

Can probably omit "name".

@@ +113,5 @@
>          GeckoAppShell.sendEventToGecko(event);
>      }
>  
>      public static void stopUISession(String sessionName, String reason) {
> +        Log.d(LOGTAG, "StopUISession: name=" + sessionName + ", reason=" + reason);

same.

@@ +148,5 @@
> +        public static final String HOME = "home";
> +    }
> +
> +    public static class Reason {
> +        public static final String USER_LEFT_HOME_PAGE = "user_left_home_page";

This string is probably too long - since this is already associated with the home session, it's unnecessary to include user, home, or page. How about just exit?

Or if you want to be more specific, we could use different reasons for different means of exiting the home panels (tapping a link, typing a url, opening a tab, etc). That way we get more information out of this probe.

So, like we discussed today, let's try to keep all these strings short, more like tags than sentences. When in doubt, try to weigh whether each part of the string contributes new, *useful* information.

::: mobile/android/base/home/HomePager.java
@@ +205,5 @@
>      public void unload() {
>          mLoaded = false;
>          setAdapter(null);
> +        Telemetry.stopUISession(Telemetry.Session.HOME,
> +                                Telemetry.Reason.USER_LEFT_HOME_PAGE);

This is probably fine for a first pass (with a generic reason like "exit") but it would also be good to move this session end to a more specific place (see previous comment).
Attachment #8388924 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8388926 [details] [diff] [review]
WIP: Sessions for each panel

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

::: mobile/android/base/Telemetry.java
@@ +119,5 @@
>          GeckoAppShell.sendEventToGecko(event);
>      }
>  
>      public static void sendUIEvent(String action, String method, long timestamp, String extras) {
> +        Log.d(LOGTAG, "SendUIEvent: action = " + action + " method = " + method + " timestamp = " + timestamp + " extras = " + extras);

Eventually, these should be removed, I think - there's no need to keep track of every probe we add in the logs. This is definitely useful for making sure you cover all the right places to add events/sessions, though.

@@ +151,5 @@
>      }
>  
>      public static class Reason {
>          public static final String USER_LEFT_HOME_PAGE = "user_left_home_page";
> +        public static final String USER_SWITCHED_TO_OTHER_PANEL = "user_switched_to_other_panel";

Again, see comments on previous patch - this is pretty wordy. Maybe just "change" - if you want to be fancy, you could have different events for "swipe" and "click" (tablet-only).

::: mobile/android/base/home/HomePager.java
@@ +209,5 @@
>          mLoaded = false;
>          setAdapter(null);
> +        if (mCurrentPanelSession != null) {
> +            Telemetry.stopUISession(mCurrentPanelSession,
> +                                    Telemetry.Reason.USER_LEFT_HOME_PAGE);

Clear the session after you've stopped it.

@@ +356,5 @@
>              if (mHomeBanner != null) {
>                  mHomeBanner.setActive(position == mDefaultPageIndex);
>              }
> +
> +            // Stop current panel session if we have one.

Be more specific, that we're ending the session because the user has left the panel. "Panel session" might be ambiguous.

@@ +368,5 @@
> +            if (newPanelId != null) {
> +                mCurrentPanelSession = Telemetry.Session.HOME_PANEL + ":" + newPanelId;
> +                Telemetry.startUISession(mCurrentPanelSession);
> +            } else {
> +                mCurrentPanelSession = null;

I'd rather you set mCurrentPanelSession null after you stop the session, instead of here, because the null-setting should be associated with stopping a session, not on-session-start-but-if-there's-no-panel-id.
Attachment #8388926 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8388927 [details] [diff] [review]
WIP: Panels settings probes

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

I'd prefer to use JSON or something more ordered for extras, just so that it's consistent. This is something that you should add to the Telemetry.java module, because it'll be shared.

::: mobile/android/base/Telemetry.java
@@ +144,5 @@
>      public static class Event {
> +        public static final String PANEL_ENABLE = "enablepanel";
> +        public static final String PANEL_DISABLE = "disablepanel";
> +        public static final String PANEL_INSTALL = "installpanel";
> +        public static final String PANEL_MAKE_DEFAULT = "makedefault";

How about a more hierarchical naming: panel-enable, panel-install, panel-setdefault, etc?

::: mobile/android/base/home/HomeConfig.java
@@ +267,5 @@
>          public boolean isDynamic() {
>              return (mType == PanelType.DYNAMIC);
>          }
>  
> +        public boolean isBuiltIn() {

You should use isDynamic here.

::: mobile/android/base/home/HomePanelPicker.java
@@ +24,5 @@
>  import java.util.Arrays;
>  import java.util.List;
>  
>  import org.mozilla.gecko.R;
> +import org.mozilla.gecko.Telemetry;

Our import ordering is a little inconsistent, but I think in most cases, we do alpha-order before case. Double check to see what other files do.

@@ +137,5 @@
>          final PanelConfig newPanelConfig = panelInfo.toPanelConfig();
>          HomeConfigInvalidator.getInstance().installPanel(newPanelConfig);
>  
> +        final String extras = "panel_id=" + panelInfo.getId() + "," +
> +                              "is_built_in=" + panelInfo.toPanelConfig().isBuiltIn();

I'm not sure that comma-separated extras is the best way to do this - maybe a JSON string is better. The downside is that 1) perf and 2) we'll need to parse JSON every time we want to get to any of these extras.

Alternatively, we could come up with some kind of simple, explicitly documented data format - e.g., every key-value pair is terminated by a comma. Basically, something that is dead-easy to parse. What do you think, Sola?

::: mobile/android/base/preferences/PanelsPreference.java
@@ +22,5 @@
>      private String LABEL_HIDE;
>      private String LABEL_SHOW;
>  
>      protected boolean mIsHidden = false;
> +    protected boolean mIsBuiltIn = false;

This patch is bit-rotted - you should co-opt the mIsRemovable boolean for this (rename it, though).

@@ +118,5 @@
>      public boolean isHidden() {
>          return mIsHidden;
>      }
> +
> +    public void setIsBuiltIn(boolean isBuiltIn) {

Don't need this, anymore.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +122,5 @@
>  
>          mConfigEditor.setDefault(id);
>          mConfigEditor.apply();
> +        PanelsPreference panelPref = (PanelsPreference) pref;
> +        final String extras = "panel_id=" + panelPref.getKey() + "," +

Yeah, seeing this now, string concatenations look really janky. Either abstract this out to a function that uses StringBuilders (why not) and takes Params... or use JSON. In any case, this should be in Telemetry.java.

@@ +156,5 @@
>          mConfigEditor.apply();
>  
>          pref.setHidden(toHide);
>          setDefaultFromConfig();
> +        final String event = toHide ? Telemetry.Event.PANEL_DISABLE:

Add a very short comment to this section detailing that this is for telemetry.

@@ +164,5 @@
> +        Telemetry.sendUIEvent(event, Telemetry.Method.SETTINGS, extras);
> +    }
> +
> +    /**
> +     * Get current panel configuration status as a string.

Be more specific, panel configuration "status" is not very descriptive.
Attachment #8388927 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8388926 [details] [diff] [review]
WIP: Sessions for each panel

I know it might not be possible, but I'd like to not add variables to track session/telemetry. Do we need mCurrentPanelSession? Can't we get that any other way? Don't we already have the current panel id somewhere?

I also like less specific reasons: "switched" or "changed" could be used for tabs as well as panels. We should be able to depend on other data to get more of the context.

Thinking out loud:
I think sessions are only written out when the session ends. If data looks like this, we might have trouble analyzing it to find the session we are moving to:

{ session: "topsites", reason: "switched" }
... other sessions and events ...
{ session: "history", reason: ... }

If we treated the switch as an event the data would look like:

{ session: "topsites", reason: "switched" }
{ event: "switch-panel", session: "home", extra: "history" }
... other sessions and events ...
{ session: "history", reason: ... }

this assumes we fire the event after ending the "topsite" session and before starting the "history" session. If we fired the event after starting the "history" session, it would look like:

{ session: "topsites", reason: "switched" }
{ event: "switch-panel", session: "home,history", extra: "history" }
... other sessions and events ...
{ session: "history", reason: ... }

Like I said, just thinking...
Comment on attachment 8388927 [details] [diff] [review]
WIP: Panels settings probes

>     private void installNewPanelAndQuit(PanelInfo panelInfo) {
>         final PanelConfig newPanelConfig = panelInfo.toPanelConfig();
>         HomeConfigInvalidator.getInstance().installPanel(newPanelConfig);
> 
>+        final String extras = "panel_id=" + panelInfo.getId() + "," +
>+                              "is_built_in=" + panelInfo.toPanelConfig().isBuiltIn();
>+        Telemetry.sendUIEvent(Telemetry.Event.PANEL_INSTALL,
>+                              Telemetry.Method.SETTINGS,
>+                              extras);

More thinking. Do we really need "is_built_in" ? We already know the IDs of the built-ins and can use that in the post-processing. We should only need a single extra here: panelid

>diff --git a/mobile/android/base/preferences/PanelsPreferenceCategory.java b/mobile/android/base/preferences/PanelsPreferenceCategory.java

>+        PanelsPreference panelPref = (PanelsPreference) pref;
>+        final String extras = "panel_id=" + panelPref.getKey() + "," +
>+                              "is_built_in=" + panelPref.isBuiltIn();
>+        Telemetry.sendUIEvent(Telemetry.Event.PANEL_MAKE_DEFAULT,
>+                              Telemetry.Method.SETTINGS, extras);

Same

>+        final String event = toHide ? Telemetry.Event.PANEL_DISABLE:
>+                                      Telemetry.Event.PANEL_ENABLE;
>+        final String extras = "panel_id=" + pref.getKey() + "," +
>+                              "is_built_in=" + pref.isBuiltIn() + "," + getCurrentConfigStatus();
>+        Telemetry.sendUIEvent(event, Telemetry.Method.SETTINGS, extras);

Same, minus getCurrentConfigStatus (see below)

>+    private String getCurrentConfigStatus() {
>+        // First preference (index 0) is Preference to add panels.
>+        final int numPanels = getPreferenceCount() - 1;
>+        int numPanelsDisabled = 0;
>+        int numDynamicPanels = 0;
>+        for (int i = 1; i <= numPanels; i++) {
>+            final PanelsPreference pref = (PanelsPreference) getPreference(i);
>+            if (!pref.isEnabled()) {
>+                numPanelsDisabled += 1;
>+            }
>+
>+            if (!pref.isBuiltIn()) {
>+                numDynamicPanels += 1;
>+            }
>+        }
>+        return "num_panels=" + numPanels + "," +
>+               "num_dyanmic_panels=" + numDynamicPanels + "," +
>+               "num_panels_disabled=" + numPanelsDisabled;

I don't think we should send "counts" back with each these events. I would want to know exactly how we expect use this data first. I don't see a need to send the counts like this. We could use the old Telemetry system for counts. We could use the simple measures part of UI Telemetry:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/UITelemetry.jsm#194
(In reply to Mark Finkle (:mfinkle) from comment #13)

> I don't think we should send "counts" back with each these events. I would
> want to know exactly how we expect use this data first. I don't see a need
> to send the counts like this.

Concur. It's a lot of data to sift through, particularly parsing kinda-free-form text (with spelling errors!) without an obvious goal.

If we care about users disabling panels (we do), we can answer that question from those events.

If we care where they're coming from when they switch panels, we can answer that with the transition event.

If we're trying to bucket users based on whether they've customized panels -- e.g., do those users switch panels more or less? -- let's do that explicitly: make that an extant session.

I'd like to keep us away from putting too much in event extras; it's a bad habit to get into. Make a session, or a well-chosen event, and think carefully about what exactly you're denoting.
(In reply to Mark Finkle (:mfinkle) from comment #12)

> I know it might not be possible, but I'd like to not add variables to track
> session/telemetry. Do we need mCurrentPanelSession? Can't we get that any
> other way? Don't we already have the current panel id somewhere?

The idiomatic way to do this, IMO:

* When you switch from panel A to panel B, end the "in panel A" session and start the "in panel B" session.
* When you switch from panel A to panel B, record an "I just switched panels" event.

That gives you context for future events, timestamps, and everything else you might need.

The important point here: the app doesn't need to track much state. The active sessions in UITelemetry track that state.

See Bug 978994 comment 3 for more thoughts.
(Assignee)

Comment 16

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #11)
> I'm not sure that comma-separated extras is the best way to do this - maybe
> a JSON string is better. The downside is that 1) perf and 2) we'll need to
> parse JSON every time we want to get to any of these extras.
> 
> Alternatively, we could come up with some kind of simple, explicitly
> documented data format - e.g., every key-value pair is terminated by a
> comma. Basically, something that is dead-easy to parse. What do you think,
> Sola?
> 
Comma-separated key-value pairs sound good but we might run into trouble with escaping special characters. JSON handles that.
(Assignee)

Comment 17

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 8388926 [details] [diff] [review]
> WIP: Sessions for each panel
> 
> I know it might not be possible, but I'd like to not add variables to track
> session/telemetry. Do we need mCurrentPanelSession? Can't we get that any
> other way? Don't we already have the current panel id somewhere?
> 

Currently we don't store the current panel id. HomePager.getCurrentItem() returns the current page/panel but when we are notified a panel change, getCurrentItem() already reflects the newly selected page not the previous one. Not sure there is an alternative to using a member variable to track the previous panel/session. 

> Thinking out loud:
> I think sessions are only written out when the session ends. If data looks
> like this, we might have trouble analyzing it to find the session we are
> moving to:
> 
> If we treated the switch as an event the data would look like:
> 
> { session: "topsites", reason: "switched" }
> { event: "switch-panel", session: "home", extra: "history" }
> ... other sessions and events ...
> { session: "history", reason: ... }
> 
Sounds like good plan!

It would be nice if we could somehow capture it in the reason tag ... "User left some panel X because they 'switched-to-panel-Y'". Just a thought.
(In reply to Sola Ogunsakin [:sola] from comment #17)

> Sounds like good plan!
> 
> It would be nice if we could somehow capture it in the reason tag ... "User
> left some panel X because they 'switched-to-panel-Y'". Just a thought.

Some of this is captured in the modeling suggestions in Bug 978994.
(Assignee)

Comment 19

4 years ago
Created attachment 8394446 [details] [diff] [review]
Sessions for each panel
Attachment #8388926 - Attachment is obsolete: true
Attachment #8394446 - Flags: feedback?(liuche)
(Assignee)

Comment 20

4 years ago
Created attachment 8394447 [details] [diff] [review]
Panels settings probes
Attachment #8388927 - Attachment is obsolete: true
Attachment #8394447 - Flags: feedback?(liuche)
(Assignee)

Comment 21

4 years ago
Created attachment 8394966 [details] [diff] [review]
Panels settings probes

Previous patch didn't have the constants in it. My bad.
Attachment #8394447 - Attachment is obsolete: true
Attachment #8394447 - Flags: feedback?(liuche)
Attachment #8394966 - Flags: feedback?(liuche)
(Assignee)

Comment 22

4 years ago
Created attachment 8395067 [details] [diff] [review]
About Home Session

added comments
Attachment #8388924 - Attachment is obsolete: true
Attachment #8395067 - Flags: feedback?(liuche)
(Assignee)

Comment 23

4 years ago
Created attachment 8395068 [details] [diff] [review]
Panels settings probes

added comments
Attachment #8394966 - Attachment is obsolete: true
Attachment #8394966 - Flags: feedback?(liuche)
Attachment #8395068 - Flags: feedback?(liuche)
(Assignee)

Comment 24

4 years ago
Created attachment 8395069 [details] [diff] [review]
Panels settings probes
Attachment #8395068 - Attachment is obsolete: true
Attachment #8395068 - Flags: feedback?(liuche)
Attachment #8395069 - Flags: feedback?(liuche)
Comment on attachment 8395069 [details] [diff] [review]
Panels settings probes

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

Looks good, I like the event names now.

However, it occurs to me that these events are missing any panel events that add-ons might make (auto-installing a panel, for example, from Home.jsm). HomeConfig handles all the panel changes though, so you should move these events there.

This changes the "settings" method, so instead of making a the events "settings" method events, you should have a "settings-homepanels" session, so we know which events happen through the Settings UI.

::: mobile/android/base/Telemetry.java
@@ +159,5 @@
> +        // as "panel-install:<panel_id>"
> +        public static final String PANEL_INSTALL = "panel-install";
> +
> +        // Occurs when a home panel is made default. Dynamic, encoded
> +        // as "panel-makedefault:<panel_id>"

I think these comments are too similar to warrant separate comments. In general, if you're doing lots of copy-paste, think twice.

The useful thing is the encoding, but this is the same for all of them, so one comment for the "panel actions events" is sufficient.

@@ +160,5 @@
> +        public static final String PANEL_INSTALL = "panel-install";
> +
> +        // Occurs when a home panel is made default. Dynamic, encoded
> +        // as "panel-makedefault:<panel_id>"
> +        public static final String PANEL_MAKE_DEFAULT = "panel-makedefault";

Use "panel-setdefault" to be consistent with our code base (see the methods, comments, where we use set-default).

::: mobile/android/base/home/HomePanelPicker.java
@@ +25,5 @@
>  import java.util.Arrays;
>  import java.util.List;
>  
>  import org.mozilla.gecko.R;
> +import org.mozilla.gecko.Telemetry;

Nit: Check the resource ordering again. I think we do alpha ordering, then case (but you should check other files!)

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +6,5 @@
>  
>  import java.util.ArrayList;
>  import java.util.List;
>  
> +import org.mozilla.gecko.Telemetry;

Nit: same, alpha/casing.

@@ +140,5 @@
>          mConfigEditor.apply();
> +
> +        // UI Telemetry.
> +        PanelsPreference panelPref = (PanelsPreference) pref;
> +        final String event = Telemetry.Event.PANEL_MAKE_DEFAULT + ":" + panelPref.getKey();

You don't need to cast here, Preference has the getKey() method.
Attachment #8395069 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8395067 [details] [diff] [review]
About Home Session

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

Good. Make sure to lift out the Telemetry Events/Sessions into a constants class.

::: mobile/android/base/Telemetry.java
@@ +139,5 @@
>      public static void sendUIEvent(String action) {
>          sendUIEvent(action, null, realtime(), null);
>      }
> +
> +    public static class Event {

Richard points out that we should have these (Event, Session, Reason) in a different constants class (like BrowserContract), to avoid mixing the Telemetry implementation and its data descriptors.

And in that class, add comments about the intent for each of these classes (Event, Session, Reason) and maybe how they relate/interact, so someone adding probes for the first time can have the context for how they're used, and come up with good names.

::: mobile/android/base/home/HomePager.java
@@ +9,5 @@
>  import java.util.EnumSet;
>  import java.util.List;
>  
>  import org.mozilla.gecko.R;
> +import org.mozilla.gecko.Telemetry;

Nit: alpha vs case ordering I think we do alpha and then case, not the other way around. You might want to update your Eclipse auto-formats.
Attachment #8395067 - Flags: feedback?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #26)

> Richard points out that we should have these (Event, Session, Reason) in a
> different constants class (like BrowserContract), to avoid mixing the
> Telemetry implementation and its data descriptors.

N.B., similar to BrowserContract, not in BrowserContract. I suggest "BrowserEvents", but feel free to have better ideas :D
(Assignee)

Comment 28

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #25)
> ::: mobile/android/base/Telemetry.java
> @@ +159,5 @@
> > +        // as "panel-install:<panel_id>"
> > +        public static final String PANEL_INSTALL = "panel-install";
> > +
> > +        // Occurs when a home panel is made default. Dynamic, encoded
> > +        // as "panel-makedefault:<panel_id>"
> 
> I think these comments are too similar to warrant separate comments. In
> general, if you're doing lots of copy-paste, think twice.
> 
> The useful thing is the encoding, but this is the same for all of them, so
> one comment for the "panel actions events" is sufficient.
> 
they look similar but don't we need to indicate which event names are dynamic and in what way. 
it could be helpful for the server-side phase. that way you know what you're looking for in the data.
> ::: mobile/android/base/home/HomePanelPicker.java
> @@ +25,5 @@
> >  import java.util.Arrays;
> >  import java.util.List;
> >  
> >  import org.mozilla.gecko.R;
> > +import org.mozilla.gecko.Telemetry;
> 
> Nit: Check the resource ordering again. I think we do alpha ordering, then
> case (but you should check other files!)
> 
believe we currently do case then alpha. 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#15
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#20
(Assignee)

Comment 29

4 years ago
Created attachment 8395120 [details] [diff] [review]
About Home Session

Move constants over to TelemetryContract
Attachment #8395067 - Attachment is obsolete: true
Attachment #8395120 - Flags: feedback?(liuche)
(Assignee)

Comment 30

4 years ago
Created attachment 8395121 [details] [diff] [review]
Sessions for each panel

Address comments. Move constants over TelemetryContract
Attachment #8394446 - Attachment is obsolete: true
Attachment #8394446 - Flags: feedback?(liuche)
Attachment #8395121 - Flags: feedback?(liuche)
Comment on attachment 8394446 [details] [diff] [review]
Sessions for each panel

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

::: mobile/android/base/Telemetry.java
@@ +141,5 @@
>          sendUIEvent(action, null, realtime(), null);
>      }
>  
>      public static class Event {
> +        public static final String PANEL_SWITCH = "panel-switch";

I would prefer panel.switch.

@@ +151,4 @@
>      }
>  
>      public static class Reason {
>          public static final String EXIT = "exit";

I would prefer something less conflated with actually exiting the app.

How about "navigate.{up,away,back}"?

That leaves us "finish" for ending an activity, and "exit" for quitting the app.

@@ +151,5 @@
>      }
>  
>      public static class Reason {
>          public static final String EXIT = "exit";
> +        public static final String CHANGED = "changed";

"panel.changed"?
Attachment #8394446 - Attachment is obsolete: false
(Assignee)

Comment 32

4 years ago
Created attachment 8395122 [details] [diff] [review]
Panels settings probes

Address comments. Move constants over TelemetryContract
Attachment #8395069 - Attachment is obsolete: true
Attachment #8395122 - Flags: feedback?(liuche)
Comment on attachment 8395120 [details] [diff] [review]
About Home Session

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

Nits: Also be sure to include rnewman's comments from before.

::: mobile/android/base/TelemetryContract.java
@@ +5,5 @@
> +
> +package org.mozilla.gecko;
> +
> +/**
> + * Holds data definition for our UI Telemetry implementation.

Nit: "definitions for UI Telemetry implementation"

@@ +6,5 @@
> +package org.mozilla.gecko;
> +
> +/**
> + * Holds data definition for our UI Telemetry implementation.
> + * Will eventually be shared with a server-side.

What does this mean?

@@ +28,5 @@
> +        public static final String HOME = "home";
> +    }
> +
> +    /**
> +     * Holds event names. Intended for use in

Nit: copy-pasta mistake!
Attachment #8395120 - Flags: feedback?(liuche)
(Assignee)

Comment 34

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #33)
> @@ +6,5 @@
> > +package org.mozilla.gecko;
> > +
> > +/**
> > + * Holds data definition for our UI Telemetry implementation.
> > + * Will eventually be shared with a server-side.
> 
> What does this mean?

meant that event/session/reason definitions here will also be used by a server side when analyzing the data. Seems a bit confusing now, i'll take it out :)
(Assignee)

Comment 35

4 years ago
Created attachment 8395129 [details] [diff] [review]
Sessions for each panel

Update naming
Attachment #8394446 - Attachment is obsolete: true
Attachment #8395121 - Attachment is obsolete: true
Attachment #8395121 - Flags: feedback?(liuche)
Attachment #8395129 - Flags: feedback?(rnewman)
(Assignee)

Comment 36

4 years ago
Created attachment 8395130 [details] [diff] [review]
Panels settings probes

Update naming
Attachment #8395122 - Attachment is obsolete: true
Attachment #8395122 - Flags: feedback?(liuche)
Attachment #8395130 - Flags: feedback?(rnewman)
Comment on attachment 8395129 [details] [diff] [review]
Sessions for each panel

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

The big thing not covered in my in-line comments: versioning.

UITelemetry events and sessions themselves aren't versioned; it needs to be part of the payload. That is, if in Firefox 31 we alter the definition of navigation, or the bookmarks panel, or whatever, we should have an idea of how we're going to differentiate between "panel.bookmarks" (the old one) and "panel.bookmarks" (the new one).

Please spend some time thinking about that (and discussing with me or Chenxia if necessary) before moving forward with this bug.

::: mobile/android/base/TelemetryContract.java
@@ +10,3 @@
>   * Will eventually be shared with a server-side.
>   */
>  public class TelemetryContract {

interface?

@@ +14,5 @@
>      /**
>       * Holds event names. Intended for use with
>       * Telemetry.sendUIEvent() as the "action" parameter.
>       */
>      public static class Event {

interface?

@@ +19,5 @@
> +        // Fired when a user transitions between home panels. Event
> +        // occurs after the previous home panel session is closed and
> +        // before the new home panel session is started. Encoded as
> +        // "panel.switch:<panel_id>"
> +        public static final String PANEL_SWITCH = "panel.switch";

It would seem more useful to have this inside one of the two panel sessions, either [<old> TO:new][<new> ...] or [<old>][<new> FROM:old]

And the former makes sense to me: after all, they're starting the switch from the old session, no?

::: mobile/android/base/home/HomePager.java
@@ +210,5 @@
>          mLoaded = false;
>          setAdapter(null);
>          Telemetry.stopUISession(TelemetryContract.Session.HOME,
> +                                TelemetryContract.Reason.NAVIGATE_AWAY);
> +        stopCurrentPanelTelemetrySession(TelemetryContract.Reason.NAVIGATE_AWAY);

The HOME session should be ended *after* the current panel session.

But: are you sure that unload() is always called immediately after the user stops looking at the HomePager?

IIRC, it sticks around for a really long time. That means this is probably the wrong place for either of these events.

@@ +406,5 @@
> +     *
> +     * @param panelId of panel to start a session for
> +     */
> +    private void startPanelTelemetrySession(String panelId) {
> +        assert mCurrentPanelSession == null : "Must stop current panel telemetry session before starting a new one.";

Do a normal check and throw a RuntimeException.

@@ +417,5 @@
> +     *
> +     * @param reason for stopping the telemetry session
> +     */
> +    private void stopCurrentPanelTelemetrySession(String reason) {
> +        if (mCurrentPanelSession != null) {

You should probably log or throw if it's null.

@@ +419,5 @@
> +     */
> +    private void stopCurrentPanelTelemetrySession(String reason) {
> +        if (mCurrentPanelSession != null) {
> +            Telemetry.stopUISession(mCurrentPanelSession, reason);
> +            mCurrentPanelSession = null;

Have you analyzed the threading model of access to (a) Telemetry, and (b) mCurrentPanelSession?

I'm concerned about races here. It's utterly unsafe to modify or inspect mCurrentPanelSession from more than one thread, so which thread owns it? And if it's the UI thread, are we going to be janking by starting and stopping sessions on that thread?
Attachment #8395129 - Flags: feedback?(rnewman)
Comment on attachment 8395130 [details] [diff] [review]
Panels settings probes

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

::: mobile/android/base/TelemetryContract.java
@@ +71,5 @@
>      public static class Method {
>          // Event occured via the home panels.
>          public static final String HOME = "home";
> +
> +        // Event occured via the Settings.

via Settings.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +135,5 @@
>      public void installPanel(PanelConfig panelConfig) {
>          Log.d(LOGTAG, "installPanel: " + panelConfig.getTitle());
>          handlePanelInstall(panelConfig, InvalidationMode.IMMEDIATE);
> +
> +        // UI Telemetry.

Zero-value comment.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +140,5 @@
>          mConfigEditor.setDefault(id);
>          mConfigEditor.apply();
> +
> +        // UI Telemetry.
> +        final String event = TelemetryContract.Event.PANEL_SET_DEFAULT + ":" + pref.getKey();

We have a very clear pattern forming here: "domain.event:subject".

I am in two minds about whether we're better served by having a generic event with an explicit named extra, or including the ID in the event.

Historical precedent says the former (observer notifications with a subject), but perhaps we'll typically want to query for a particular panel, in which case the latter is easier.

Please consult with Ian and Deb, see what kind of analyses they anticipate.
Attachment #8395130 - Flags: feedback?(rnewman) → feedback+
(In reply to Richard Newman [:rnewman] from comment #37)

> The big thing not covered in my in-line comments: versioning.
> 
> UITelemetry events and sessions themselves aren't versioned; it needs to be
> part of the payload. That is, if in Firefox 31 we alter the definition of
> navigation, or the bookmarks panel, or whatever, we should have an idea of
> how we're going to differentiate between "panel.bookmarks" (the old one) and
> "panel.bookmarks" (the new one).


Chenxia and I talked about this a bit. I ended up thinking that we should not include an explicit version in the payload. If "panel.bookmarks" changes in some way that breaks past analyses, it should change it's name, in a way similar to string entities, only for a different reason.

"panel.bookmarks" -> "panel.bookmarks2" for lack of any creativity

Adding versions to all payloads adds too much to the payload itself. Smedberg has already asked us to keep the payload small. I think changing the name should be enough for breaking format changes. The analysis scripts should be able to support defense against small changes or additions to the payload.
Comment on attachment 8395129 [details] [diff] [review]
Sessions for each panel


>diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java

>     public static class Event {
>-
>+        // Fired when a user transitions between home panels. Event
>+        // occurs after the previous home panel session is closed and
>+        // before the new home panel session is started. Encoded as
>+        // "panel.switch:<panel_id>"
>+        public static final String PANEL_SWITCH = "panel.switch";

This not what I had in mind. "panel.switch" is a good event name, but let's not encode it with the panel_id. That is exactly what the "extra" param was intended to support: a single breadcrumb, but I really wanted Session to do the work so we would not need to use extras. We need the events to be a bit generic and use the sessions/context to add specifics. The act of "switching" might force us to use an extra since a switch means we are moving from state1 to state2 and we probably want easy access to both states. So for a panel switch:

<fennec loads and opens about:home>
1. start HOME session (nothing written to telemetry)
2. start TOPSITES session (nothing written to telemetry)
<user moves to HISTORY>
3. event PANEL_SWITCH, sessions: [HOME, TOPSITES], extra: HISTORY (written to telemetry)
4. end TOPSITES session (written to telemetry)
5. start HISTORY session (nothing written to telemetry)

I think we need to include the panel_id with the PANEL_SWITCH event since we won't be able to scan the telemetry data for the "next session" and see it is HISTORY.

>     public static class Session {
>         // Started when a user enters about:home.
>         public static final String HOME = "home";
>+
>+        // Started when a user enters a given home panel.
>+        // Session name is dynamic, encoded as "homepanel:<panel_id>"
>+        public static final String HOME_PANEL = "homepanel";

I am OK with encoding the panel_id in the session name since it's core to the session/context. In my example above, TOPSITES and HISTORY would be "homepanel:topsitesid" and "homepanel:historyid" since the session/context is supposed to be somewhat specific. We need it to be to give the events better context.

>     public static class Reason {
>-        public static final String EXIT = "exit";
>+        // User navigated away.
>+        public static final String NAVIGATE_AWAY = "navigate.away";
>+
>+        // User moved to another home panel.
>+        public static final String CHANGED_PANELS = "panel.changed";

If these reasons add too much grief in trying to get "right" and placed in the code, then we need to ask "Are they helping with analysis?" If not, then we drop them. Let's not add anything that makes life harder to implement and does not provide known value.

Think about that as working on patches. You feedback on the complexity is valuable. Complexity without benefit means we drop the complexity.

>+    public static class Method {
>+        // Event occured via the home panels.
>+        public static final String HOME = "home";
>+     }

The intention for "methods" was to give context to how an event was fired: mainmenu, contextmenu, swipe, or empty if we can't give meaningful context. Using "home" sounds like something we would get from the sessions attached to the event.

>+    /**
>+     * Start a UI telemetry session for a panel.
>+     *
>+     * @param panelId of panel to start a session for
>+     */
>+    private void startPanelTelemetrySession(String panelId) {
>+        assert mCurrentPanelSession == null : "Must stop current panel telemetry session before starting a new one.";
>+        mCurrentPanelSession = TelemetryContract.Session.HOME_PANEL + ":" + panelId;
>+        Telemetry.startUISession(mCurrentPanelSession);
>+    }
>+
>+    /**
>+     * Stop the current panel telemetry session if one exists.
>+     *
>+     * @param reason for stopping the telemetry session
>+     */
>+    private void stopCurrentPanelTelemetrySession(String reason) {
>+        if (mCurrentPanelSession != null) {
>+            Telemetry.stopUISession(mCurrentPanelSession, reason);
>+            mCurrentPanelSession = null;
>+        }
>+    }
> }

It should come as no surprise that I don't want to add this kind of complexity.
Let me make a simplification, or what I think is a simplification:
* Do not track "switch" as an event.

Let's make this bug ONLY about creating sessions for the main HOME page and the inner panels. Do not worry about an event in this bug at all.

We can look at the session data to find out how many times a new session "homepanel:panel_id" shows up. We can look for *other* events that are tagged with the session to answer "did the user do anything in the panel?" We can use the time duration to answer "how long did the user spend in the panel?"

Let's make this more simple than we have it now, OK?
Comment on attachment 8395130 [details] [diff] [review]
Panels settings probes

Yes, I am also suggesting we drop this patch. My reason:
1. This is technically Setting UI telemetry
2. No one is asking for the telemetry to answer questions 
3. If someone does ask for it, we should look for other ways to capture the data, like using simpleMeasures

I think we must fight the temptation to "INSTRUMENT ALL THE THINGS!!!!"
(In reply to Mark Finkle (:mfinkle) from comment #39)

> Chenxia and I talked about this a bit. I ended up thinking that we should
> not include an explicit version in the payload. If "panel.bookmarks" changes
> in some way that breaks past analyses, it should change it's name, in a way
> similar to string entities, only for a different reason.

We're using opposite terminology. By "events and sessions themselves aren't versioned", I mean "the JSON stuff that UITelemetry.jsm writes isn't versioned". By "payload" I mean "panel.bookmarks". So we're in violent agreement.

(Though we should probably include some kind of version in the raw data itself, if only at a document level, because that schema will at some point change too.)


> "panel.bookmarks" -> "panel.bookmarks2" for lack of any creativity

Yeah, that's one way to do it. Another is panel.1.bookmarks, or 1.panel.bookmarks, or panel.bookmarks, 1. Which one we choose has some impact on the 'schema' interfaces that we're defining in Java, and also some impact on how we're documenting all this.


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

> I am OK with encoding the panel_id in the session name since it's core to
> the session/context. In my example above, TOPSITES and HISTORY would be
> "homepanel:topsitesid" and "homepanel:historyid" since the session/context
> is supposed to be somewhat specific. We need it to be to give the events
> better context.

With consideration, this is also the line at which I'd draw things. Events don't get :foo, sessions do.
(Assignee)

Comment 44

4 years ago
(In reply to Richard Newman [:rnewman] from comment #37)
> ::: mobile/android/base/TelemetryContract.java
> @@ +10,3 @@
> >   * Will eventually be shared with a server-side.
> >   */
> >  public class TelemetryContract {
> 
> interface?
> 

how so? its just storing constants.

> ::: mobile/android/base/home/HomePager.java
> @@ +210,5 @@
> >          mLoaded = false;
> >          setAdapter(null);
> >          Telemetry.stopUISession(TelemetryContract.Session.HOME,
> > +                                TelemetryContract.Reason.NAVIGATE_AWAY);
> > +        stopCurrentPanelTelemetrySession(TelemetryContract.Reason.NAVIGATE_AWAY);
> 
> The HOME session should be ended *after* the current panel session.
> 
> But: are you sure that unload() is always called immediately after the user
> stops looking at the HomePager?
> IIRC, it sticks around for a really long time. That means this is probably
> the wrong place for either of these events.
> 

yeah. unload() its always called by BrowserApp.hideHomePager() which is called from updateHomePagerForTab()

> Have you analyzed the threading model of access to (a) Telemetry, and (b)
> mCurrentPanelSession?
> 

Telemetry.{stopUISession, startUISession, sendUIEvent} dont rely any class members. Changes mCurrentPanelSession can only be triggered from the UI thread. 

> I'm concerned about races here. It's utterly unsafe to modify or inspect
> mCurrentPanelSession from more than one thread, so which thread owns it? And
> if it's the UI thread, are we going to be janking by starting and stopping
> sessions on that thread?

We're essentially just sending gecko messages though.
(Assignee)

Comment 45

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #40)
> >     public static class Reason {
> >-        public static final String EXIT = "exit";
> >+        // User navigated away.
> >+        public static final String NAVIGATE_AWAY = "navigate.away";
> >+
> >+        // User moved to another home panel.
> >+        public static final String CHANGED_PANELS = "panel.changed";
> 
> If these reasons add too much grief in trying to get "right" and placed in
> the code, then we need to ask "Are they helping with analysis?" If not, then
> we drop them. Let's not add anything that makes life harder to implement and
> does not provide known value.
> 

Sounds good. I'll overload stopUISession() to make reason optional.

> >+    /**
> >+     * Start a UI telemetry session for a panel.
> >+     *
> >+     * @param panelId of panel to start a session for
> >+     */
> >+    private void startPanelTelemetrySession(String panelId) {
> >+        assert mCurrentPanelSession == null : "Must stop current panel telemetry session before starting a new one.";
> >+        mCurrentPanelSession = TelemetryContract.Session.HOME_PANEL + ":" + panelId;
> >+        Telemetry.startUISession(mCurrentPanelSession);
> >+    }
> >+
> >+    /**
> >+     * Stop the current panel telemetry session if one exists.
> >+     *
> >+     * @param reason for stopping the telemetry session
> >+     */
> >+    private void stopCurrentPanelTelemetrySession(String reason) {
> >+        if (mCurrentPanelSession != null) {
> >+            Telemetry.stopUISession(mCurrentPanelSession, reason);
> >+            mCurrentPanelSession = null;
> >+        }
> >+    }
> > }
> 
> It should come as no surprise that I don't want to add this kind of
> complexity.

Understood. However, I don't think there's much complexity though. I factored out the code to stop a panel session because it was being used in multiple places. the "if" statement in there to prevent us from recording junk.
(Assignee)

Comment 46

4 years ago
Created attachment 8396014 [details] [diff] [review]
Part 1: Make "reason" optional in stopUISession
Attachment #8396014 - Flags: feedback?(liuche)
(Assignee)

Comment 47

4 years ago
Created attachment 8396016 [details] [diff] [review]
Part 2: Session for about:home

remove non-informative reason when stopping home panel session
Attachment #8395120 - Attachment is obsolete: true
Attachment #8396016 - Flags: feedback?(liuche)
(Assignee)

Comment 48

4 years ago
Created attachment 8396020 [details] [diff] [review]
Part 2: Session for about:home

Version about:home session
Attachment #8396016 - Attachment is obsolete: true
Attachment #8396016 - Flags: feedback?(liuche)
Attachment #8396020 - Flags: feedback?(liuche)
(Assignee)

Comment 49

4 years ago
Created attachment 8396023 [details] [diff] [review]
Part 3: Sessions for each panel

* remove panel-switch event
* version panel sessions
Attachment #8395129 - Attachment is obsolete: true
Attachment #8396023 - Flags: feedback?(liuche)
(Assignee)

Comment 50

4 years ago
Comment on attachment 8395130 [details] [diff] [review]
Panels settings probes

Drop settings patch
Attachment #8395130 - Attachment is obsolete: true
(In reply to Sola Ogunsakin [:sola] from comment #44)

> > interface?
> 
> how so? its just storing constants.

Exactly. It's not something you ever want to instantiate or extend, right, so why should it be a class?

See BrowserContract for an example:

    @RobocopTarget
    public interface URLColumns {
        public static final String URL = "url";
        public static final String TITLE = "title";
    }

If you don't make it an interface, you should specify a private constructor and declare it final.


> Changes mCurrentPanelSession can only be triggered from the UI
> thread. 

Could you document/ensure this by adding

  ThreadUtils.assertOnUiThread(AssertBehavior.THROW);

?


> We're essentially just sending gecko messages though.

Question still applies. If we're just sending Gecko messages, is the UI thread the best place to do that? Just something to think about.

I notice that some callers of sendEventToGecko explicitly run on the background thread (e.g., BrowserApp#handleReader*). That worries me, because:


    @RobocopTarget
    public static void sendEventToGecko(GeckoEvent e) {
        if (GeckoThread.checkLaunchState(GeckoThread.LaunchState.GeckoRunning)) {
            notifyGeckoOfEvent(e);
            // Gecko will copy the event data into a normal C++ object. We can recycle the evet now.
            e.recycle();
        } else {
            gPendingEvents.addLast(e);
        }
    }

and

    static private LinkedList<GeckoEvent> gPendingEvents =
        new LinkedList<GeckoEvent>();

imply that two threads sending events to Gecko before GeckoThread has started will leave gPendingEvents in an inconsistent state.
(Assignee)

Comment 52

4 years ago
(In reply to Richard Newman [:rnewman] from comment #51)
> Exactly. It's not something you ever want to instantiate or extend, right,
> so why should it be a class?

alright will make it an interface then.

> > Changes mCurrentPanelSession can only be triggered from the UI
> > thread. 
> 
> Could you document/ensure this by adding
> 
>   ThreadUtils.assertOnUiThread(AssertBehavior.THROW);
> 
> ?

Good idea but it sounds like the kind of complexity that mfinkle is trying to prevent.
> 
> > We're essentially just sending gecko messages though.
> 
> Question still applies. If we're just sending Gecko messages, is the UI
> thread the best place to do that? Just something to think about.
> 
> I notice that some callers of sendEventToGecko explicitly run on the
> background thread (e.g., BrowserApp#handleReader*). That worries me, because:

I haven't noticed any performance hits. Posting to a background thread will cause issues of its own since there is no guarantee of when the code will run. mCurrentPanelSession could have changed in between.
Attachment #8396014 - Flags: feedback?(liuche) → feedback+
(Assignee)

Comment 53

4 years ago
Created attachment 8396087 [details] [diff] [review]
Part 2: Session for about:home

Make TelemetryContract and its children interfaces
Attachment #8396020 - Attachment is obsolete: true
Attachment #8396020 - Flags: feedback?(liuche)
Attachment #8396087 - Flags: feedback?(liuche)
Comment on attachment 8396020 [details] [diff] [review]
Part 2: Session for about:home

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

::: mobile/android/base/TelemetryContract.java
@@ +8,5 @@
> +/**
> + * Holds data definitions for our UI Telemetry implementation.
> + * Will eventually be shared with a server-side.
> + */
> +public class TelemetryContract {

Interface, like rnewman's comment.

@@ +22,5 @@
> +     * Telemetry.startUISession() as the "sessionName" parameter.
> +     */
> +    public static class Session {
> +        // Started when a user enters about:home.
> +        public static final String HOME = "v1.home";

If we're going to explicitly include the versions (which is fine), the top level of hierarchy should not be version. Use home.v1 instead.
Attachment #8396020 - Attachment is obsolete: false
(Assignee)

Comment 55

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #54)
> @@ +22,5 @@
> > +     * Telemetry.startUISession() as the "sessionName" parameter.
> > +     */
> > +    public static class Session {
> > +        // Started when a user enters about:home.
> > +        public static final String HOME = "v1.home";
> 
> If we're going to explicitly include the versions (which is fine), the top
> level of hierarchy should not be version. Use home.v1 instead.

figured it would be easier parse.
(Assignee)

Comment 56

4 years ago
Created attachment 8396146 [details] [diff] [review]
Part 2: Session for about:home

change v1.home to home.v1
Attachment #8396020 - Attachment is obsolete: true
Attachment #8396087 - Attachment is obsolete: true
Attachment #8396087 - Flags: feedback?(liuche)
Attachment #8396146 - Flags: feedback?(liuche)
(Assignee)

Comment 57

4 years ago
Created attachment 8396147 [details] [diff] [review]
Part 3: Sessions for each panel

change v1.homepanel to homepanel.v1
Attachment #8396023 - Attachment is obsolete: true
Attachment #8396023 - Flags: feedback?(liuche)
Attachment #8396147 - Flags: feedback?(liuche)
Comment on attachment 8396146 [details] [diff] [review]
Part 2: Session for about:home

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

::: mobile/android/base/TelemetryContract.java
@@ +6,5 @@
> +package org.mozilla.gecko;
> +
> +/**
> + * Holds data definitions for our UI Telemetry implementation.
> + * Will eventually be shared with a server-side.

I'm not sure what this comment means. Are we sharing this file server-side?
Attachment #8396146 - Flags: feedback?(liuche) → feedback+
> > + * Holds data definitions for our UI Telemetry implementation.
> > + * Will eventually be shared with a server-side.
> 
> I'm not sure what this comment means. Are we sharing this file server-side?

See Comment 34.
(Assignee)

Comment 60

4 years ago
Created attachment 8396601 [details] [diff] [review]
Part 2: Session for about:home

edit comments
Attachment #8396146 - Attachment is obsolete: true
Attachment #8396601 - Flags: feedback?(liuche)
Comment on attachment 8396147 [details] [diff] [review]
Part 3: Sessions for each panel

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

In general, when we have more than just individual calls to static Telemetry methods, it'd be good to lift out the telemetry code into private methods so that it is very clear what is telemetry and what is not.

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

Should we just include the ":" in this string? Then we don't need to do another string concat later.

::: mobile/android/base/home/HomePager.java
@@ +32,5 @@
>  import android.view.View;
>  import android.view.ViewGroup;
>  
>  public class HomePager extends ViewPager {
> +    private static final String LOGTAG = "GeckoHomePager";

Just HomePager. Generally, we match the LOGTAG to the class name.

@@ +53,5 @@
>  
>      // Cached original ViewPager background.
>      private final Drawable mOriginalBackground;
>  
> +    // Current panel telemetry session.

Nit: "Telemetry session for current panel." makes more sense.

@@ +378,5 @@
> +            // session if we have one.
> +            stopCurrentPanelTelemetrySession();
> +
> +            final String newPanelId = ((HomeAdapter) getAdapter()).getPanelIdAtPosition(position);
> +            if (newPanelId != null) {

Should the panelId ever be null? I think HomeConfig validates, so this can't happen.

@@ +380,5 @@
> +
> +            final String newPanelId = ((HomeAdapter) getAdapter()).getPanelIdAtPosition(position);
> +            if (newPanelId != null) {
> +                // Start a session for the newly selected panel.
> +                startPanelTelemetrySession(newPanelId);

Might as well lift out all this logic (stopping the old telemetry session, getting the panelId) into startPanelTelemetrySession since you're not going to use any of the intermediate variables anyways. You could optionally rename the method to startNewPanelTelemetrySession (though blah about long Java names).

@@ +406,5 @@
> +     * @param panelId of panel to start a session for
> +     */
> +    private void startPanelTelemetrySession(String panelId) {
> +        if (mCurrentPanelSession == null) {
> +            Log.e(LOGTAG, "Attemped to start a new panel telemetry session without closing the old one.");

This should be a debug-level log at most, because this will get called with a null mCurrentPanelSession every time HomePager is created.
Attachment #8396147 - Flags: feedback?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #61)

> > +    private static final String LOGTAG = "GeckoHomePager";
> 
> Just HomePager. Generally, we match the LOGTAG to the class name.

Many devs do

  adb logcat | grep Gecko

which is why we do this. One day it'll become moot when we use Logger instead of Log.

> This should be a debug-level log at most, because this will get called with
> a null mCurrentPanelSession every time HomePager is created.

Just kill the logging. Those string concats take time.
Comment on attachment 8396601 [details] [diff] [review]
Part 2: Session for about:home

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

::: mobile/android/base/TelemetryContract.java
@@ +35,5 @@
> +    /**
> +     * Holds various event methods. Intended for use in
> +     * Telemetry.sendUIEvent() as the "method" parameter.
> +     */
> +    public interface Method {}

One last nit: Move this up to be right under Event, because Method and Event are both related to sendUIEvent.
Attachment #8396601 - Flags: feedback?(liuche) → feedback+
(Assignee)

Comment 64

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #61)
> @@ +380,5 @@
> > +
> > +            final String newPanelId = ((HomeAdapter) getAdapter()).getPanelIdAtPosition(position);
> > +            if (newPanelId != null) {
> > +                // Start a session for the newly selected panel.
> > +                startPanelTelemetrySession(newPanelId);
> 
> Might as well lift out all this logic (stopping the old telemetry session,
> getting the panelId) into startPanelTelemetrySession since you're not going
> to use any of the intermediate variables anyways. You could optionally
> rename the method to startNewPanelTelemetrySession (though blah about long
> Java names).
>
 
We need the position of the new panel to get the panel id. could make it startPanelTelemetrySession(position) but then might as well pass in newPanelId.
(Assignee)

Comment 65

4 years ago
Created attachment 8396807 [details] [diff] [review]
Part 2: Session for about:home

Move Methods class
Attachment #8396601 - Attachment is obsolete: true
Attachment #8396807 - Flags: feedback?(liuche)
(Assignee)

Comment 66

4 years ago
Created attachment 8396812 [details] [diff] [review]
Part 3: Sessions for each panel

-remove error logging
-change homepanel.v1 to homepanel.v1:
-other nits
Attachment #8396147 - Attachment is obsolete: true
Attachment #8396812 - Flags: feedback?(liuche)
Comment on attachment 8396807 [details] [diff] [review]
Part 2: Session for about:home

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

Just one question - why do you use "various" in some of the comments, but not others? Unless you have a specific reason, I might take that out.

Looks good!
Attachment #8396807 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8396812 [details] [diff] [review]
Part 3: Sessions for each panel

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

Almost there.

::: mobile/android/base/home/HomePager.java
@@ +401,5 @@
> +     *
> +     * @param panelId of panel to start a session for
> +     */
> +    private void startPanelTelemetrySession(String panelId) {
> +        if (mCurrentPanelSession != null) {

Like I mentioned before, I'd like to have startPanelTelemetrySession call stopCurrentPanelTelemetrySession, rather then the two be separate, since that's the invariant you're assuming already. Might as well codify that assumption.
Attachment #8396812 - Flags: feedback?(liuche) → feedback+
(Assignee)

Comment 69

4 years ago
Created attachment 8397332 [details] [diff] [review]
Part 2: Session for about:home

edit comments
Attachment #8396807 - Attachment is obsolete: true
Attachment #8397332 - Flags: feedback?(liuche)
(Assignee)

Comment 70

4 years ago
Created attachment 8397334 [details] [diff] [review]
Part 3: Sessions for each panel

-move stopCurrentPanelTelemetrySession into startPanelTelemetrySession
Attachment #8396812 - Attachment is obsolete: true
Attachment #8397334 - Flags: feedback?(liuche)
Attachment #8397332 - Flags: feedback?(liuche) → feedback+
Comment on attachment 8397334 [details] [diff] [review]
Part 3: Sessions for each panel

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

::: mobile/android/base/home/HomePager.java
@@ +26,5 @@
>  import android.support.v4.app.LoaderManager.LoaderCallbacks;
>  import android.support.v4.content.Loader;
>  import android.support.v4.view.ViewPager;
>  import android.util.AttributeSet;
> +import android.util.Log;

This is no longer needed, right?

@@ +392,5 @@
>          public void onPageScrollStateChanged(int state) { }
>      }
> +
> +    /**
> +     * Start a UI telemetry session for the a panel.

Nit: accidentally too many words

@@ +393,5 @@
>      }
> +
> +    /**
> +     * Start a UI telemetry session for the a panel.
> +     * If there is currently a session opened for a panel,

Nit: either open, or running

@@ +398,5 @@
> +     * it will be stopped before a new one is started.
> +     *
> +     * @param panelId of panel to start a session for
> +     */
> +    private void startPanelTelemetrySession(String panelId) {

I have a very slight preference for this to be named startNewPanelTelemetrySession.
Attachment #8397334 - Flags: feedback?(liuche) → feedback+
(Assignee)

Comment 72

4 years ago
Created attachment 8397458 [details] [diff] [review]
Part 3: Sessions for each panel

-rename startPanelTelemetrySession as startNewPanelTelemetrySession
-edit comments
-removed unused log import
Attachment #8397334 - Attachment is obsolete: true
Attachment #8397458 - Flags: feedback?(liuche)
Attachment #8397458 - Flags: feedback?(liuche) → feedback+
(Assignee)

Updated

4 years ago
Attachment #8396014 - Flags: feedback+ → review?(liuche)
(Assignee)

Updated

4 years ago
Attachment #8397332 - Flags: feedback+ → review?(liuche)
(Assignee)

Updated

4 years ago
Attachment #8397458 - Flags: feedback+ → review?(liuche)
Comment on attachment 8396014 [details] [diff] [review]
Part 1: Make "reason" optional in stopUISession

One last thing: all these patches have the same patch comment, which is not useful - your attachment names are much more descriptive, so use those instead. (e.g., Part 1: Make "reason" optional in stopUISession. r=liuche)
Attachment #8396014 - Flags: review?(liuche) → review+
Comment on attachment 8397332 [details] [diff] [review]
Part 2: Session for about:home

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

Again, update the patch comment for this patch too.
Attachment #8397332 - Flags: review?(liuche) → review+
Comment on attachment 8397458 [details] [diff] [review]
Part 3: Sessions for each panel

Update patch comment here as well.
Attachment #8397458 - Flags: review?(liuche) → review+
(Assignee)

Comment 76

4 years ago
Created attachment 8397510 [details] [diff] [review]
Part 1: Make "reason" optional in stopUISession
Attachment #8396014 - Attachment is obsolete: true
(Assignee)

Comment 77

4 years ago
Created attachment 8397511 [details] [diff] [review]
Part 2: Session for about:home
Attachment #8397332 - Attachment is obsolete: true
(Assignee)

Comment 78

4 years ago
Created attachment 8397512 [details] [diff] [review]
Part 3: Sessions for each panel
Attachment #8397458 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 81

4 years ago
Created attachment 8398817 [details] [diff] [review]
Part 2: Session for about:home

change versioning format
Attachment #8397511 - Attachment is obsolete: true
(Assignee)

Comment 82

4 years ago
Created attachment 8398819 [details] [diff] [review]
Part 3: Sessions for each panel

change versioning format
Attachment #8397512 - Attachment is obsolete: true
(Assignee)

Comment 83

4 years ago
Created attachment 8398820 [details] [diff] [review]
Part 4: Fix testUITelemetry bustage
Attachment #8398820 - Flags: review?(liuche)
(Assignee)

Comment 84

4 years ago
Created attachment 8398821 [details] [diff] [review]
Part 5: Fix test_TelemetryLog.js bustage

-found that this test was also broken
Attachment #8398821 - Flags: review?(liuche)
(Assignee)

Comment 85

4 years ago
Created attachment 8398822 [details] [diff] [review]
Part 6: Update docs.r=liuche
Attachment #8398822 - Flags: review?(liuche)
Comment on attachment 8398820 [details] [diff] [review]
Part 4: Fix testUITelemetry bustage

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

Looks good!

::: mobile/android/base/tests/testUITelemetry.java
@@ +6,5 @@
>  
>  import android.util.Log;
>  
>  public class testUITelemetry extends JavascriptTest {
> +    // Prefix used to distinguish test events and session from

Nit: sessions, plural
Attachment #8398820 - Flags: review?(liuche) → review+
Attachment #8398821 - Flags: review?(liuche) → review+
Comment on attachment 8398822 [details] [diff] [review]
Part 6: Update docs.r=liuche

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

::: mobile/android/base/docs/index.rst
@@ +43,2 @@
>  
> +To stop a session call ``Telemetry.stopUISession(String sessionName, String reason)``.

Nit: comma after session
Attachment #8398822 - Flags: review?(liuche) → review+
Looks good - I assume there's a try run for these test changes that's green?
(Assignee)

Comment 89

4 years ago
(In reply to Chenxia Liu [:liuche] from comment #88)
> Looks good - I assume there's a try run for these test changes that's green?

https://tbpl.mozilla.org/?tree=Try&rev=0e861f685e08
(Assignee)

Comment 90

4 years ago
Created attachment 8399720 [details] [diff] [review]
Part 6: Update docs.r=liuche
Attachment #8398822 - Attachment is obsolete: true
(Assignee)

Comment 91

4 years ago
Created attachment 8399721 [details] [diff] [review]
Part 4: Fix testUITelemetry bustage
Attachment #8398820 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 94

4 years ago
The following changesets are now in Firefox Nightly:

> 033cd731fb46 Bug 968308 - Part 1: Make "reason" optional in stopUISession. r=liuche
> 219d43d76642 Bug 968308 - Part 2: Session for about:home. r=liuche
> a58e8a657b49 Bug 968308 - Part 3: Sessions for each panel. r=liuche
> 607347c9a202 Bug 968308 - Part 4: Fix testUITelemetry bustage. r=liuche
> 862e63e75fd3 Bug 968308 - Part 5: Fix test_TelemetryLog.js bustage. r=liuche
> 782d2a8b00cb Bug 968308 - Part 6: Update docs. r=liuche

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
(Reporter)

Comment 95

4 years ago
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.