1229083 - [Metrics] addonHistograms missing a level

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Metrics, defect)

Description

[Tamara Hills [:thills]]

Currently, gecko is just reporting the following format for histograms:

payload: {
  addonHistograms: {
    customMetricName: {
      min:
      max: 
      ....
    }     
  }
}

it should report the following to be compatible with the spark processing:
payload: {
  addonHistograms: {
    addonName: {
      customMetricName: {
        min:
        max: 
        ...
      }     
    }
  }
}

This needs a gecko portion and a gaia portion to fix it.

Comment 1

[Tamara Hills [:thills]]

Attachment: Gecko Portion

Hi Jan,

This patch fixes a problem where the addon name was being left out of the addonHistograms portion.

Thanks,
-tamara

Comment 2

[Tamara Hills [:thills]]

Attachment: Gaia Portion

Hi Marshall,

This adds back the addon level to the addonHistograms that was missing as it was not being properly sent from gecko. 

Thanks,

-tamara

Comment 3

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8694307 [details] [review]
Gaia Portion

lgtm

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8694229 [details] [diff] [review]
Gecko Portion

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

If I understand `Services.telemetry.addonHistogramSnapshots` correctly, I think your approach won't work if one day there are other unrelated addonHistograms in Firefox OS.

::: b2g/chrome/content/devtools/hud.js
@@ +353,5 @@
>        payload.keyedHistograms[item] =
>          Services.telemetry.getKeyedHistogramById(item).snapshot();
>      });
>      // Package the registered hud custom histograms
> +    payload.addonHistograms = Services.telemetry.addonHistogramSnapshots;

I understand that this works today, but what if one day Firefox OS Addons are allowed to use Telemetry? Or if some other code starts using addonHistograms? This would cause Advanced Telemetry to send unrelated addonHistograms in its payload.

Instead, isn't it possible to do something like the following?

    developerHUD._customHistograms.forEach(item => {
      let addonHistograms = payload.addonHistograms;
      let addonHistogram = this._getAddonHistogram(item);
      let addonName = item.split('_')[3].toUpperCase(); // FIXME
      if (!(addonName in addonHistograms)) {
        addonHistograms[addonName] = {};
      }
      addonHistograms[addonName][item] = addonHistogram.snapshot();
    });

To clean this up, maybe you could add a _getAddonHistogramName() helper, and use it both here to get addonName and in _getAddonHistogram().

Another option would be to filter `Services.telemetry.addonHistogramSnapshots` according to what you have in `this._customHistograms`, but that sounds more complicated.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/Bug-1229083-AddonHistograms-missing-level > mozilla-b2g:master

Comment 6

[Tamara Hills [:thills]]

Attachment: Updated gecko portion

Hi Jan,

I updated this patch to handle that case that you mentioned to make sure we only ship the histograms we are interested in.  Also, I added a check to make sure that we don't unneccessarily send histograms that have no data.

Thanks,

tamara

Comment 7

[Tamara Hills [:thills]]

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=f631b2ab61bf729a81a6e4600bdd77c6c3712058

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8698126 [details] [diff] [review]
Updated gecko portion

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

Thank you Tamara! Two nits.

::: b2g/chrome/content/devtools/hud.js
@@ +320,5 @@
>    },
>  
>    _getAddonHistogram(item) {
> +    return Services.telemetry.getAddonHistogram(this._getAddonHistogramName(item, APPNAME_IDX),
> +      CUSTOM_HISTOGRAM_PREFIX + this._getAddonHistogramName(item, HISTNAME_IDX));

Nit: This would be cleaner with shorthand variables, e.g. `let appName = getAddonHistogramName(...)`.

@@ +359,5 @@
>      developerHUD._customHistograms.forEach(item => {
> +      let appName = this._getAddonHistogramName(item, APPNAME_IDX);
> +      let histName = CUSTOM_HISTOGRAM_PREFIX +
> +        this._getAddonHistogramName(item, HISTNAME_IDX);
> +      let addoHist = Services.telemetry.getAddonHistogram(appName, histName).snapshot();

Nit: `addoHist` is not a great name. Maybe `addonHist` or `addonHistogram`?

Comment 9

[Tamara Hills [:thills]]

Attachment: 1229083-addonHistogram-level

Updated patch with nits fixed from review.  Thank you Jan for the review.

Comment 10

[Tamara Hills [:thills]]

checkin-needed for gecko portion.  Please leave-open as I need to land the gaia portion as well before closing.

try run for gecko portion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22f2d63c0f59

Comment 11

[Tamara Hills [:thills]]

Attachment: Updated gecko portion

Updated patch to fix a nit.

Comment 12

[Tamara Hills [:thills]]

Updated try run for the nit.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f33cba05afad

Checkin-needed on the gecko portion.  Please leave open so I can land the Gaia portion.

thank you,

-tamara

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/371837ab6f35

Comment 15

[Tamara Hills [:thills]]

https://github.com/mozilla-b2g/gaia/commit/2756c03ff64fca5aa1355dce576a5efd30e26ae3