940807 - Modify UITelemetry.jsm to allow registration of simple measures functions

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

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

Bug 932092 adds a new UITelemetry JSM which provides a centralized place for UI devs to send UI-related Telemetry data to.

In its current form, the JSM listens for nsIObserver notifications for populating the Telemetry data. It doesn't, however, allow us to populate Telemetry's "simple measures" blob that goes out with the ping.

Examples of "simple measures" are things like how many toolbars exist in the UI, or how many non-default toolbar items exist in the UI. These types of measures aren't really suited to being passed through nsIObserver notifications.

Instead, we can get TelemetryPing.jsm to request simple measures data from UITelemetry.jsm, which can in turn poll a set of registered functions to populate the simple measure data. While my interests are strictly Desktop related, I can see such a capability being useful for the Fennec team as well.

Comment 1

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

Attachment: WIP Patch 1

Hey liuche - this is what I'm thinking for some of the measures we want to take for Australis. What do you think of this?

Comment 2

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

I should be more precise on the relationship with bug 932092 - the patch in here very much depends on it, and not the other way around.

Comment 3

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

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

> In its current form, the JSM listens for nsIObserver notifications for
> populating the Telemetry data. It doesn't, however, allow us to populate
> Telemetry's "simple measures" blob that goes out with the ping.

The JSM only listens for observer notification to support UI Telemetry coming from Java. JavaScript probes would be handled using UiTelemetry.addEvent, UiTelemetry.startSession and UiTelemetry.stopSession

> Examples of "simple measures" are things like how many toolbars exist in the
> UI, or how many non-default toolbar items exist in the UI. These types of
> measures aren't really suited to being passed through nsIObserver
> notifications.

I agree that "counts" will be ill suited for the event and session APIs. But are simple counts better suited for the traditional Telemetry bin system?

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #3)
> I agree that "counts" will be ill suited for the event and session APIs. But
> are simple counts better suited for the traditional Telemetry bin system?

You'll have to forgive me - I'm quite new to Telemetry, and I'm using bug 810146 (which used the getSimpleMeasures approach) for most of my guidance. What is the Telemetry bin system?

Comment 5

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

The histogram file has lots of values with "*_COUNT" names:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json

Those values are turned into histograms (not a surpirse) and that's how the dashboard displays the data too.

I don't know if the simple measures are also turned into histograms at some point.

Note: I dislike the current histogram approach because I dislike needing to register a a counter in the Histogram.json file.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Simple Measures are best for values that are recorded once per browser session; they have much less overhead in the data transmission and storage at the Telemetry server side. The telemetry dashboard makes histograms out of simple measures when it analyzes the data; they show up in the dashboard as SIMPLE_MEASURE_this_that_whatever; see for example http://telemetry.mozilla.org/#path=nightly/28/SIMPLE_MEASURES_ADDONMANAGER_XPIDB_SAVES_TOTAL

The process for adding simple measurements isn't as straightforward or well documented, which is why a lot of one-per-session measurements are currently implemented as histograms; see bug 852974.

The dashboard extracted and aggregated that value from a nested JSON structure; the addon manager registered a simple measure containing a JS object. You can see these nested structures by looking at the Simple Measurements section in about:telemetry

Comment 7

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

Irving - Great explanation. I'm sold on using Simple Measures. You mention "browsing session" and I wonder how we would determine that for multi-day sessions, or on Android, where getting killed on the background and restarting is not indicative of a new browsing session, but could be the OS trying to free memory.

That's one reason I want to capture the concept of "session" in the other UI telemetry API.

Comment 8

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

Unrelated, but how do I pick a product from the telemtry.mozilla.org dashboard?

Comment 9

[:Irving Reid (No longer working on Firefox)]

If UI telemetry breaks up a single run of the Firefox executable (which defines a Telemetry "session") into multiple user activity sessions, you'll probably want to switch back to Histograms, so that you can record separate values for each user activity session.

That doesn't let you correlate data points across user activity sessions - for example you'd get a histogram of session durations, and a histogram of click counts, but not know which click count went with which session duration. This might not matter if you're aggregating and averaging everything, but it's worth keeping in mind.

Comment 10

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

(In reply to :Irving Reid from comment #9)
> If UI telemetry breaks up a single run of the Firefox executable (which
> defines a Telemetry "session") into multiple user activity sessions, you'll
> probably want to switch back to Histograms, so that you can record separate
> values for each user activity session.
> 
> That doesn't let you correlate data points across user activity sessions -
> for example you'd get a histogram of session durations, and a histogram of
> click counts, but not know which click count went with which session
> duration. This might not matter if you're aggregating and averaging
> everything, but it's worth keeping in mind.

I would like to be able to see a click count histogram for the "app startup" session, where "app startup" session is the short period of time right after firefox opens.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8335051 [details] [diff] [review]
WIP Patch 1

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

Looks good to me! (I bitrotted this a tiny bit, sorry.)

::: toolkit/components/telemetry/UiTelemetry.jsm
@@ +104,5 @@
>  
>      this.tmpTelemetryLog.push(aEvent);
> +  },
> +
> +  _simpleMeasureFuncs: new Map(),

