932092 - Add JS module for adding UI telemetry probes

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Chenxia Liu [:liuche]]

Both the desktop Australis rewrite and Firefox for Android could use some telemetry probes to anonymously measure UI usage, to give us more insight into how people are using Firefox, as well as give us data for figuring usage regressions caused by UI changes.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

(Adding Irving, who has some thoughts along these lines… :)

Comment 2

[Chenxia Liu [:liuche]]

Also linking the mobile etherpad: https://mobile.etherpad.mozilla.org/24

Comment 3

[Chenxia Liu [:liuche]]

Attachment: WIP Part 1: Basic event handling of a UiTelemetry module

A very basic mobile/android module (that needs a little work to be hooked into browser.js) that shows that single-event form of a UI telemetry event.

(Module will eventually live elsewhere, not in mobile/android, but is there for the moment for ease of building and testing.)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey liuche - is there anything I can do to help push this along?

Comment 5

[Chenxia Liu [:liuche]]

Thanks mconley!

I originally planned to use the Telemetry histograms for a first pass with simple events, but after talking to :irving and going through the Telemetry code, it looks like for each histogram, we'd need to add an entry into Histograms.json, so it's not really possible to have the UiTelemetry.jsm handle creating histograms.

This just doesn't create much win over just using the existing patterns of adding Telemetry, so I'm just going to send JSON blobs; I'm currently working on the API for adding sensible fields for the the types of data we might want to collect. This does mean additional handling of the data once it's collected though, because it won't automatically display as histograms - I'll put up a WIP later today!

Comment 6

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

We also need to make sure the UI probes make it possible for us to answer as many types of UI/UX questions as possible.

* Simple events that track an action are easy enough, and can answer questions like "How many times does some use the X action?", but we may also want to track some extra meta data for even these events. Things like a timestamp and the method of executing the action (button, menu, etc).

* We probably also need the concept of "sessions" which act as weak "envelopes" or "tags" for the events.
  * We'll want to track when the application goes to the background and comes to the foreground.
  * We'll want to track events that occur during startup or shortly after.
  * We'll want to track events that occur while the Home page is visible (or the bookmark editor or the awesomescreen)
  * Some of these sessions will overlap. Chenxia and I talked about using a "tag" idea for tagging sessions to an event. So each event could be in zero, one or many sessions at the same time.

* We have seen UX asking for ways to identify a "confused" user. I'm not sure if timestamps and sessions are enough for us to be able to do that.
  * For example, what would we look for to determine that user was confused about bookmarking? Would data that showed the same URL being bookmarked and unbookmarked within a short span be enough? If so, we'd need to hash the URL (privacy) and send it too.

Comment 7

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

Comment on attachment 830563 [details] [diff] [review]
WIP Part 1: Basic event handling of a UiTelemetry module

How do you see the code in the patch being used?

UiTelemetry seems like a service that manages collecting events, possibily sent from Java using GeckoEvents. It looks like an "event" consists of: category, data and tags.

If I wanted to create a bookmark event, would I use something like:

UiTelemetryEvent("bookmark:create", hash(url), ??)

aTags is confusing me a bit and I wonder if we could include a "method", something like:

UiTelemetryEvent("bookmark:create", "menu", hash(url))

and skip sending the tag in with the event. We could use a separate call to enable/disable sessions, which would be saved as an array of tag.

UiTelemetrySession.start("bookmark:editor")

...

UiTelemetryEvent("bookmark:update", "dialog", hash(url))

...

UiTelemetrySession.end("bookmark:editor")

This means the "bookmark:update" event would be saved with the "bookmark:editor" tag

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Chenxia Liu [:liuche] from comment #5)
> This just doesn't create much win over just using the existing patterns of
> adding Telemetry, so I'm just going to send JSON blobs; I'm currently
> working on the API for adding sensible fields for the the types of data we
> might want to collect. This does mean additional handling of the data once
> it's collected though, because it won't automatically display as histograms
> - I'll put up a WIP later today!

Probably the biggest win a module like this could bring is a centralized place for front-end hackers to add probes, and a single point from which Telemetry needs to pull simple measures from - so I do think this proposed module has value.

