The developer HUD is an excellent foundation for sending Advanced Telemetry data to gaia given it captures a lot of the metrics we'd like to record as telemetry data and it already sends events to the system app. Therefore, this bug is for making the developer HUD send Advanced Telemetry data to gaia.

Hi Jan, I'm now using this bug for the hud.js changes I'm making. I have addressed your feedback from https://bugzilla.mozilla.org/show_bug.cgi?id=1166377#c3. Btw, the meaning of "RN" is my initials; I had meant to remove that...

Are there automated tests for hud.js that I should update for the telemetry functionality?

I've updated the patch with a minor bugfix regarding when "context" is supplied to a custom metric (via console API)

The Developer HUD tests live in Gaia: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/developer_hud_test.js It mostly covers the front-end widget system, so I'm not sure you'll be able to exercise the Telemetry code with it. I don't think we have a good way to directly test the B2G back-end code yet, but if you're able to help with that please do!

Comment on attachment 8627941 [details] [diff] [review] hud.js.patch This patch format is too hard to review, sorry. Please use something like `diff -u8`, `git show -U8`, or better use the usual Mozilla tools to upload your patch (e.g. `git bz attach` from the moz-git-tools if you're using Git, or the Mercurial equivalent).

A good sanity check when asking for a review is to click on "Review" on the attachment, and then select "all files". This may help detect simple problems, like a bad patch format.

Trying again. My .hgrc has the following entries with respect to diff format: [diff] git = 1 unified = 8 showfunc = 1 I will review the patch after I submit it.

I think I found the problem. Apologies for the unnecessary attachments.

Comment on attachment 8628322 [details] [diff] [review] hud.js.patch Review of attachment 8628322 [details] [diff] [review]: ----------------------------------------------------------------- All right, r+ with nits! Please: 1) Address all the nits (and only the nits), 2) Re-upload your patch, 3) Carry over my r+ (by setting it yourself on the new patch), 4) Set the "checkin-needed" keyword on this bug to move forward with this. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76c43dc377c1 --- Nit: Thanks for creating the `this.frame` getter! However, I believe that for the System App, the appManifestURL lives in `this._frame` instead (even though `this.frame` is the correct place to send the event to). Please also create a `this.manifest` getter like so: > @@ -190,6 +190,10 @@ function Target(frame, actor) { > > Target.prototype = { > > + get manifest() { > + return this._frame.appManifestURL; > + }, > + And fix the creation of `data` in `Target.update()` like so: > @@ -217,7 +218,7 @@ Target.prototype = { > > let data = { > metrics: [], // FIXME(Bug 982066) Remove this field. > - manifest: this.frame.appManifestURL, > + manifest: this.manifest, > metric: metric, > message: message > }; ::: b2g/chrome/content/devtools/hud.js @@ +51,5 @@ > _client: null, > _conn: null, > _watchers: [], > _logging: true, > + _telemetry: null, Nit: Since this is a boolean, please initialize it to `false` instead of `null`. @@ +100,5 @@ > SettingsListener.observe('hud.logging', this._logging, enabled => { > this._logging = enabled; > }); > + > + SettingsListener.observe('debug.performance_data.advanced_telemetry', false, enabled => { Nit: Please use `this._telemetry` as default value instead of `false`. @@ +270,5 @@ > + this._sendTelemetryEvent(data.metric); > + }, > + > + _sendTelemetryEvent: function target_sendTelemetryEvent(metric) { > + if (!developerHUD._telemetry || metric.skipTelemetry) { When a target is destroyed, line 263 calls `_send({})`, causing line 270 to call `_sendTelemetryEvent(undefined)`, causing this line to fail with "TypeError: metric is undefined". Please verify that `metric` is defined, i.e. by replacing the bailout condition with `!_telemetry || !metric || metric.skipTelemetry`. @@ +277,5 @@ > + > + let frame = this.frame; > + > + if (!this.appName) { > + if (!frame.appManifestURL) { For the System App, I believe `this.frame` is the correct frame to send an event to, however it doesn't seem to have `appManifestURL`. Please leave the rest of the code as is, but in this if, use: > let manifest = this.manifest; > if (!manifest) return; > let start = manifest.indexOf('/') + 2; > ... @@ +295,5 @@ > + telemetryDebug('sending advanced-telemetry-update with this data: ' + JSON.stringify(data)); > + shell.sendEvent(frame, 'advanced-telemetry-update', Cu.cloneInto(data, frame)); > + }, > + > + get frame() { Nit: Please move this getter to the top of the prototype (that's were getters and setters usually are).

Actually, sorry to do this, but in order to expedite some HUD changes before I leave tomorrow, I took the liberty of folding my nits into your patch and re-upload it. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab1327c7ad7 Interdiff (my nits): https://hg.mozilla.org/try/rev/d6076cd2798f

Previous try is fully green and interdiff is small.

Fixed typo.