Closed Bug 1122058 Opened 5 years ago Closed 4 years ago

Telemetry for New Performance Tool

Categories

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

37 Branch
x86
macOS
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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?
Flags: needinfo?(mratcliffe)
No longer blocks: 1077462
No longer blocks: perf-tool-v2
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.
Flags: needinfo?(mratcliffe)
We already have the telemetry hooks that were available for the old profiler. Adding new things can be done after shipping.
Blocks: perf-tool-v2
No longer blocks: enable-perf-tool
Bug 1075567 was repurposed to mean "v2". Unblocking.
No longer blocks: perf-tool-v2
No longer blocks: perf-tool-v2
Should get these in sooner than later -- what else can we track?
Blocks: perf-tools-fx43
No longer blocks: perf-tools-fx42
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
Attached patch 1122058-telemetry.patch (obsolete) — Splinter Review
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.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8645390 - Flags: feedback?(vporof)
Attachment #8645390 - Flags: feedback?(mratcliffe)
Comment on attachment 8645390 [details] [diff] [review]
1122058-telemetry.patch

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

Really happy to see all of these.
Attachment #8645390 - Flags: feedback?(vporof) → feedback+
Attachment #8645390 - Flags: feedback?(mratcliffe) → feedback+
Attached patch 1122058-telemetry.patch (obsolete) — Splinter Review
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
Attachment #8645390 - Attachment is obsolete: true
Attachment #8658955 - Flags: review?(vporof)
Attachment #8658955 - Flags: review?(mratcliffe)
NI for privacy review
Flags: needinfo?(benjamin)
Attachment #8658955 - Flags: review?(vporof) → review+
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.
Flags: needinfo?(benjamin) → needinfo?(jsantell)
Attachment #8658955 - Flags: review?(mratcliffe) → review+
@bsmedberg: Jeff Griffiths is responsible for monitoring our telemetry data so you are best speaking to him.
Flags: needinfo?(jsantell) → needinfo?(jgriffiths)
@jsantell: bsmedberg's n_buckets comment should obviously be addressed before we land this.
Flags: needinfo?(jsantell)
I'll want to mark a privacy feedback+ before you land this as well.
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.
Attachment #8658955 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8659408 - Flags: review+
Attachment #8659408 - Flags: feedback?(benjamin)
(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.
Flags: needinfo?(jgriffiths)
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.
Attachment #8659408 - Flags: feedback?(benjamin) → feedback+
(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.
Flags: needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths) → needinfo?(benjamin)
Yep, we're all good then.
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/5d0470be49b3
https://hg.mozilla.org/mozilla-central/rev/206f2986ff80
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.