Closed Bug 986095 Opened 10 years ago Closed 10 years ago

Add UI Telemetry for Reading List

Categories

(Firefox for Android Graveyard :: Reader View, defect)

All
Android
defect
Not set
normal

Tracking

(firefox31 fixed, firefox32 fixed)

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: liuche, Assigned: mfinkle)

References

Details

Attachments

(5 files)

Some basic things to track:
* Clicking on the Reader Mode page action
* Reader mode
* Actions from Reader Mode (Add, Reading List, etc)
Assignee: nobody → liuche
This patch adds a basic session for Reader. We only want to track a Session if the Reader viewer is in the foreground. We do not want to track all the open Reader viewers that might be in the background.

Reader.updatePageAction is called whenever the toolbar pageaction needs to be changed. This matches our Session behaivor too. If the reader icon is orange (active) we start a Session. If not, we stop the Session.
Assignee: liuche → mark.finkle
Attachment #8413958 - Flags: review?(rnewman)
Attachment #8413958 - Flags: review?(liuche)
This patch adds an Event for sharing via Reader toolbar. I added it to Java, but now I am thinking of moving it to aboutReader.js so the timestamps are based on the same relative start.

Agreed?
Attachment #8413960 - Flags: review?(rnewman)
This patch adds Events for adding and removing an article from the Reading List. I used "saveforlater.1" and "unsaveforlater.1" as the Event names since these can be used for Bookmarking too. The "reader.1" Session should be active and I throw in a "reader" extra.
Attachment #8413965 - Flags: review?(liuche)
Even though the Events and Session are used from JS for now, I added them to the Java TelemetryContract file.
Attachment #8413966 - Flags: review?(liuche)
We don't log errors for a duplication Session on startSession. Let's not log errors for missing Session on stopSession. The "reader.1" Session will be noisy otherwise.

I could have made UITelemetry.activeSessions public and done the check myself, but why bother?
Attachment #8413968 - Flags: review?(rnewman)
Attachment #8413968 - Flags: review?(rnewman) → review+
Comment on attachment 8413958 [details] [diff] [review]
uitelemetry-reader-session v0.1

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

::: mobile/android/chrome/content/browser.js
@@ +7492,5 @@
>        delete this.pageAction.id;
>      }
>  
> +    // Create a relative timestamp for telemetry
> +    let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized;

TODO: Bug XXXXXXX: get uptime from a stable source.
Attachment #8413958 - Flags: review?(rnewman) → review+
Attachment #8413960 - Flags: review?(rnewman) → review+
Comment on attachment 8413966 [details] [diff] [review]
uitelemetry-reader-contract v0.1

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

::: mobile/android/base/TelemetryContract.java
@@ +41,5 @@
> +        public static final String SAVE_FOR_LATER = "saveforlater.1";
> +
> +        // Stop holding a resource (reader, bookmark, etc) for viewing later.
> +        // Note: Only used in JavaScript for now, but here for completeness.
> +        public static final String UNSAVE_FOR_LATER = "unsaveforlater.1";

These are supposed to be being documented in m/a/b/docs/index.rst. Please do so.

If you want to feel particularly old, name the section "Data dictionary". Man, that makes me feel old.
Attachment #8413966 - Flags: review?(liuche) → review+
Status: NEW → ASSIGNED
Component: General → Reader Mode
Hardware: ARM → All
> ::: mobile/android/base/TelemetryContract.java
> @@ +41,5 @@
> > +        public static final String SAVE_FOR_LATER = "saveforlater.1";
> > +        public static final String UNSAVE_FOR_LATER = "unsaveforlater.1";

After talking to Chenxia, I changed these to "save.1" and "unsave.1" since it is more generic and works just as well with bookmarks and pdfs. We are using the Extras field to hold the extra context: "reader", "bookmark", and "pdf".
Comment on attachment 8413960 [details] [diff] [review]
uitelemetry-reader-sharing v0.1

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

::: mobile/android/base/BrowserApp.java
@@ +1212,5 @@
>                  GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>                                                Intent.ACTION_SEND, title);
> +
> +                // Context: Sharing via Reader toolbar (reader session is active)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.LIST);

Do we need this LIST method? Is this to delineate if we add sharing through the browser menu?
(In reply to Chenxia Liu [:liuche] from comment #9)
> Comment on attachment 8413960 [details] [diff] [review]
> uitelemetry-reader-sharing v0.1
> 
> Review of attachment 8413960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1212,5 @@
> >                  GeckoAppShell.openUriExternal(url, "text/plain", "", "",
> >                                                Intent.ACTION_SEND, title);
> > +
> > +                // Context: Sharing via Reader toolbar (reader session is active)
> > +                Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.LIST);
> 
> Do we need this LIST method? Is this to delineate if we add sharing through
> the browser menu?

The LIST here is the manner in which we are performing the SHARE:
* LIST = a list of sharing apps will be displayed and the user picks one. (non-QuickShare)
* BUTTON = the sharing app is displayed in a single button and the user tapped it. (QuickShare)
That said, I would be OK with removing the LIST if/when we decide how we will deal with normal vs exceptions.

In the case of SHARE, LIST is the norm and BUTTON is the exception.
In the case of LOAD_URL, LIST_ITEM is the norm and GRID_ITEM is the exception. Although, sometimes we skip setting LIST_ITEM since it's really, really the norm.

We can change this later if we decide to skip the "norm" and focus on the "exception". It won't break the telemetry data and we won't even need to increment our versioning. We'll just start ignoring the "norm" Methods in our analysis scripts.
(In reply to Mark Finkle (:mfinkle) from comment #10)

> > Do we need this LIST method? Is this to delineate if we add sharing through
> > the browser menu?
> 
> The LIST here is the manner in which we are performing the SHARE:
> * LIST = a list of sharing apps will be displayed and the user picks one.
> (non-QuickShare)
> * BUTTON = the sharing app is displayed in a single button and the user
> tapped it. (QuickShare)

Oh, I forgot that this was the Reader bug. In the Share bug, the patches use LIST for any telemetry where a SHARE displays the LIST.
Comment on attachment 8413958 [details] [diff] [review]
uitelemetry-reader-session v0.1

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

I'm seeing lots of extraneous start/stop events, but I think this is okay, and there's not really a better place to put them. UITelemetry.jsm will handle duplicate events as well.
Attachment #8413958 - Flags: review?(liuche) → review+
Comment on attachment 8413965 [details] [diff] [review]
uitelemetry-reader-saveforlater v0.1

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

Make sure to handle the long-press "save to reading list" case as well.

::: mobile/android/chrome/content/aboutReader.js
@@ +298,5 @@
>  
> +        let result = gChromeWin.Reader.READER_ADD_FAILED;
> +        if (success) {
> +          result = gChromeWin.Reader.READER_ADD_SUCCESS;
> +          UITelemetry.addEvent("saveforlater.1", "button", uptime, "reader");

Just the note to change this to "save" and "unsave"
Attachment #8413965 - Flags: review?(liuche) → review+
Comment on attachment 8413958 [details] [diff] [review]
uitelemetry-reader-session v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We miss out on some "before changes" telemetry that could show affect of Fx32 changes
Testing completed (on m-c, etc.): Working
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

(request for all patches)
Attachment #8413958 - Flags: approval-mozilla-aurora?
Attachment #8413960 - Flags: approval-mozilla-aurora?
Attachment #8413965 - Flags: approval-mozilla-aurora?
Attachment #8413966 - Flags: approval-mozilla-aurora?
Attachment #8413968 - Flags: approval-mozilla-aurora?
Attachment #8413958 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8413960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8413965 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8413966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8413968 - 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.