1178512 - [Metrics] Make the developer HUD send Advanced Telemetry events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Russ Nicoletti [:russn]]

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.

Comment 1

[Russ Nicoletti [:russn]]

Attachment: hud.js.patch

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...

Comment 2

[Russ Nicoletti [:russn]]

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

Comment 3

[Russ Nicoletti [:russn]]

Attachment: hud.js.patch

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

Comment 4

[Jan Keromnes [:janx]]

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 5

[Jan Keromnes [:janx]]

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).

Comment 6

[Jan Keromnes [:janx]]

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.

Comment 7

[Russ Nicoletti [:russn]]

Attachment: hud.js.patch

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.

Comment 8

[Russ Nicoletti [:russn]]

Attachment: hud.js.patch

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

Comment 9

[Jan Keromnes [:janx]]

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).

Comment 10

[Jan Keromnes [:janx]]

Attachment: hud.js.patch

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

Comment 11

[Jan Keromnes [:janx]]

Previous try is fully green and interdiff is small.

Comment 12

[Jan Keromnes [:janx]]

Attachment: hud.js.patch

Fixed typo.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e2a194964fa2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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