Closed Bug 1166377 Opened 5 years ago Closed 2 years ago

[Metrics] Make hud.js metrics context specific vs. app specific

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: thills, Assigned: rnicoletti)

References

Details

Attachments

(2 obsolete files)

We need to make the hud.js metrics context specific in addition to app specific.

Essentially, we want to know not only the app that the HUD metric is associated with, but also the context (e.g. content page, service worker, shared worker, etc.).
Assignee: nobody → rnicoletti
Status: NEW → ASSIGNED
Attached patch hud.js.patch (obsolete) — Splinter Review
Hi Jan, the attached patch is the culmination of the changes I've made to hud.js based on your feedback from bug 1166295. This bug seems more appropriate for this patch, which is why I'm requesting a review here.
Attachment #8626401 - Flags: review?(janx)
Attached patch hud-advanced-telemetry.patch (obsolete) — Splinter Review
I'm obsoleting the previous patch because it was not the correct format.
Attachment #8626401 - Attachment is obsolete: true
Attachment #8626401 - Flags: review?(janx)
Attachment #8626934 - Flags: review?(janx)
Comment on attachment 8626934 [details] [diff] [review]
hud-advanced-telemetry.patch

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

Looking good! I'm running out of details to complain about, so this is probably the last round.

I'm not sure this bug is more appropriate, but I don't mind. However, your patch description says "imported patch hud-advanced-telemetry" where it should say something like "Bug 1166377 - Make the developer HUD send Advanced Telemetry events. r=janx" instead.

Also, I hope I found the problem you hinted at during our chat last week!

::: b2g/chrome/content/devtools/hud.js
@@ +34,5 @@
> +let _telemetryDebug = true;
> +
> +function telemetryDebug(...args) {
> +  if (_telemetryDebug) {
> +    args.unshift('[AdvancedTelemetry-RN]');

Just curious, what's the meaning of "RN"?

@@ +284,5 @@
> +
> +    let systemapp = document.querySelector('#systemapp');
> +    if (this.frame === systemapp) {
> +      frame = getContentWindow();
> +    }

Nit: I'd prefer avoiding duplication of this code.

Please add a `get frame() { ... }` getter to the Target prototype, and rename the attribute `this.frame` to `this._frame` in the Target constructor. Then in the getter you will be able to compare `this._frame` to `systemapp` and return the correct frame. That way, everywhere you access `this.frame`, it will already be fixed.

@@ +415,5 @@
>  
>          if (this._security.indexOf(pageError.category) > -1) {
>            metric.name = 'security';
> +
> +          // Telemetry sends the security error category, not simply

Super-nit: The word "simply" makes it sound like telemetry sends both category and count. Please rephrase to "not the count".

@@ +507,5 @@
> +      return;
> +    }
> +
> +    // If this is a 'telemetry' log entry, create a telemetry metric from
> +    // the log content

Super-nit: Please end comments with a full-stop.

@@ +517,5 @@
> +    }
> +
> +    let telemetryData = logContent.split(separator);
> +
> +    // Positions of the components of a telemetry log entry

Super-nit: Please end comments with a full-stop.

@@ +524,5 @@
> +    let VALUE_IDX = 2;
> +    let CONTEXT_IDX = 3;
> +
> +    if ( telemetryData[TELEMETRY_IDENTIFIER_IDX] != 'telemetry' ||
> +        !(telemetryData.length === 3 || telemetryData.length === 4) ) {

Nit: I think the form "t[IDX] != 'telemetry' || t.length < 3 || t.length > 4" would be more readable than "t[IDX] != 'telemetry' || !(t.length === 3 || t.length === 4)".
Attachment #8626934 - Flags: review?(janx) → feedback+
These hud.js changes are now being tracked by bug 1178512.
Comment on attachment 8626934 [details] [diff] [review]
hud-advanced-telemetry.patch

(marking patch as obsolete)
Attachment #8626934 - Attachment is obsolete: true
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.