Add telemetry for the grid inspector

RESOLVED FIXED in Firefox 56

Status

P3
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Priority: -- → P3
(Assignee)

Comment 1

2 years 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

2 years 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

2 years ago
Attachment #8879017 - Flags: feedback?(benjamin)
(Assignee)

Comment 4

2 years ago
Requesting data vouches from clarkbw
Flags: needinfo?(clarkbw)
Attachment #8879017 - Flags: review?(mratcliffe)

Comment 5

2 years 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

2 years 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

2 years ago
Attachment #8879017 - Flags: feedback?(benjamin)
(Assignee)

Comment 9

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

Comment 10

2 years 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

2 years 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)

Updated

2 years ago
Attachment #8879664 - Attachment is obsolete: true
Attachment #8879664 - Flags: feedback?(benjamin)
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

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

Comment 15

2 years 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

Comment 17

2 years 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

2 years ago
Flags: needinfo?(gl)

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86f86f971014
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.