Add telemetry for the grid inspector

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Inspector
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 months ago
Priority: -- → P3
(Assignee)

Comment 1

6 months ago
Here's the metrics we're looking for with CSS Grid.  We want this to look like a funnel where the number of Firefox Developers who have seen an element with `display: grid` CSS is our total addressable market.


How many CSS Grid elements DevTools sees
How many times the Layout panel is opened
How many of the Grid layout views are activated
What CSS Grid Layout options are turned on
Comment hidden (mozreview-request)
(Assignee)

Comment 3

6 months ago
You will find that I removed the "navigate" event listener on the layout actor for the tab actor. I found that this "navigate" event actually fires off 4 times per navigation compare to the tab target on the client side. It's actually kinda crazy. 

I am using scalarAdd instead of toolOpened because I found that toolOpened only sets the scalar value to 1, which seems to be very wrong instead of incrementally adding the count.

I am gonna have follow up patches that will make sure we don't count on reload, but that has been a bit tricky since the "will-navigate" event will give you the url of the page that you are navigating to, so, not actually possible to easily save the previous URL before navigation.

The metric information that I posted on comment 1 was actually written by clarkbw.
(Assignee)

Updated

6 months ago
Attachment #8879017 - Flags: feedback?(benjamin)
(Assignee)

Comment 4

6 months ago
Requesting data vouches from clarkbw
Flags: needinfo?(clarkbw)

Updated

6 months ago
Attachment #8879017 - Flags: review?(mratcliffe)

Comment 5

6 months ago
mozreview-review
Comment on attachment 8879017 [details]
Bug 1373483 - Part 1: Add telemetry for the grid inspector.

https://reviewboard.mozilla.org/r/150298/#review154964

Looks good to me, but I'd like Mike to chime in on that too.
Attachment #8879017 - Flags: review?(pbrosset) → review+
Comment on attachment 8879017 [details]
Bug 1373483 - Part 1: Add telemetry for the grid inspector.

https://reviewboard.mozilla.org/r/150298/#review155000

Looks good to me.
Attachment #8879017 - Flags: review?(mratcliffe) → review+
(In reply to Gabriel [:gl] (ΦωΦ) from comment #4)
> Requesting data vouches from clarkbw

Yes, I'll be tracking this data on redash to better understand ways to optimize the interface for grid users as well as developing communication strategies to better inform people how to use the tools.
Flags: needinfo?(clarkbw)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8879017 [details]
Bug 1373483 - Part 1: Add telemetry for the grid inspector.

https://reviewboard.mozilla.org/r/150298/#review155286

::: toolkit/components/telemetry/Histograms.json:10085
(Diff revision 1)
>      "description": "Reports the command name used in GCLI e.g. 'screenshot'"
>    },
> +  "DEVTOOLS_NUMBER_OF_CSS_GRIDS_IN_A_PAGE": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "never",

The usual rule is that a permanent histogram should have an automated test, because it's not uncommon for code refactorings to silently break telemetry metrics.

I don't see any automated tests in this changeset. Is there a reason why this value is not testable?

::: toolkit/components/telemetry/Histograms.json:10086
(Diff revision 1)
>    },
> +  "DEVTOOLS_NUMBER_OF_CSS_GRIDS_IN_A_PAGE": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",

I don't understand using an enumerated histogram for this. If this is a counter value it should be a linear or exponential histogram rather than enumerated.

An enumerated histogram should warn if you record a value outside of boundaries, while a linear/exponential histogram won't.

::: toolkit/components/telemetry/Histograms.json:10089
(Diff revision 1)
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 25,
> +    "bug_numbers": [1373483],
> +    "description": "How many CSS Grid elements DevTools sees?",

Please improve this description. A metric description should include both *what* is measured and *when* it is measured. I don't know the details of this one, but perhaps: "On page load when the devtools are open, record how many grid elements are present in the page."
Attachment #8879017 - Flags: review-

Updated

6 months ago
Attachment #8879017 - Flags: feedback?(benjamin)
(Assignee)

Comment 9

6 months ago
Created attachment 8879450 [details] [diff] [review]
1373483.patch
Attachment #8879017 - Attachment is obsolete: true
(Assignee)

Comment 10

6 months ago
Created attachment 8879663 [details] [diff] [review]
1373483.patch [1.0]
Attachment #8879450 - Attachment is obsolete: true
Attachment #8879663 - Flags: review+
Attachment #8879663 - Flags: feedback?(benjamin)
(Assignee)

Comment 11

6 months ago
Created attachment 8879664 [details] [diff] [review]
1373483.patch [1.0]
Attachment #8879450 - Attachment is obsolete: true
Attachment #8879664 - Flags: review+
Attachment #8879664 - Flags: feedback?(benjamin)
(Assignee)

Comment 12

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75349e0aded104ab299bb3522489730e4946f7b9
(Assignee)

Updated

6 months ago
Attachment #8879664 - Attachment is obsolete: true
Attachment #8879664 - Flags: feedback?(benjamin)

Comment 13

6 months ago
Comment on attachment 8879663 [details] [diff] [review]
1373483.patch [1.0]

Notes copied from IRC:

* don't add this to histogram-whitelists: that's intended for old histograms which don't follow the guidelines, and new ones should hardly ever be added to that list
* I'm pretty certain you want n_buckets to be 32: that gives you one bucket for <0, 30 buckets for 0-29, and one bucket for 30+

data-r=me with those nits fixed
Attachment #8879663 - Flags: feedback?(benjamin) → feedback+
(Assignee)

Comment 14

6 months ago
Created attachment 8879728 [details] [diff] [review]
1373483.patch [2.0]
Attachment #8879663 - Attachment is obsolete: true
Attachment #8879728 - Flags: review+

Comment 15

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/830b3b513bf1
Add telemetry for the grid inspector. r=pbro. data-r=bsmedberg
Backed out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108671626&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/32adb4bf3e162ee33cc685d9b2028286768c3df0
Flags: needinfo?(gl)

Comment 17

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f86f971014
Add telemetry for the grid inspector. r=pbro. data-r=bsmedberg
(Assignee)

Updated

6 months ago
Flags: needinfo?(gl)

Comment 18

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86f86f971014
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.