[Metrics] Make the developer HUD send Advanced Telemetry events

RESOLVED FIXED in Firefox 42

Status

Firefox OS
Developer Tools
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: russn, Assigned: russn)

Tracking

(Blocks: 1 bug)

unspecified
FxOS-S2 (10Jul)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Summary: Make the developer HUD send Advanced Telemetry events → [Metrics] Make the developer HUD send Advanced Telemetry events
(Assignee)

Comment 1

2 years ago
Created attachment 8627815 [details] [diff] [review]
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...
Attachment #8627815 - Flags: review?(janx)
(Assignee)

Comment 2

2 years ago
Are there automated tests for hud.js that I should update for the telemetry functionality?
Flags: needinfo?(janx)
(Assignee)

Comment 3

2 years ago
Created attachment 8627941 [details] [diff] [review]
hud.js.patch

I've updated the patch with a minor bugfix regarding when "context" is supplied to a custom metric (via console API)
Attachment #8627941 - Flags: review?(janx)
(Assignee)

Updated

2 years ago
Attachment #8627815 - Flags: review?(janx)

Comment 4

2 years ago
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!
Flags: needinfo?(janx)

Comment 5

2 years ago
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).
Flags: needinfo?(rnicoletti)
Attachment #8627941 - Flags: review?(janx)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8628319 [details] [diff] [review]
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.
Attachment #8627815 - Attachment is obsolete: true
Attachment #8627941 - Attachment is obsolete: true
Flags: needinfo?(rnicoletti)
Attachment #8628319 - Flags: review?(janx)
(Assignee)

Comment 8

2 years ago
Created attachment 8628322 [details] [diff] [review]
hud.js.patch

I think I found the problem. Apologies for the unnecessary attachments.
Attachment #8628319 - Attachment is obsolete: true
Attachment #8628319 - Flags: review?(janx)
Attachment #8628322 - Flags: review?(janx)

Comment 9

2 years ago
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).
Attachment #8628322 - Flags: review?(janx) → review+
Created attachment 8628734 [details] [diff] [review]
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
Attachment #8628322 - Attachment is obsolete: true
Attachment #8628734 - Flags: review+
Previous try is fully green and interdiff is small.
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed
Created attachment 8628767 [details] [diff] [review]
hud.js.patch

Fixed typo.
Attachment #8628734 - Attachment is obsolete: true
Attachment #8628767 - Flags: review+

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/e2a194964fa2
Keywords: checkin-needed

Updated

2 years ago
Blocks: 1177143
https://hg.mozilla.org/mozilla-central/rev/e2a194964fa2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S2 (10Jul)
(Assignee)

Updated

2 years ago
Blocks: 1166295
(Assignee)

Updated

2 years ago
Blocks: 1166286
(Assignee)

Updated

2 years ago
Blocks: 1166294
(Assignee)

Updated

2 years ago
Blocks: 1166298
(Assignee)

Updated

2 years ago
Blocks: 1166299
(Assignee)

Updated

2 years ago
Blocks: 1166300
You need to log in before you can comment on or make changes to this bug.