[Metrics] Custom metrics should support more complex histogram types

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: thills, Assigned: thills)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

3 years ago
Assignee: nobody → thills
Blocks: 1152000
(Assignee)

Comment 1

3 years ago
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.
Status: NEW → ASSIGNED
Summary: [Metrics] Expand the advanced telemetry app specific histogram collection to be more sophisticated → [Metrics] Custom metrics should support more complex histogram types
(Assignee)

Comment 2

3 years ago
Created attachment 8634367 [details] [diff] [review]
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
Attachment #8634367 - Flags: feedback?(rnicoletti)
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';
Attachment #8634367 - Flags: feedback?(rnicoletti) → feedback+
(Assignee)

Comment 4

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

Updated

3 years ago
Depends on: 1183101
Created attachment 8644505 [details] [review]
[gaia] tamarahills:bugfix/1176992-Support-histograms-in-custom-metrics > mozilla-b2g:master
(Assignee)

Comment 6

3 years ago
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
Attachment #8644505 - Flags: review?(rnicoletti)
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.
Attachment #8644505 - Flags: review?(rnicoletti) → review+
(Assignee)

Comment 8

3 years ago
(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.
Flags: needinfo?(rnicoletti)
I added some more comments. Primarily I was wondering if '0' is a valid minimum value.
Flags: needinfo?(rnicoletti)
(Assignee)

Comment 10

3 years ago
Created attachment 8646963 [details] [diff] [review]
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
Attachment #8634367 - Attachment is obsolete: true
Attachment #8646963 - Flags: review?(jryans)
Comment on attachment 8646963 [details] [diff] [review]
Gecko Portion

Jan has returned from PTO, so let's have him take a look.
Attachment #8646963 - Flags: review?(jryans) → review?(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.
Attachment #8646963 - Flags: review?(janx) → feedback+
Blocks: 1180699
feature-b2g: --- → 2.5+
(Assignee)

Comment 13

3 years ago
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
Attachment #8644505 - Flags: review+ → review?(marshall)
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+ :)
Attachment #8644505 - Flags: review?(marshall) → review-
(Assignee)

Comment 15

3 years ago
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
Attachment #8644505 - Flags: review?(marshall)
Attachment #8644505 - Flags: review-
Attachment #8644505 - Flags: feedback?(rnicoletti)
Attachment #8644505 - Flags: feedback?(rnicoletti) → feedback+
I've left my comments in the PR.
(Assignee)

Comment 17

3 years ago
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
Attachment #8644505 - Flags: review?(marshall)
(Assignee)

Comment 18

3 years ago
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
Attachment #8644505 - Flags: feedback+ → feedback?(rnicoletti)
(Assignee)

Comment 19

3 years ago
Created attachment 8651958 [details] [diff] [review]
1176992 - Gecko Portion

Hi Jan,

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

thanks,

-tamara
Attachment #8646963 - Attachment is obsolete: true
Attachment #8651958 - Flags: review?(janx)
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.
Attachment #8644505 - Flags: feedback?(rnicoletti) → feedback+
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 `===`).
Attachment #8651958 - Flags: review?(janx) → feedback+
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.
(Assignee)

Comment 23

3 years ago
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
Attachment #8644505 - Flags: review?(marshall)
(Assignee)

Comment 24

3 years ago
Created attachment 8652528 [details] [diff] [review]
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
Attachment #8651958 - Attachment is obsolete: true
Attachment #8652528 - Flags: review?(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".
Attachment #8652528 - Flags: review?(janx) → review+
(Assignee)

Comment 26

3 years ago
Created attachment 8652825 [details] [diff] [review]
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
(Assignee)

Comment 27

3 years ago
Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d2c16633cd
Keywords: checkin-needed
Attachment #8652528 - Attachment is obsolete: true
Please post a patch with a proper commit message. Also, should this be marked leave-open for the Gaia patch?
Keywords: checkin-needed
(Assignee)

Comment 29

3 years ago
Created attachment 8653493 [details] [diff] [review]
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
Attachment #8652825 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed, leave-open
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1166375
Attachment #8644505 - Flags: review?(marshall) → review+
(Assignee)

Updated

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