Closed Bug 940807 Opened 6 years ago Closed 6 years ago

Modify UITelemetry.jsm to allow registration of simple measures functions

Categories

(Toolkit :: Telemetry, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

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.
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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?
Attachment #8335051 - Flags: feedback?(liuche)
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.
No longer blocks: 932092
Depends on: 932092
(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?
(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?
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
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
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.
Unrelated, but how do I pick a product from the telemtry.mozilla.org dashboard?
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.
(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 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.
Attachment #8335051 - Flags: feedback?(liuche) → feedback+
Whiteboard: [Australis:P1]
Attached patch Patch v1 (obsolete) — Splinter Review
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. :)
Attachment #8335051 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Whoops, forgot about the trailing whitespace...
Attachment #8336911 - Attachment is obsolete: true
Comment on attachment 8336917 [details] [diff] [review]
Patch v1.1

As an addition to liuche's work.
Attachment #8336917 - Flags: review?(mark.finkle)
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.
Attachment #8336917 - Flags: review?(mark.finkle) → review+
Comment on attachment 8336917 [details] [diff] [review]
Patch v1.1

Thanks Mark!
Attachment #8336917 - Flags: review?(dteller)
Attached patch Patch v1.2 (r+'d by mfinkle) (obsolete) — Splinter Review
Whoops - forgot to update to latest (takes into account last-minute naming changes to bug 932092).
Attachment #8336917 - Attachment is obsolete: true
Attachment #8336917 - Flags: review?(dteller)
Attachment #8338046 - Flags: review?(dteller)
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?
Attachment #8338046 - Flags: review?(dteller) → review+
(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!
Attachment #8338046 - Attachment is obsolete: true
Attachment #8338568 - Flags: review+
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.
Attachment #8338568 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c46b1a166199
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Australis:P1]
Garr, this is the wrong bug. Sorry for the spam.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1]
Attachment #8338568 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
UITelemetry.jsm doesn't exist on Aurora. This will need a branch-specific patch for uplift or a dependent bug approved as well.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.
Whiteboard: [Australis:P1]
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.
Attachment #8338568 - Flags: approval-mozilla-beta?
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.
Attachment #8338568 - Flags: approval-mozilla-beta?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.