1122058 - Telemetry for New Performance Tool

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Beyond general open/closing of the tool:

* Duration of each profile
* How many "recordings" are done at once (how many items are in the recording list on the side)
* Metrics on save/importing
* Most common "details" view
* Counter of manually started recordings
* Counter of console[profile|timeline] started recordings

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Going through current telemetry (bug 1098374), noticed that there are some changes to how we're collecting this information. Is there a good source to look at how it -should- be done? Including the basic opening/closing of the tool, as well as the extra info to collect from comment #1?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

There are some comments at the top of browser/devtools/shared/telemetry.js:
```
/**
 * Telemetry.
 *
 * To add metrics for a tool:
 *
 * 1. Create boolean, flag and exponential entries in
 *    toolkit/components/telemetry/Histograms.json. Each type is optional but it
 *    is best if all three can be included.
 *
 * 2. Add your chart entries to browser/devtools/shared/telemetry.js
 *    (Telemetry.prototype._histograms):
 *    mytoolname: {
 *      histogram: "DEVTOOLS_MYTOOLNAME_OPENED_BOOLEAN",
 *      userHistogram: "DEVTOOLS_MYTOOLNAME_OPENED_PER_USER_FLAG",
 *      timerHistogram: "DEVTOOLS_MYTOOLNAME_TIME_ACTIVE_SECONDS"
 *    },
 *
 * 3. toolbox.js will automatically ping telemetry with your tools opening and
 *    timing information.
 *
 * Note:
 * You can view telemetry stats for your local Firefox instance via
 * about:telemetry.
 *
 * You can view telemetry stats for large groups of Firefox users at
 * telemetry.mozilla.org.
 */
```

For extra stuff you can create the histogram then use this pattern:
```
this._telemetry = new Telemetry();
this._telemetry.log(TABS_OPEN_PEAK_HISTOGRAM, 10);
```

To decide which charts to use check:
https://developer.mozilla.org/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

If you need to know anything else then feel free to give me a shout.

Comment 3

[Victor Porof [:vporof][:vp]]

We already have the telemetry hooks that were available for the old profiler. Adding new things can be done after shipping.

Comment 4

[Victor Porof [:vporof][:vp]]

Bug 1075567 was repurposed to mean "v2". Unblocking.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Should get these in sooner than later -- what else can we track?

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Thinking about types[0] of telemetry and what we'd want to know -- I'd like way more granular than telemetry allows, but this is a good start, I think. Will let us know how many people are importing/exporting, what views are in use (how can we get time per view?) and whether or not people are using certain options.

* DURATION: record final duration upon recording completion (exponential)
* RECORDING: record and increment the number of recordings done (count)
* CONSOLE_PROFILE: record how many times a console.profile() recording is done (count)
* RECORDING_EXPORT: whether a recording was exported (flag)
* RECORDING_IMPORT: whether a recording was imported (flag)
* VIEWS_USED: whether a view was used in the session, one for each view (flag)
* OPTIONS_USED: on load, and on change, check options used for recording and set to true if it was used at all in session (flag)

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1122058-telemetry.patch

So here are some telemetry results. Biggest question is are these all the best types of metrics to record, or if there are any other things we should be recording.

The DEVTOOLS_PERFTOOLS_SELECTED_VIEW_MS one starts and stops timers when changing views, and want to make sure I understand "keyed" values correctly. And if it's ok just using "true" as a value for flags and counts.

If this looks good I'll add some tests.

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8645390 [details] [diff] [review]
1122058-telemetry.patch

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

Really happy to see all of these.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1122058-telemetry.patch

Fixed up a few things, added tests; Victor for changes and workings in performance/*, Mike for changes to telemetry and the types of histograms used

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=125fa25b1138

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

NI for privacy review

Comment 12

[Benjamin Smedberg]

Comment on attachment 8658955 [details] [diff] [review]
1122058-telemetry.patch

Since this is opt-in (prerelease), the requirements aren't very strict for these. My comments/questions:

Who is responsible for monitoring the data? Especially for measurements that never expire, we still want a clear reason to collect permanently and a designated owner who commits to monitoring the data at a minimum monthly.

Are the existing telemetry dashboards sufficient for monitoring, or will you be developing a custom dashboard for monitoring?

The docs in histograms.json are pretty light. In particular for the keyed histogram DEVTOOLS_PERFTOOLS_SELECTED_VIEW_MS and DEVTOOLS_PERFTOOLS_RECORDING_FEATURES_USED, what kinds of things will be in the key? We need to make sure that isn't privacy-sensitive. Do you have examples?

n_buckets: 10000 is much too large. The data is sent in an array of n_buckets size, e.g. [0,0,0,0,1,4,0,0,0,0,0,...] and so large numbers cost a lot of bandwidth for the user and money to parse and store. Typically we want n_buckets to be less than 100, and hopefully smaller than that.

Please set feedback? when this is done.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@bsmedberg: Jeff Griffiths is responsible for monitoring our telemetry data so you are best speaking to him.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@jsantell: bsmedberg's n_buckets comment should obviously be addressed before we land this.

Comment 15

[Benjamin Smedberg]

I'll want to mark a privacy feedback+ before you land this as well.

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1122058-telemetry.patch

Lowered n_buckets to 20
Added better descriptions and example data for the keyed fields

I'm not sure if the current dashboards are sufficient for this data -- Jeff I think can answer that better than I can.

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)
> Created attachment 8659408 [details] [diff] [review]
> 1122058-telemetry.patch
> 
> Lowered n_buckets to 20
> Added better descriptions and example data for the keyed fields
> 
> I'm not sure if the current dashboards are sufficient for this data -- Jeff
> I think can answer that better than I can.

The current custom dashboard I have doesn't use this at all. Telemetry is in the middle of changing a lot ( the new main dashboard is potentially much more useful as-is ) so I think we need some time to evaluate if it's enough. I got pretty good results with the system / screensize measures we added recently just by exporting csv into a spreadsheet.

Comment 18

[Benjamin Smedberg]

Comment on attachment 8659408 [details] [diff] [review]
1122058-telemetry.patch

It turns out I was completely wrong about n_buckets: we record those using a keyed system that only records the buckets. I still recommend smaller buckets to make it easier to visualize, but it's not necessary.

I still need to have a definite answer that Jeff is committing to monitoring this data regularly and either has or will develop the dashboards to do that.

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> Comment on attachment 8659408 [details] [diff] [review]
> 1122058-telemetry.patch
> 
> It turns out I was completely wrong about n_buckets: we record those using a
> keyed system that only records the buckets. I still recommend smaller
> buckets to make it easier to visualize, but it's not necessary.
> 
> I still need to have a definite answer that Jeff is committing to monitoring
> this data regularly and either has or will develop the dashboards to do that.

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> > Comment on attachment 8659408 [details] [diff] [review]
> > 1122058-telemetry.patch
> > 
> > It turns out I was completely wrong about n_buckets: we record those using a
> > keyed system that only records the buckets. I still recommend smaller
> > buckets to make it easier to visualize, but it's not necessary.
> > 
> > I still need to have a definite answer that Jeff is committing to monitoring
> > this data regularly and either has or will develop the dashboards to do that.

Yes we are and I think the new generic dashboard is good enough for now. needinfo'ing to ensure you see this.

Comment 21

[Benjamin Smedberg]

Yep, we're all good then.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5d0470be49b3

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/206f2986ff80

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5d0470be49b3
https://hg.mozilla.org/mozilla-central/rev/206f2986ff80