What's the incentive to use Map instead of {}? Unless the keys are not all going to be strings, my impression was that using an {} object was preferred.

@@ +135,5 @@
> +      throw new Error("A simple measurement function is already registered for "
> +                      + aName);
> +    }
> +
> +    this._simpleMeasureFuncs.set(aName, aFunction); 

Trailing space.

Comment 12

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

Attachment: Patch v1

Ok, I've swapped out the Map for a simple object. I guess I'm just used to working with Maps from the CustomizableUI stuff. :)

Comment 13

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

Attachment: Patch v1.1

Whoops, forgot about the trailing whitespace...

Comment 14

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

Comment on attachment 8336917 [details] [diff] [review]
Patch v1.1

As an addition to liuche's work.

Comment 15

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

Comment on attachment 8336917 [details] [diff] [review]
Patch v1.1

You might want to get :Yoric to review the TelemetryPing.js part, just for completeness.

Comment 16

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

Comment on attachment 8336917 [details] [diff] [review]
Patch v1.1

Thanks Mark!

Comment 17

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

Attachment: Patch v1.2 (r+'d by mfinkle)

Whoops - forgot to update to latest (takes into account last-minute naming changes to bug 932092).

Comment 18

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

Comment on attachment 8338046 [details] [diff] [review]
Patch v1.2 (r+'d by mfinkle)

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

::: toolkit/components/telemetry/UITelemetry.jsm
@@ +100,5 @@
>    _logEvent: function sendEvent(aEvent) {
>      this.measurements.push(aEvent);
> +  },
> +
> +  _simpleMeasureFuncs: {},

Please document this object.

@@ +129,5 @@
> +  addSimpleMeasureFunction: function(aName, aFunction) {
> +    if (aName in this._simpleMeasureFuncs) {
> +      throw new Error("A simple measurement function is already registered for "
> +                      + aName);
> +    }

If, as I understand, aFunction needs to be a function, could you check its typeof, to provide early error reporting?

Comment 19

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

Attachment: Patch v1.3 (r+'d by mfinkle, Yoric)

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #18)
> Comment on attachment 8338046 [details] [diff] [review]
> Patch v1.2 (r+'d by mfinkle)
> 
> Review of attachment 8338046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/UITelemetry.jsm
> @@ +100,5 @@
> >    _logEvent: function sendEvent(aEvent) {
> >      this.measurements.push(aEvent);
> > +  },
> > +
> > +  _simpleMeasureFuncs: {},
> 
> Please document this object.

Done!

> 
> @@ +129,5 @@
> > +  addSimpleMeasureFunction: function(aName, aFunction) {
> > +    if (aName in this._simpleMeasureFuncs) {
> > +      throw new Error("A simple measurement function is already registered for "
> > +                      + aName);
> > +    }
> 
> If, as I understand, aFunction needs to be a function, could you check its
> typeof, to provide early error reporting?

Good idea. Done. Thanks Yoric!

Comment 20

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

Landed on inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c46b1a166199

Comment 21

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

Comment on attachment 8338568 [details] [diff] [review]
Patch v1.3 (r+'d by mfinkle, Yoric)

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

Not a bug. This allows UITelemetry to collect simple measurements.


User impact if declined: 

None. Not a user facing feature.


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

All manual.


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

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 22

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

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

Comment 23

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

I think liuche, mfinkle, or someone else from the Fennec team will probably want to land that last bit before resolving this bug, so re-opening.

The parts that affect Australis have landed and merged, so removing Australis:P1 tag.

Comment 24

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

Garr, this is the wrong bug. Sorry for the spam.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

UITelemetry.jsm doesn't exist on Aurora. This will need a branch-specific patch for uplift or a dependent bug approved as well.

Comment 26

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #25)
> UITelemetry.jsm doesn't exist on Aurora. This will need a branch-specific
> patch for uplift or a dependent bug approved as well.

Yep - this patch relies on the patches in bug 932092, which also received Aurora approval.

Comment 27

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

Landed on mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/1fcd91ab99b5

Comment 28

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

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

Comment 29

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

Comment on attachment 8338568 [details] [diff] [review]
Patch v1.3 (r+'d by mfinkle, Yoric)

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

None. The UITelemetry bugs that I requested (and received! Thanks!) approval for all depend on this one, and I accidentally forgot to request approval on it. :/

This is just the foundational work that allows BrowserUITelemetry to actually populate the TelemetryPing with its data via the UITelemetry module.


User impact if declined: 

None.


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

All manual for quite some time now.


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

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 30

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

Comment on attachment 8338568 [details] [diff] [review]
Patch v1.3 (r+'d by mfinkle, Yoric)

And now it occurs to me that this is because this patch landed on Aurora and made the uplift, so does not require beta uplift.