Closed Bug 1205845 Opened 4 years ago Closed 3 years ago

Implement telemetry measure to track toolbox docking setting

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox43 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox43 --- affected
firefox51 --- fixed

People

(Reporter: canuckistani, Assigned: darkowlzz, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [good first bug])

Attachments

(1 file, 7 obsolete files)

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)
Attachment #8778600 - Flags: feedback?(jgriffiths)
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.
Attached image toolboxHost under Environment Data (obsolete) —
Redirecting to Bryan, who is devtools PM these days.
Flags: needinfo?(clarkbw)
Attachment #8778600 - Flags: feedback?(jgriffiths)
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]
Attachment #8778600 - Attachment is obsolete: true
Attachment #8778601 - Attachment is obsolete: true
Attachment #8780913 - Flags: feedback?(jryans)
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+
(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)
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Improved patch.
Attachment #8780914 - Attachment is obsolete: true
Attachment #8783292 - Flags: feedback?(jryans)
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+
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 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)
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
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
https://hg.mozilla.org/mozilla-central/rev/306233ac26ed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1389995
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.