1176992 - [Metrics] Custom metrics should support more complex histogram types

Status: RESOLVED FIXED

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

Description

[Tamara Hills [:thills]]

Currently, the AdvancedTelemetryHelper just collects histograms from certified apps as a counter.  We need to expand this to a more sophisticated set of types of histograms that we collect.

Comment 1

[Tamara Hills [:thills]]

Right now the custom metrics only supports one type of histogram. We want to be able to support multiple types as opposed to just counters.

Comment 2

[Tamara Hills [:thills]]

Attachment: 1176992.diff

Hi Russ,

Attached a diff for you for what I initially thought of for the changes to the API for advanced_telemetry_helper.

There were two ways I was thinking of:
1. Have one function that covers all type of histograms.
2. Have a separate API for count histograms vs. exponential and linear.

I ultimately thought of option 2 since the app developer needs to have some basic knowledge of what they are trying to do:  e.g. keep a simple count or track values on an exponential graph.

I thought option 1 will lead to a lot of complexity for the app developer and also lot of extra checking on each API call.

Anyhow, let me know what you think will be best from an app developer perspective and then I'll start cleaning it up and then I  also need to work on the gecko part and tests.

Thanks,

-tamara

Comment 3

[Russ Nicoletti [:russn]]

Comment on attachment 8634367 [details] [diff] [review]
1176992.diff

I like option 2, having API functions per type of data being recorded. I think it will be more intuitive and understandable compared to having a single function (and it will force developers to think about how their data should be stored/represented).

One thing I'm not sure of is how the HUD backend (hud.js) will know what the developer is intending in terms of the type of data being recorded. Wouldn't the log entry always need to contain a "type"?

This line is confusing me:

+      message += '|4';

Comment 4

[Tamara Hills [:thills]]

(In reply to Russ Nicoletti [:russn] from comment #3)

> This line is confusing me:
> 
> +      message += '|4';

This is because I'm hard-coding in the histogram count type into the message. When gecko gets the message, it will have to match this field to the histogram type:  https://hg.mozilla.org/mozilla-central/file/72835344333f/toolkit/components/telemetry/nsITelemetry.idl#l30

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Comment 6

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Russ,

Can you do a first review of this?  I also need to have it reviewed by Marshall, but wanted to see if we can get ahead on this one and find any problems ahead of time.

Thanks,

-tamara

Comment 7

[Russ Nicoletti [:russn]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Tamara, I have added my review comments.

Comment 8

[Tamara Hills [:thills]]

(In reply to Russ Nicoletti [:russn] (PTO until 8/10/15) from comment #7)
Hi Russ,

Thanks for your review comments.  I flagged you just so you can see the updates I've made.  I need to wait until 1183101 lands before I can land this anyways.

Comment 9

[Russ Nicoletti [:russn]]

I added some more comments. Primarily I was wondering if '0' is a valid minimum value.

Comment 10

[Tamara Hills [:thills]]

Attachment: Gecko Portion

Hi Ryan,
This handles different histogram types including exponential and linear.  There is a gaia portion and a gecko portion.

This patch depends on 1183101 and needs to go in after that.

Thanks,
-tamara

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Jan has returned from PTO, so let's have him take a look.

Comment 12

[Jan Keromnes [:janx]]

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

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

Thank you Tamara! Mostly looks good, but please address the remarks in the other bug first (some of them also apply here) and then re-upload / request review again.

Comment 13

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Marshall,

This is the piece that does the custom histograms.  Essentially, an app developer can create their own histogram via this.

Let me know if you have any questions.  Russ has also reviewed this as wel.

thanks,
-tamara

Comment 14

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

This looks mostly good, but I agree with Russ' observation about encapsulation of histograms in the API, and I'd like to make sure that the API discussion is complete before landing. Check my comments on Github -- I also left a few minor nits

Once the API is sorted out (either way is fine, I have a preference but I'm not trying to assert it here), just tag me again and I'll try to give you a quick r+ :)

Comment 15

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Russ and Marshall,

Here's an updated patch.  

Marshall, Russ and I talked and decided to separate the creation and the logging calls. 

Russ, I wanted your review, but I guess two people can't review (or at least I can't figure out how to ask for two reviewers).  It will be good if you can feedback it before Marshall looks at it. 

Thanks,

-tamara

Comment 16

[Russ Nicoletti [:russn]]

I've left my comments in the PR.

Comment 17

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Marshall,

Russ had a suggestion about putting the initialization in the ctor to make it more lightweight on the consumer of the API.  You can see his comment in the PR.  I'm going to change this a bit.

Thanks,

-tamara

Comment 18

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Russ,

Can you take a look at this and let me know what you think?

I took your suggestion and put the initialization into the ctor.

-tamara

Comment 19

[Tamara Hills [:thills]]

Attachment: 1176992 - Gecko Portion

Hi Jan,

Here is an updated Gecko Portion.  This allows us to handle exponential, linear, and counter-based histograms.

thanks,

-tamara

Comment 20

[Russ Nicoletti [:russn]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Looks good. I left some comments in the PR. Mainly, I think there's an opportunity to remove the 'count' function.

Comment 21

[Jan Keromnes [:janx]]

Comment on attachment 8651958 [details] [diff] [review]
1176992 - Gecko Portion

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

This code is starting to have too many intricate `if/else` clauses, which are hard to follow. Examples and suggestions below.

::: b2g/chrome/content/devtools/hud.js
@@ +385,2 @@
>          }
> +      } else {

If I follow correctly, your if/else structure looks like this:

> if (A) {
>   if (!B) {
>     // code 1
>   }
> } else {
>   // code 2
> }

It's preferable to avoid unnecessary nesting of if/else clauses. You could simplify it like so:

> if (!A) {
>   // code 2
> } else if (!B) {
>   // code 1
> }

Or even better, since `code 1` is rather long and has nested indentation:

> if (!A) {
>   // code 2
>   return;
> }
> if (B) {
>   return;
> }
> // code 1

Bail-out style avoids `else` clauses and makes the code much simpler to read and to debug.

@@ +624,5 @@
>        }
>      } else {
> +      if (telemetryData.length == MIN_CUSTOM_ARGS) {
> +        metric.value = telemetryData[VALUE_IDX];
> +      } else if (telemetryData.length === MAX_CUSTOM_ARGS) {

Nit: You use the `==` operator elsewhere, please consistently use `==` or `===` for all these checks (preferably the stricter `===`).

Comment 22

[Jan Keromnes [:janx]]

I forgot to say thanks for the patch! The nested if/else blocks mostly grew organically, sorry you had to be the one I complain to.

Comment 23

[Tamara Hills [:thills]]

Comment on attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master

Hi Marshall,

Patch is updated with: 
1) simplifying the calls for the user so usage is like this:
//Create the object.
var ath = new AdvancedTelemetryHelper(ATH.HISTOGRAM_LINEAR, 'mylinear', 1, 100, 10);
//Record a metric
ath.add(15);

2) Removed the count function as per Russ' feedback as it was possible to consolidate them.

