Closed Bug 1373483 Opened 2 years ago Closed 2 years ago

Add telemetry for the grid inspector

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Priority: -- → P3
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
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.
Attachment #8879017 - Flags: feedback?(benjamin)
Requesting data vouches from clarkbw
Flags: needinfo?(clarkbw)
Attachment #8879017 - Flags: review?(mratcliffe)
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 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-
Attachment #8879017 - Flags: feedback?(benjamin)
Attached patch 1373483.patch (obsolete) — Splinter Review
Attachment #8879017 - Attachment is obsolete: true
Attached patch 1373483.patch [1.0] (obsolete) — Splinter Review
Attachment #8879450 - Attachment is obsolete: true
Attachment #8879663 - Flags: review+
Attachment #8879663 - Flags: feedback?(benjamin)
Attached patch 1373483.patch [1.0] (obsolete) — Splinter Review
Attachment #8879450 - Attachment is obsolete: true
Attachment #8879664 - Flags: review+
Attachment #8879664 - Flags: feedback?(benjamin)
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+
Attachment #8879663 - Attachment is obsolete: true
Attachment #8879728 - Flags: review+
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
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
Flags: needinfo?(gl)
https://hg.mozilla.org/mozilla-central/rev/86f86f971014
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.