Closed
Bug 1205845
Opened 7 years ago
Closed 6 years ago
Implement telemetry measure to track toolbox docking setting
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox43 affected, firefox51 fixed)
RESOLVED
FIXED
Firefox 51
People
(Reporter: canuckistani, Assigned: darkowlzz, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [good first bug])
Attachments
(1 file, 7 obsolete files)
8.69 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
Basically, let's track values for 'devtools.toolbox.host'.
Let's do something! Mike, maybe you'd like to?
Flags: needinfo?(mratcliffe)
At the moment I really need to focus on making the Storage Inspector read only.
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8778600 -
Flags: feedback?(jgriffiths)
Assignee | ||
Comment 4•6 years ago
|
||
Hi, Came across this bug and thought of working on it. After going through about:telemetry, I felt `toolboxHost` could be placed under Environment Data > settings. Created a patch. Correct me if there is a better place than Environment Data. Also, attaching a picture of how it looks.
Assignee | ||
Comment 5•6 years ago
|
||
Reporter | ||
Comment 6•6 years ago
|
||
Redirecting to Bryan, who is devtools PM these days.
Flags: needinfo?(clarkbw)
Reporter | ||
Updated•6 years ago
|
Attachment #8778600 -
Flags: feedback?(jgriffiths)
Comment 7•6 years ago
|
||
Thanks for working on this! I don't think we need it to display in the about:telemetry or at least I don't believe we have other devtools items in there besides the historgrams section. I believe this bug was primarily about implementing the telemetry probe to capture the data. Something like bug 1247985 might have some relevant code. Adding Ryan to help out.
Flags: needinfo?(clarkbw) → needinfo?(jryans)
Right, like :clarkbw mentioned, we'd like to add a new telemetry probe for this case so we're able to track how much each docking mode is used. There is a general telemetry module[1] in DevTools. Also check out the MDN page[2] about adding new probes. It looks like the toolbox has a good place[3] to add an extra probe like this. After discussing this a bit on IRC with :clarkbw, we're thinking the best approach would be a new probe of type "count". It would also be "keyed", where the keys are each of the possible docking types. We should increment the appropriate count each time the toolbox is opened[3] and also when the docking mode is changed[4]. [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/telemetry.js [2]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe [3]: https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/framework/toolbox.js#502 [4]: https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/framework/toolbox.js#1808
Mentor: jryans
Flags: needinfo?(jryans)
Whiteboard: [good first bug]
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8778600 -
Attachment is obsolete: true
Attachment #8778601 -
Attachment is obsolete: true
Attachment #8780913 -
Flags: feedback?(jryans)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8780913 -
Attachment is obsolete: true
Attachment #8780913 -
Flags: feedback?(jryans)
Attachment #8780914 -
Flags: feedback?(jryans)
:clarkbw, do we want opt-in or opt-out collection for this on release?
Flags: needinfo?(clarkbw)
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
Comment on attachment 8780914 [details] [diff] [review] Add telemetry probe for toolbox host setting Review of attachment 8780914 [details] [diff] [review]: ----------------------------------------------------------------- It's looking good overall, thanks for adding a test as well! A few changes to make, so I'd like to see another version. ::: devtools/client/framework/test/browser_toolbox_hosts_telemetry.js @@ +22,5 @@ > + yield toolbox.destroy(); > + > + toolbox = target = null; > + gBrowser.removeCurrentTab(); > + resetTelemetry(Telemetry); Call `resetTelemetry` from `registerCleanupFunction` to be sure it happens even if the test blows up in the middle (so that further tests in the run aren't impacted). @@ +46,5 @@ > +function checkResults(Telemetry) { > + let result = Telemetry.prototype.telemetryInfo; > + for (let [histId, value] in Iterator(result)) { > + if (histId === "DEVTOOLS_TOOLBOX_HOST_SETTING|bottom") { > + ok(value.length === 3, `${histId} has 2 successful entries`); 2 -> 3 @@ +54,5 @@ > + ok(value.length === 2, `${histId} has 2 successful entries`); > + checkEveryValue(histId, value); > + } > + if (histId === "DEVTOOLS_TOOLBOX_HOST_SETTING|window") { > + ok(value.length === 2, `${histId} has 1 successful entry`); 1 -> 2 ::: devtools/client/framework/test/head.js @@ +153,5 @@ > + * it. > + * Store all recordings in Telemetry.telemetryInfo. > + * @return {Telemetry} > + */ > +function mockTelemetry() { There is a very similar function (though without keyed support in shared/test/head.js called `loadTelemetryAndRecordLogs`. Let's move it to the file framework/test/shared-head.js, which is a common used across multiple directories. Then add logging for keyed values to that version. ::: devtools/client/framework/toolbox.js @@ +508,5 @@ > Services.appinfo.is64Bit ? 1 : 0); > this._telemetry.logOncePerBrowserVersion(SCREENSIZE_HISTOGRAM, system.getScreenDimensions()); > + > + this._telemetry.logKeyed(HOST_HISTOGRAM, > + this.hostType || "UNKNOWN", true); Since the `hostType` value is lowercase, let's put unknown in lowercase too. ::: toolkit/components/telemetry/Histograms.json @@ +7040,4 @@ > "keyed": true, > "description": "Measures whether a particular JavaScript error has been displayed in the webconsole." > }, > + "DEVTOOLS_TOOLBOX_HOST_SETTING": { "SETTING" feels a bit redundant, so I think it should be removed. @@ +7040,5 @@ > "keyed": true, > "description": "Measures whether a particular JavaScript error has been displayed in the webconsole." > }, > + "DEVTOOLS_TOOLBOX_HOST_SETTING": { > + "alert_emails": ["jryans@gmail.com"], Let's use the devtools mailing list, "dev-developer-tools@lists.mozilla.org". @@ +7041,5 @@ > "description": "Measures whether a particular JavaScript error has been displayed in the webconsole." > }, > + "DEVTOOLS_TOOLBOX_HOST_SETTING": { > + "alert_emails": ["jryans@gmail.com"], > + "expires_in_version": "never", Privacy team doesn't let us do this anymore, so let's pick some number. There are roughly 7 releases per year and we're in 51 right now, so let's say "58". @@ +7045,5 @@ > + "expires_in_version": "never", > + "kind": "count", > + "bug_numbers": [1205845], > + "keyed": true, > + "description": "Measures whether a particular JavaScript error has been displayed in the webconsole." "Records DevTools toolbox host (side, bottom, etc.) each time the toolbox is opened and when the host is changed."
Attachment #8780914 -
Flags: feedback?(jryans) → feedback+
Comment 13•6 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11) > :clarkbw, do we want opt-in or opt-out collection for this on release? Opt-Out, the lions share of our users are on release so we need to know what their configuration looks like.
Flags: needinfo?(clarkbw)
Assignee | ||
Comment 15•6 years ago
|
||
Improved patch.
Attachment #8780914 -
Attachment is obsolete: true
Attachment #8783292 -
Flags: feedback?(jryans)
Assignee | ||
Comment 16•6 years ago
|
||
Attachment #8783292 -
Attachment is obsolete: true
Attachment #8783292 -
Flags: feedback?(jryans)
Attachment #8783293 -
Flags: feedback?(jryans)
Comment on attachment 8783293 [details] [diff] [review] Add telemetry probe for toolbox host Review of attachment 8783293 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good! I'd like to see one more round to double check the test function cleanup. ::: devtools/client/framework/test/browser_toolbox_hosts_telemetry.js @@ +1,1 @@ > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ Leave out these editor config lines, we should really remove them from older files... ::: devtools/client/framework/test/shared-head.js @@ +511,5 @@ > + * it. > + * Store all recordings in Telemetry.telemetryInfo. > + * @return {Telemetry} > + */ > +function loadTelemetryAndRecordLogs() { Since this is now the "improved" version with keyed support, you should remove the older `loadTelemetryAndRecordLogs` from devtools/client/shared/test/head.js so this will be used everywhere. @@ +539,5 @@ > + * mock methods and objects. > + * @param {Telemetry} Required Telemetry > + * Telemetry object that needs to be reset. > + */ > +function resetTelemetry(Telemetry) { We also want this to replace `stopRecordingTelemetryLogs` from devtools/client/shared/test/head.js. Do one of the following: A: Rename this to `stopRecordingTelemetryLogs`, remove older `stopRecordingTelemetryLogs` from devtools/client/shared/test/head.js B: Remove older `stopRecordingTelemetryLogs` from devtools/client/shared/test/head.js, change all callers to use `resetTelemetry` now
Attachment #8783293 -
Flags: feedback?(jryans) → feedback+
Assignee | ||
Comment 18•6 years ago
|
||
Removed `loadTelemetryAndRecordLogs` and `stopRecordingTelemetryLogs` from head.js and added them to shared-head.js. Renamed `resetTelemetry` to `stopRecordingTelemetryLogs` and changed the places where it was used.
Attachment #8783293 -
Attachment is obsolete: true
Attachment #8783686 -
Flags: feedback?(jryans)
Comment on attachment 8783686 [details] [diff] [review] Add telemetry probe for toolbox host Review of attachment 8783686 [details] [diff] [review]: ----------------------------------------------------------------- I think this version looks good! Do you need any help running this on try to make sure the test changes are looking good? ::: devtools/client/framework/test/browser_toolbox_hosts_telemetry.js @@ +1,1 @@ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ This vim mode line should also be removed.
Attachment #8783686 -
Flags: feedback?(jryans) → review+
Comment on attachment 8783686 [details] [diff] [review] Add telemetry probe for toolbox host :bsmedberg, we're adding a new telemetry probe to track the docking style of the DevTools toolbox. It is currently marked as opt-out on release as well. :clarkbw will include this probe in his DevTools metrics dashboard.
Attachment #8783686 -
Flags: feedback?(benjamin)
Comment 21•6 years ago
|
||
Comment on attachment 8783686 [details] [diff] [review] Add telemetry probe for toolbox host There are a limited number of sides, right (five maybe? left right top bottom window)? For efficiency, this should probably be enumerated histogram rather than keyed. Keys are less efficient to send and process and are generally reserved for less predictable data. data-review=me with that change, or come back to me if that isn't workable for some reason
Attachment #8783686 -
Flags: feedback?(benjamin) → feedback+
Ah, that's a good point! :darkowlzz, can you update the patch to make this change? In the histogram definition, remove keyed, change to kind "enumerated", and set "n_values" to something like 10 so there's room to grow in case we add more types. You'll need to create a function to map the host type to a number. It probably makes sense to use the max value for the unknown case. I know you already made a few testing changes to better support keyed histograms. I think it's fine to keep that work in the patch if you'd like to, as it's helpful for the next case of keyed. Please also add "r=jryans p=bsmedberg" to the commit message in your patch.
Flags: needinfo?(indiasuny000)
Assignee | ||
Comment 23•6 years ago
|
||
Keeping keyed histogram work in the patch. Changed histogram kind to enumerated and changed the tests accordingly. I lost my try-server keys. Have to apply for a new one. Could you push it?
Attachment #8783686 -
Attachment is obsolete: true
Flags: needinfo?(indiasuny000)
Attachment #8785702 -
Flags: review?(jryans)
Comment on attachment 8785702 [details] [diff] [review] Add telemetry probe for toolbox host Review of attachment 8785702 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me locally, thanks for working on this! Let's see what try thinks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25c4e763e11d
Attachment #8785702 -
Flags: review?(jryans) → review+
Looks like the check for null telemetryInfo was still needed in the test helper. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=271a97cf7940
Comment 26•6 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/306233ac26ed Implement telemetry measure to track toolbox docking setting; r=jryans p=bsmedberg
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/306233ac26ed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•