Thanks,
-tamara

Comment 24

[Tamara Hills [:thills]]

Attachment: 1176992 - Gecko Portion

Hi Jan,

Thanks for the feedbacks and the good suggestions!  I took your suggestion with the early bailouts and I believe I fixed all the '==' to '==='.

Thanks so much for the review.

-tamara

Comment 25

[Jan Keromnes [:janx]]

Comment on attachment 8652528 [details] [diff] [review]
1176992 - Gecko Portion

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

Much better, thanks! R+ with nits.

::: b2g/chrome/content/devtools/hud.js
@@ +365,5 @@
>        }
>      } else {
>        let histogramName = CUSTOM_HISTOGRAM_PREFIX + metricAppName + '_'
>          + metricName;
> +      // This is a call to add a value to existing histogram.

Nit: Missing word "an".

@@ +372,5 @@
> +          CUSTOM_HISTOGRAM_PREFIX + metricName).add(parseInt(metric.value, 10));
> +        return;
> +      }
> +
> +      if (developerHUD._customHistograms.has(histogramName)) {

Nit: Please add a comment to explain this bail-out, e.g. "The histogram already exists and we don't have new data to add".

Comment 26

[Tamara Hills [:thills]]

Attachment: Final gecko patch with nits addressed.

Thanks Jan,

Attaching final patch here.  I will update with try results before I flag it for checkin.

Thanks,

-tamara

Comment 27

[Tamara Hills [:thills]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d2c16633cd

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please post a patch with a proper commit message. Also, should this be marked leave-open for the Gaia patch?

Comment 29

[Tamara Hills [:thills]]

Attachment: Final gecko patch with commit message

Ryan,

I've added commit message and will mark as leave-open for the Gaia patch.

Thank you,

-tamara

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/5f19b3d8430a

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f19b3d8430a

Comment 33

[Tamara Hills [:thills]]

Gaia Try:  https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=cb1bb46ac3bddd8d9588c640758ca471f3c12e30

Comment 34

[Tamara Hills [:thills]]

https://github.com/mozilla-b2g/gaia/commit/4110c791017fa94c7b4cfa6a6112e6464f8be731