1205845 - Implement telemetry measure to track toolbox docking setting

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

Basically, let's track values for 'devtools.toolbox.host'.

Comment 1

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

Let's do something!  Mike, maybe you'd like to?

Comment 2

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

At the moment I really need to focus on making the Storage Inspector read only.

Comment 3

[Sunny [:darkowlzz]]

Attachment: Add toolbox docking setting to telemetry

Comment 4

[Sunny [:darkowlzz]]

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.

Comment 5

[Sunny [:darkowlzz]]

Attachment: toolboxHost under Environment Data

Comment 6

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

Redirecting to Bryan, who is devtools PM these days.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 8

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

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

Comment 9

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host setting

Comment 10

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host setting

Comment 11

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

:clarkbw, do we want opt-in or opt-out collection for this on release?

Comment 12

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

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": ["[e-mail]"],

Let's use the devtools mailing list, "[e-mail]".

@@ +7041,5 @@
>      "description": "Measures whether a particular JavaScript error has been displayed in the webconsole."
>    },
> +  "DEVTOOLS_TOOLBOX_HOST_SETTING": {
> +    "alert_emails": ["[e-mail]"],
> +    "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."

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

(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.

Comment 14

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 15

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host

Improved patch.

Comment 16

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host

Comment 17

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

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

Comment 18

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host

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.

Comment 19

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

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.

Comment 20

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

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.

Comment 21

[Benjamin Smedberg]

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

Comment 22

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

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.

Comment 23

[Sunny [:darkowlzz]]

Attachment: Add telemetry probe for toolbox host

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?

Comment 24

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

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

Comment 25

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

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/306233ac26ed
Implement telemetry measure to track toolbox docking setting; r=jryans p=bsmedberg

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/306233ac26ed