1180793 - [New Gaia Architecture][Metrics] Hud should report specific app for those Apps housed under "Communications"

Status: RESOLVED FIXED

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

Description

[Tamara Hills [:thills]]

Currently, contacts, phone, and SMS are all under the Communications App Manifest.  For more granular metrics reporting, it will be good have have the hud identify the sub-app vs. just "communications".

Comment 1

[Russ Nicoletti [:russn]]

I have a patch for this I will be attaching shortly. I uses the frame's 'src' attribute to find the app name when the manifest "app name" is 'communications'

Comment 2

[Russ Nicoletti [:russn]]

Attachment: correctly report name of 'communication' apps

Comment 3

[Russ Nicoletti [:russn]]

Attachment: properly report names of communications apps

This is a rebased patch.

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8652054 [details] [diff] [review]
properly report names of communications apps

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

Thanks Russ! Just a few nits but they should be easy to fix.

::: b2g/chrome/content/devtools/hud.js
@@ +210,5 @@
>    },
>  
> +  get appName() {
> +
> +    if (!this._appName) {

Nit: Prefer bail-out style:

> if (this._appName) {
>   return this._appName;
> }
> // rest of code not indented

@@ -339,5 @@
>  
> -    if (!this.appName) {
> -      let manifest = this.manifest;
> -      if (!manifest) {
> -        return;

When there is no manifest, `_logHistogram()` used to return here instead of continuing.

You changed this behavior: Now when there is no manifest, `metric.appName` will just be set to `null` and the function will continue. Is this the new expected behavior?

Comment 5

[Russ Nicoletti [:russn]]

Attachment: properly report names of communications apps

Thanks for catching the change in behavior caused by my patch. That was unintentional. I'm now logging an error for that situation because I don't like silently failing.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8652413 [details] [diff] [review]
properly report names of communications apps

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

Looks good to me!

A few very minor nits: You can choose to address them now, or in a subsequent patch, but I'm OK with this patch as it is.

::: b2g/chrome/content/devtools/hud.js
@@ +217,5 @@
> +
> +    let manifest = this.manifest;
> +    if (!manifest) {
> +      let msg = DEVELOPER_HUD_LOG_PREFIX + ': Unable to determine app for telemetry metric. src: ' +
> +                this.frame.src; 

Nit: Trailing space.

@@ +218,5 @@
> +    let manifest = this.manifest;
> +    if (!manifest) {
> +      let msg = DEVELOPER_HUD_LOG_PREFIX + ': Unable to determine app for telemetry metric. src: ' +
> +                this.frame.src; 
> +      console.error(msg);

Nit: Maybe at some point all the prefixed `console.error()` calls could become a helper function? (e.g. `telemetryError()` or `developerHUD.logError()`) But it's fairly unimportant, we don't have to address this now.

@@ +386,5 @@
>          let keyed = Services.telemetry.getKeyedHistogramById(keyedMetricName);
>          if (keyed) {
>            keyed.add(metric.appName, parseInt(metric.value, 10));
>            developerHUD._histograms.add(keyedMetricName);
> +          telemetryDebug(keyedMetricName + ', ' + metric.value + ', ' + metric.appName);

Nit: If you don't care about commas, you could also log them this way: `telemetryDebug(keyedMetricName, metric.value, metric.appName)`, the proxied `console.log()` will separate these values with spaces, and won't have to coerce the values to a single string.

Comment 7

[Russ Nicoletti [:russn]]

Attachment: properly report names of communications apps

Updated patch, addressing final review nits

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc4b4cc673c

Comment 8

[Russ Nicoletti [:russn]]

Attachment: properly report names of communications apps

Rebased patch.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6474bb688d11

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/527c66668f40

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/527c66668f40