I think it's just a matter of having the module accept interesting things from various parts of the UI, and getting TelemetryPing.js to get the module's "simple measurements" during its own getSimpleMeasurements function. I think. This stuff is kinda new to me. :)

Comment 9

[Blake Winton (:bwinton) (:☕️)]

I suspect that if we on the desktop and mobile teams start using these kind of events, the people responsible for the server side of telemetry will happily provide us more useful kinds of dashboards which would do the additional handling of the data for us.  :)

Comment 10

[Chenxia Liu [:liuche]]

Attachment: WIP Part 1: Basic event handling of a UiTelemetry module v2

Updates this patch to track single events and sessions.

Comment 11

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

(In reply to Mike Conley (:mconley) from comment #8)

> Probably the biggest win a module like this could bring is a centralized
> place for front-end hackers to add probes, and a single point from which
> Telemetry needs to pull simple measures from - so I do think this proposed
> module has value.

I agree with using a central place to push event data to TelemetryPing, but I'd rather not hard code the probe types in a registry, like we do with the current performance probes. Unless there is a really good reason to do so.

Comment 12

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

Comment on attachment 833297 [details] [diff] [review]
WIP Part 1: Basic event handling of a UiTelemetry module v2

>diff --git a/toolkit/components/telemetry/UiTelemetry.jsm b/toolkit/components/telemetry/UiTelemetry.jsm

>+  init: function init() {
>+    Services.obs.addObserver(this, "UiTelemetry:AddSingleEvent");
>+    Services.obs.addObserver(this, "UiTelemetry:Session");
>+    this.activeSessions = [];
>+  },

At some point we'll need to call init from Android's browser.js so Java can send data to UiTelemetry. Desktop won't need to do this.

You should move the activeSession initialization out of init() and add it like this:

this.UiTelemetry =  {
  activeSessions: [],

  init: function init() {

That way we only need to call init() to get the Java messaging set up.

>+  observe: function observe(aMessage, aTopic, aData) {
>+    switch(aTopic) {
>+      case "UiTelemetry:AddSingleEvent":
>+        let args = JSON.parse(aData);
>+        this.addSingleEvent(args.action, args.method, args.extras, args.timestamp);
>+        break;
>+      case "UiTelemetry:Session":
>+        let args = JSON.parse(aData);
>+        let sessionName = args.name;
>+        let timestamp = args.timestamp;
>+        if (args.state == "start") {
>+          this.startSession(sessionName, timestamp);
>+        } else if (args.state == "stop") {
>+          this.stopSession(sessionName, timestamp);
>+        }
>+        break;
>+    }
>+  },
>+
>+  /**
>+   * Adds a single event described by an action, and the calling method. Optional
>+   * paramaters are extras and timestamp. The timestamp will be set here if it is
>+   * not passed in by the caller.
>+   */
>+  addSingleEvent: function addSingleEvent(aAction, aMethod, aExtras, aTimestamp) {
>+    let aEvent = {
>+      action: aAction,
>+      method: aMethod,
>+      extras: aExtras,
>+      timestamp: aTimstamp
>+    };

Since the event and the session are just pushed to Telemetry (and then the server), we probably need a "type" in the JS/JSON to tell them apart?

    let aEvent = {
      type: "event",
      action: aAction,

>+  startSession: function startSession(aName, aTimestamp) {
>+   let timestamp = aTimestamp || Date.now();
>+   if (this.activeSessions[aName]) {
>+    // Do not overwrite a previous event start if it already exsts.

I am OK with this assumption, but others might suggest auto-closing the existing session. There might be pros/cons to both approaches and we can talk them out.

>+  stopSession: function stopSession(aName, aTimestamp) {

>+    let aEvent = {
>+      name: aName,
>+      timestamp_start: sessionStart,
>+      timestamp_end: timestamp
>+    };

Add a "type", and lets drop the "timestamp" prefixes:

    let aEvent = {
      type: "session",
      name: aName,
      start: sessionStart,
      end: timestamp

Overall, I think we could start trying out some test instrumentation with this code. We should start thinking about the types of sessions, events and analysis we want to create using the data we could collect with this service.

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data format

This is for simple events and sessions.

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add some probes

Adding some basic probes - a session probe and a search engine default event.

Comment 15

[Chenxia Liu [:liuche]]

Working on a part 3 to write some files out - I'll have that up soon, and will post some actual JSON data.

Comment 16

[Chenxia Liu [:liuche]]

Sorry for being slow about this - I found a few bugs in the first two patches when actually writing out the data that's collected (surprise!), which I'm in the process of splitting out. Also running into some hangs with OS.File, or file overwrites, so untangling that.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data format v2

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data format v3

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 3: Write some logs to $PROFILE/ui-telemetry.txt

Writing out some sample probes: foregrounding of the main Browser app, and changing the default search engine (currently with no indication of the specific search engine, but we could hash that value and include it as an extra).

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Probe logs

Not super impressive, but here are some of the probes that were collected and written out!

You can pull these logs (off of Android) with the Copy Profile add-on.

Comment 21

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

(In reply to Chenxia Liu [:liuche] from comment #20)
> Created attachment 8335699 [details]
> Probe logs
> 
> Not super impressive, but here are some of the probes that were collected
> and written out!
> 
> You can pull these logs (off of Android) with the Copy Profile add-on.

Cool.

Looking at the data, I'm pretty sure we should be able to convert the JS timestamp to other formats on the server.

Some ideas for other events/sessions:
* Opening URLbar
* Tapping a thumbnail in topsites
* Adding a session that starts in onCreate and ends when we get a Gecko:Ready message
* Adding a session that starts when AboutHome is shown and ends when it hides

I just want to get a feel for what the data would look like for a more full featured set of probes. Then we can take a look and run ideas for simple queries and analyses over the data.

Note for explorers: The data is not true JSON right now, but wrap it in [] and convert "}{" to "},{" and it's a legal JSON array of objects.

Comment 22

[Chenxia Liu [:liuche]]

A quick update: I have a patch for actually sending these new measurements to Telemetry, and also displaying them in a section in about:telemetry. Just wrapping up the formatting, and running into a small problem with iterators/generators.

I'll add the rest of the suggested probes after that, and we can take a look at what they return.

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data format

Comment 24

[Chenxia Liu [:liuche]]

Attachment: Part 2: Support adding probes from Java

This splits out the Telemetry.java parts from the original part 2 patch - those will be moved to bug 942279.

Comment 25

[Chenxia Liu [:liuche]]

Attachment: Part 3: Add probe data to telemetry ping.

Removing the write-to-profile patches, because those were for testing. There is some WIP in bug 942280 for viewing collected data in about:telemetry.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8336998 [details] [diff] [review]
Part 1: Data format

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

The capitalization of "UiTelemetry" makes me twitch every time I see it. I suspect that if it lands as-is, some of the more OCD-inclined members of fx-team will start suffering from seizures. In the interests of our health and well being, could we perhaps rename this to UITelemetry?

Comment 27

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

Comment on attachment 8336998 [details] [diff] [review]
Part 1: Data format

r+, but we can change UiTelemetry to UITelemetry before landing

Comment 28

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

Comment on attachment 8337000 [details] [diff] [review]
Part 2: Support adding probes from Java

I think we need to create specialized GeckoEvents for Session and Event messages, like the one made for Histogram. It was done for performance.

I'd also like to rename some of the APIs here, as well as the strangely named HistogramAdd method:

HistogramAdd -> addHistogram
startUiTelemetrySession -> startSession
stopUiTelemetrySession -> stopSession
sendUiTelemetryEvent -> addEvent

I can take a crack at making these changes and getting a separate review.

Comment 29

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

Comment on attachment 8337002 [details] [diff] [review]
Part 3: Add probe data to telemetry ping.

This is OK by me with a simple name change:
getUiMeasurements -> getTelemetryDetails

I can do this change, but I want Nathan to review this too.

Comment 30

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

Comment on attachment 8337002 [details] [diff] [review]
Part 3: Add probe data to telemetry ping.

Nathan is AFK for a bit, so passing to David

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 1: Data format (r+'d by mfinkle)

We're using UITelemetry instead of UiTelemetry now - everything else is unchanged.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8338006 [details] [diff] [review]
Part 1: Data format (r+'d by mfinkle)

Part 1 (attachment 8338006 [details] [diff] [review]) pushed to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/aea2b472991a

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aea2b472991a

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8337002 [details] [diff] [review]
Part 3: Add probe data to telemetry ping.

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

::: toolkit/components/telemetry/UiTelemetry.jsm
@@ +106,1 @@
>    }

Nit: That capitalization looks weird. "UI" makes more sense than "Ui".

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 3: Add probe data to telemetry ping. (r+'d by Yoric and mfinkle)

Thanks - updated patch to use UI instead of Ui.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

This should stay open until the last piece lands.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Part 3 (attachment 8338541 [details] [diff] [review]) landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceb1b115915

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8338006 [details] [diff] [review]
Part 1: Data format (r+'d by mfinkle)

[Approval Request Comment]

Bug caused by (feature/regressing bug #):

Not fixing a regression, but adding foundation for some telemetry probes. We're hungry for data, and are thinking of trying to get it from our Aurora users sooner rather than later.


User impact if declined:

None, except that we don't get about 2 weeks of telemetry data from them.


Testing completed (on m-c, etc.):

All manual testing, and has had a few hours to bake on mozilla-central.


Risk to taking this patch (and alternatives if risky):

At least for desktop, very low. I assume it's also quite low for Fennec as well, but mfinkle might want to weigh in.


String or IDL/UUID changes made by this patch:

None.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8338541 [details] [diff] [review]
Part 3: Add probe data to telemetry ping. (r+'d by Yoric and mfinkle)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

None - like before, this adds foundations for UI telemetry probes.


User impact if declined: 

None - except we don't get UI telemetry data from them.


Testing completed (on m-c, etc.): 

All manual.


Risk to taking this patch (and alternatives if risky): 

Very low risk. This just gets TelemetryPing to collect the data from UITelemetry.jsm.


String or IDL/UUID changes made by this patch:

None.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ceb1b115915

Comment 41

[bhavana bajaj [:bajaj]]

Comment on attachment 8338006 [details] [diff] [review]
Part 1: Data format (r+'d by mfinkle)

Request to make sure we have the new telemetry data as needed and ensure no surprises on any disturbance on existing telemetry ! thanks.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Part 1 (attachment 8338006 [details] [diff] [review]) landed on mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/af2d122f17ca

Part 3 (attachment 8338541 [details] [diff] [review]) landed on mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/77741eb02905

Pretty sure I've set the right flags here - hopefully someone will correct me if I'm wrong on that.

Comment 43

[Richard Newman [:rnewman]]

I have a few spare cycles, so I'll take care of tweaking Part 2 to use events.

Comment 44

[Richard Newman [:rnewman]]

Attachment: Part 2: support natively sending UI telemetry events from Java. v1

Untested, but it builds. This entirely obsoletes the previous JSON- and observer-based Part 2.

Is this what you had in mind, mfinkle?

Comment 45

[Richard Newman [:rnewman]]

Comment on attachment 8339602 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v1

I have tested this with Chenxia's patch from Bug 942279, and it seems to work.

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8339602 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v1

Yep, looks fine to me. Desktop does not user the observer notifications. Bonus points when the fallback absolute timestamp gets torn out. :)

Comment 47

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

Comment on attachment 8339602 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v1

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

>         TELEMETRY_HISTOGRAM_ADD(37),
>         PREFERENCES_OBSERVE(39),
>         PREFERENCES_GET(40),
>-        PREFERENCES_REMOVE_OBSERVERS(41);
>+        PREFERENCES_REMOVE_OBSERVERS(41),
>+        UI_TELEMETRY_SESSION_START(42),
>+        UI_TELEMETRY_SESSION_STOP(43),
>+        UI_TELEMETRY_EVENT(44);

What about creating some consistency:

UI_TELEMETRY_SESSION_START -> TELEMETRY_UI_SESSION_START
UI_TELEMETRY_SESSION_STOP -> TELEMETRY_UI_SESSION_STOP
UI_TELEMETRY_EVENT -> TELEMETRY_UI_EVENT

Maybe even move them up and under TELEMETRY_HISTOGRAM_ADD

>+    public static GeckoEvent createUITelemetrySessionStartEvent(String session, long timestamp) {
>+    public static GeckoEvent createUITelemetrySessionStopEvent(String session, long timestamp) {
>+    public static GeckoEvent createUITelemetryEvent(String action, String method, String extras, long timestamp) {

Same renaming pattern

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

>+    public static void startUiTelemetrySession(String sessionName) {
>+    public static void stopUiTelemetrySession(String sessionName) {
>+    public static void sendUiTelemetryEvent(String action, String method, String extras, long timestamp) {

At least this: Ui -> UI

I'm not sure why the original author did it this way, but histograms use Telemetry.Histogram.add(...) so we could use:
Telemetry.UI.startSession(...)
Telemetry.UI.stopSession(...)
Telemetry.UI.event(...)

Thoughts?

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-  getBrowserTab: function(tabId) {
>+  getBrowserTab: function (tabId) {

We use function(

>diff --git a/toolkit/components/telemetry/UITelemetry.jsm b/toolkit/components/telemetry/UITelemetry.jsm

> this.UITelemetry =  {

>   init: function init() {

If we no longer need init() you can remove it and we don't need to call it (or uninit()) in browser.js anymore

>diff --git a/widget/android/AndroidJavaWrappers.h b/widget/android/AndroidJavaWrappers.h

>         TELEMETRY_HISTOGRAM_ADD = 37,
>         ADD_OBSERVER = 38,
>         PREFERENCES_OBSERVE = 39,
>         PREFERENCES_GET = 40,
>         PREFERENCES_REMOVE_OBSERVERS = 41,
>+        UI_TELEMETRY_SESSION_START = 42,
>+        UI_TELEMETRY_SESSION_STOP = 43,
>+        UI_TELEMETRY_EVENT = 44,

Same renaming

>diff --git a/widget/android/nsAppShell.cpp b/widget/android/nsAppShell.cpp

>+    case AndroidGeckoEvent::UI_TELEMETRY_SESSION_STOP: {
>+

nit: prefer to remove the blank line

>+        nsCOMPtr<nsIUITelemetryObserver> obs;
>+        mBrowserApp->GetUITelemetryObserver(getter_AddRefs(obs));
>+        obs->StopSession(

null check obs?

>diff --git a/widget/android/nsIAndroidBridge.idl b/widget/android/nsIAndroidBridge.idl

> [scriptable, uuid(7508b826-4129-40a0-91da-2a6bba33681f)]
> interface nsIAndroidBrowserApp : nsISupports {
>   nsIBrowserTab getBrowserTab(in int32_t tabId);
>   void getPreferences(in int32_t requestId,
>                       [array, size_is(count)] in wstring prefNames,
>                       in unsigned long count);
>   void observePreferences(in int32_t requestId,
>                           [array, size_is(count)] in wstring prefNames,
>                           in unsigned long count);
>   void removePreferenceObservers(in int32_t requestId);
>+  nsIUITelemetryObserver getUITelemetryObserver();

Make a new uuid for this interface. It's our way of "bumping" it when we change the interface

Comment 48

[Richard Newman [:rnewman]]

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

> What about creating some consistency:
> 
> UI_TELEMETRY_SESSION_START -> TELEMETRY_UI_SESSION_START

I was considering "UITelemetry" to be the noun, just like "Preferences" or "Telemetry". We should either use UI_TELEMETRY here, or TelemetryUI elsewhere. See below...


> >+    public static void startUiTelemetrySession(String sessionName) {
> >+    public static void stopUiTelemetrySession(String sessionName) {
> >+    public static void sendUiTelemetryEvent(String action, String method, String extras, long timestamp) {
> 
> At least this: Ui -> UI

Urgh, how did that get into this patch? Chenxiaaaaaaaaa!


> Telemetry.UI.event(...)
> 
> Thoughts?

I'm happy to change the term hierarchy throughout, though I probably wouldn't introduce an inner class in Java just to get the '.'. (Definitely an option in JS, though.)


> We use function(

I was curious, so I did some grepping. We're running about 50-50 in the codebase for the two styles when used in a method context, and about 2:1 if you include anonymous functions. Our coding style doc was so out of date that it was still recommending naming the method func. Tragic.


> Make a new uuid for this interface. It's our way of "bumping" it when we
> change the interface

Good catch. I always forget to do that.

Comment 49

[Richard Newman [:rnewman]]

Attachment: Part 2: support natively sending UI telemetry events from Java.

Comment 50

[Richard Newman [:rnewman]]

Comment on attachment 8342071 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java.

Changes:

* Addressed review comments. I didn't want to rename UITelemetry itself, so I tried to draw the line in the right place between that and TelemetryUI.

* Clarified what Telemetry.Timer actually was, and opened the door to using a different clock (which we will do when we measure Real Things).

* Miscellaneous corrections, reformattings, and typo fixes. (Trailing commas continues to be good style, before you ask!)

* More error checking for durations.

* Made timestamps mandatory on events, fixing Bug 944116.

Comment 51

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

Comment on attachment 8342071 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java.

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

>-        mAboutHomeStartupTimer = new Telemetry.Timer("FENNEC_STARTUP_TIME_ABOUTHOME");
>+        mAboutHomeStartupTimer = new Telemetry.UptimeTimer("FENNEC_STARTUP_TIME_ABOUTHOME");

Question: Do we want to offer this choice of timers are is that just too many choices? I don't mind the explicit nature of the rename though.

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

> public class Telemetry {

>+    public static void startTelemetryUISession(String sessionName) {
>+    public static void stopTelemetryUISession(String sessionName) {
>+    public static void sendTelemetryUIEvent(String action, String method, long timestamp, String extras) {
>+    public static void sendTelemetryUIEvent(String action, String method, long timestamp) {
>+    public static void sendTelemetryUIEvent(String action, String method, String extras) {
>+    public static void sendTelemetryUIEvent(String action, String method) {
>+    public static void sendTelemetryUIEvent(String action) {

My redundancy OCD is forcing me to ask for a rename: Remove "Telemetry" from these names.

Telemetry.startTelemetryUISession -> Telemetry.startUISession

>diff --git a/toolkit/components/telemetry/UITelemetry.jsm b/toolkit/components/telemetry/UITelemetry.jsm
>+  addEvent: function(aAction, aMethod, aTimestamp, aExtras) {

I wouldn't mind looking for a way to make the timestamp a bit easier to supply, but we can do that later.

Comment 52

[Richard Newman [:rnewman]]

We discussed a couple of extensions to this:

* Events should be tagged with the current active sessions.
* Sessions should have a 'reason' flag to describe why they ended -- e.g., "closed menu without clicking anything", "tapped an item".

I'll add those then get this ready to land.

Comment 53

[Richard Newman [:rnewman]]

Attachment: Part 2: support natively sending UI telemetry events from Java. v2

This patch:

* Fixes a couple of bugs in the last one.
* Adds extant sessions and a reason code to events.
* Extends the test suite (and implementation of UITelemetry) to allow for testing.
* Adds tests that now pass.

Comment 54

[Richard Newman [:rnewman]]

Comment on attachment 8345061 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v2

Review for Mark.

Feedback on testing framework selection from Nick.

Comment 55

[Richard Newman [:rnewman]]

Attachment: Part 2: support natively sending UI telemetry events from Java. v3

Hooray tests! The "session end" call didn't delete the session record.

Comment 56

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

Comment on attachment 8345101 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v3


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

> public class Telemetry {

>+    public static void startTelemetryUISession(String sessionName) {
>+    public static void stopTelemetryUISession(String sessionName, String reason) {
>+    public static void sendTelemetryUIEvent(String action, String method, long timestamp, String extras) {
>+    public static void sendTelemetryUIEvent(String action, String method, long timestamp) {
>+    public static void sendTelemetryUIEvent(String action, String method, String extras) {
>+    public static void sendTelemetryUIEvent(String action, String method) {
>+    public static void sendTelemetryUIEvent(String action) {

I'd still like to remove the "Telemetry" from these names.

Telemetry.startTelemetryUISession -> Telemetry.startUISession

>diff --git a/mobile/android/base/tests/robocop_testharness.js b/mobile/android/base/tests/robocop_testharness.js

>+  testScope.BrowserApp = bridge.browserApp;

I thought you wanted the other BrowserApp. Now that I see what you want, I think we should not inject the XPCOM interface into the testScope since it is very ambiguous with the JS BrowserApp in browser.js

Instead, you can get a reference to this interface directly from your JS test code, example below

>+            Telemetry.sendTelemetryUIEvent("enone", "method0");
>+            Telemetry.startTelemetryUISession("foo");
>+            Telemetry.sendTelemetryUIEvent("efoo", "method1");
>+            Telemetry.startTelemetryUISession("foo");
>+            Telemetry.sendTelemetryUIEvent("efoo", "method2");
>+            Telemetry.startTelemetryUISession("bar");
>+            Telemetry.sendTelemetryUIEvent("efoobar", "method3", "foobarextras");
>+            Telemetry.stopTelemetryUISession("foo", "reasonfoo");
>+            Telemetry.sendTelemetryUIEvent("ebar", "method4", "barextras");
>+            Telemetry.stopTelemetryUISession("bar", "reasonbar");
>+            Telemetry.stopTelemetryUISession("bar", "reasonbar2");
>+            Telemetry.sendTelemetryUIEvent("enone", "method5");

So such "Telemetry", but I digress

>diff --git a/mobile/android/base/tests/testUITelemetry.js b/mobile/android/base/tests/testUITelemetry.js

>+add_test(function test_telemetry_events() {
>+  let obsXPCOM = BrowserApp.getUITelemetryObserver();

Try using this:

    let bridge = Components.classes["@mozilla.org/android/bridge;1"]
                           .getService(Components.interfaces.nsIAndroidBridge);
    let obsXPCOM = bridge.BrowserApp.getUITelemetryObserver();

Comment 57

[Richard Newman [:rnewman]]

Try build (now with less "Telemetry"!)

https://tbpl.mozilla.org/?tree=Try&rev=ac853b56ce56

Comment 58

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/00dd7ea56952

Comment 59

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8345101 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v3

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

::: mobile/android/base/Telemetry.java
@@ +11,5 @@
>  import android.util.Log;
>  
> +/**
> + * All telemetry times are relative to one of two clocks:
> + * 

nit: ws.

::: mobile/android/base/tests/robocop_testharness.js
@@ +65,5 @@
>                    };
>      sendMessageToJava(message);
>    };
>  
> +  testScope.BrowserApp = bridge.browserApp;

I'm fine with this, although since you're already changing the name and mfinkle said there is a clash, perhaps we can add both and make it clear which is which?

::: widget/android/nsIAndroidBridge.idl
@@ +38,2 @@
>  };
> +

nit: ws?

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00dd7ea56952

Comment 61

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander PTO until Dec 7th from comment #59)

> > +  testScope.BrowserApp = bridge.browserApp;
> 
> I'm fine with this, although since you're already changing the name and
> mfinkle said there is a clash, perhaps we can add both and make it clear
> which is which?

I landed without this part; I fetch it directly from the bridge within the test.

Comment 62

[Richard Newman [:rnewman]]

Comment on attachment 8345101 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Part 2 of a bug that mostly made it into 27. We'd like to start collecting UI telemetry in 28, so Part 2 needs to catch up.

User impact if declined: 
  No UI telemetry until Fx29.

Testing completed (on m-c, etc.): 
  Hand-tested, Robocop tests, currently in Nightly.

Risk to taking this patch (and alternatives if risky): 
  New code, so should be very slim.

String or IDL/UUID changes made by this patch:
  Modifies an Android-specific IDL to alter the signature of BrowserApp (a JS component) and add the nsIUITelemetry definition itself.

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.

Comment 64

[Benjamin Kerensa [:bkerensa]]

Comment on attachment 8345101 [details] [diff] [review]
Part 2: support natively sending UI telemetry events from Java. v3

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

Thanks for this we will go ahead approve this for uplift!

Comment 65

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd6c34135c45

Comment 66

[Richard Newman [:rnewman]]

Rudimentary docs:

https://wiki.mozilla.org/Mobile/Fennec/Android/UITelemetry