[Metrics] addonHistograms missing a level

RESOLVED FIXED

Status

Firefox OS
Metrics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: thills, Assigned: thills)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

2 years ago
Blocks: 1152000
(Assignee)

Updated

2 years ago
Assignee: nobody → thills
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8694229 [details] [diff] [review]
Gecko Portion

Hi Jan,

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

Thanks,
-tamara
Attachment #8694229 - Flags: review?(janx)
(Assignee)

Comment 2

2 years ago
Created attachment 8694307 [details] [review]
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
Attachment #8694307 - Flags: review?(marshall)
Comment on attachment 8694307 [details] [review]
Gaia Portion

lgtm
Attachment #8694307 - Flags: review?(marshall) → review+

Comment 4

2 years ago
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.
Attachment #8694229 - Flags: review?(janx) → feedback+

Comment 5

2 years ago
Created attachment 8698057 [details] [review]
[gaia] tamarahills:bugfix/Bug-1229083-AddonHistograms-missing-level > mozilla-b2g:master
(Assignee)

Comment 6

2 years ago
Created attachment 8698126 [details] [diff] [review]
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
Attachment #8694229 - Attachment is obsolete: true
Attachment #8694307 - Attachment is obsolete: true
Attachment #8698126 - Flags: review?(janx)
(Assignee)

Comment 7

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

Comment 8

2 years ago
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`?
Attachment #8698126 - Flags: review?(janx) → review+
(Assignee)

Comment 9

2 years ago
Created attachment 8698480 [details] [diff] [review]
1229083-addonHistogram-level

Updated patch with nits fixed from review.  Thank you Jan for the review.
Attachment #8698126 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
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
Keywords: checkin-needed, leave-open
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 11

2 years ago
Created attachment 8699465 [details] [diff] [review]
Updated gecko portion

Updated patch to fix a nit.
Attachment #8698480 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
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
Keywords: checkin-needed

Comment 13

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/371837ab6f35
(Assignee)

Comment 15

2 years ago
https://github.com/mozilla-b2g/gaia/commit/2756c03ff64fca5aa1355dce576a5efd30e26ae3
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.