We only have two counters enabled, just for testing, and they just count from
the SVG mapped attribute code. That's not great, and they mostly complicate the
next few patches.

And add some sanity-check assertions that the reporting code relies on.

This works much like the deprecated operation list.

By reporting the properties in MappedAttrParser that we used to (the test relies
on counting those), and by adjusting the histogram name to the new thing.

C'est fini. Now to do the same with the unimplemented ones remains, but it
should be straight-forward.

This adds histograms for the stuff added in bug 1575062, which is needed for the
webcompat metrics project.

This also tweaks the naming to be consistent across all CSS properties. Normally
the method is just equivalent to the camel_case thing except for:

• The css float property, whose method is CssFloat.
• Internal properties (but we don't count those anyway).

This is so that we get consistent usage data if we ever implement some of those
properties.

I had to increase the PHF table size to make build times sane again (for some
reason adding the "d" property in there blows up the phf map generation time).

But according to Nika the binary size impact of that change is minimal /
irrelevant:

https://mozilla.logbot.info/content/20190904#c16590376

So I think it should be fine.
chance

Self-ni? to write a data-review request and such tomorrow.

(In reply to Chris H-C :chutten from comment #9)

Comment on attachment 9090544 [details]
Data collection request form.

Do you happen to know whether the use counters will be picked up by the
Probe Dictionary? (Might need to ask Dexter)

No clue really.

[...]
Also, did you update [the use counter
docs](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
collection/use-counters.html)?

I did remove the stale docs. I'll add some before landing.

Also, just to double-check, are you fine landing this (with docs updated and review comments addressed) even without the answer to the question in comment 9?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to Chris H-C :chutten from comment #9)

Comment on attachment 9090544 [details]
Data collection request form.

Do you happen to know whether the use counters will be picked up by the
Probe Dictionary? (Might need to ask Dexter)

No clue really.

Looks like they should. Jan-Erik recently added support for use-counters in Release in the probe-scraper.

And now, we are seeing them!

(In reply to Alessio Placitelli [:Dexter] from comment #12)

Looks like they should. Jan-Erik recently added support for use-counters in Release in the probe-scraper.

The probe-scraper uses (nearly) the same parser as m-c and grabs the same files from m-c.
If the way these use counters are generated is changed, we will also need to update the probe-scraper's parser (I haven't checked the patch here yet, but I'll give it a look)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Also, just to double-check, are you fine landing this (with docs updated and review comments addressed) even without the answer to the question in comment 9?

With docs updated, that gets us most of the way to the data review being able to be reopened. I'd like to hear back from Jan-Erik about what this will take to get Probe Scraper and the Probe Dictionary in line before we reopen the Data Review Request though.

It's ... tricky.
The code makes the histogram parser execute 2 python files.
One of those python files (layout/style/ServoCSSPropList.py) is actually generated at build time from another template (I guess that's ServoCSSPropList.mako.py. I'm not sure where the data is coming from that's fed into that template.)
The other one is in the tree (and it's only "executed" to extract some static data easily).

That whole build-time generation makes this really tricky (remember: probe-scraper fetches every release version and nightly version to extract this data). It would therefore need to also generate the ServoCSSPropList somehow, either by the same template or by reading the data from some file in tree and replicate the same behavior.

Ok, I'll update the patches + docs when I'm back from PTO.

Jan-Erik, where does probe-scraper live?

We can try to generate the histograms from the devtools database plus the other unimplemented properties list, or something, if it'd be easier (probably would complicate the code because we wouldn't have the guarantee that they correspond to an nsCSSPropertyId in order).

Though I suspect it's not hard to get the right thing in probe-scraper. Or if that tool just needs the histogram names and doesn't rely in order, then grabbing either properties db plus the other non-generated python file should be good enough, probably. It may not show histograms for disabled properties or what not, but that may be ok. property_database.js shouldn't have that problem with disabled properties (all properties should appear there even if disabled, if you make the right check return true...).

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

Ok, I'll update the patches + docs when I'm back from PTO.

It's only PTO if you do the O!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

Jan-Erik, where does probe-scraper live?

This is probe-scraper

Just some minor cleanup while I think on how to best integrate this with
probe-scraper.

This was a review comment which was easier to address as a separate patch.

Not sure how much detail does it need (it just works) but suggestions welcome.

So, I couldn't catch Jan-Erik in the office today, so a few questions, as the approach I use to do the counters depends on how the tool is used, which I don't know.

Basically, there are four ways to approach this as I see it.

1 - Use another checked-in file in the repository

This is the simplest (kinda). We have a few files with lists of CSS properties, like the devtools property database, or the property_database.js that we use for CSS tests.

Pros

• Keeps the setup we have, where probe-scraper pulls files from mozilla-central.

Cons

• These are JS files, so it means we need to process them somehow. probe-scraper is python so that's a bit annoying.
• These lists can miss some properties (we intentionally ignore some properties in the devtools database, like -moz-osx-font-smoothing, which we may want to count). That's fixable, though not great. For property_database.js, I think we should not missing any content-accessible property, and if we did that'd be something to fix, as the fuzzers use that file.
• We don't share the parsing code with Gecko, so that can get out of sync. I expect it to be a tiny loop of the sort of the one Gecko uses, so it may or may not be a huge deal:

def from_ServoCSSPropList(filename, strict_type_checks):
    histograms = collections.OrderedDict()
    properties = runpy.run_path(filename)["data"]
    for prop in properties:
        add_css_property_counters(histograms, prop.name)
    return histograms

Constraints

We can only do this if probe-scraper does not depend on the order of the histograms (which I think it's the case).

2 - Like above, but generating yet another (probably python) file and linting for its existence / up-to-date-ness.

This is, effectively, checking-in ServoCSSProperties.py (or a simplified / trimmed-down version of it), and linting / testing it on automation to ensure that it doesn't go out of sync.

Pros

• Same as above, minus all the cons.

Cons

• Slightly gross.
• Another thing to get people backed out when they add a new CSS property (though unifying it with the generation of the devtools DB should be no worse than today, so not a very strong con).

Constraints

None, I think.

3 - Upload ServoCSSProperties.py as an artifact to taskcluster, make probe-scrapper use it

This one would make the properties file an artifact in taskcluster, like the logs. This is actually pretty easy to do.

Pros

• Same as above, minus all the cons.
• Puts us into a better spot to rely on build-generated stuff (see below).

Cons

• probe-scraper needs to look at tascluster URLs on top of hg.mozilla.org (this is easy, just pointing it out).
• needs a bit of taskcluster wrangling, but I'm familiar with that.

Constraints

This limits the effective revisions that probe-scraper can scrape to the ones we have builds for. I don't know whether that's an issue or not. Looking at probe_scraper/scrapers/buildhub.py it seems we use buildhub, and as such we may already be relying on that. So probably not?

4 - As above, but the artifact is all the histogram data that we generate at build-time and probe-scraper reconstructs

This basically makes probe-scraper simpler by effectively just pulling the data it wants from taskcluster rather than duplicating all the parsing and merging and what not for every revision.

Pros

• Same as above.
• Makes probe-scraper much less likely to get out of sync with Gecko.

Cons

• Basically same as above.
• May make probe-scraper harder to extend, maybe?
• May need to keep current probe-scraper code to keep data for old Firefox versions (though you can also pre-compile that data and put it somewhere on GitHub or what not).

Thoughts? Other ideas I may have missed? I'm leaning towards (2), (3), or maybe (3) + (4) as followup work if you deem it worth it, depending on whether the potential issues above are real or just potential, and whether the extra work of doing (3) / (4) is worth it, in your opinion.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)

So, I couldn't catch Jan-Erik in the office today, so a few questions, as the approach I use to do the counters depends on how the tool is used, which I don't know.

Err, the approach I use to do the use counters probe-scraper integration, I mean.

Just want to provide some context about using artifacts (options 3/4 above), since we did that for a while in Glean, but then ultimately backed it out.

For Glean, we used it to generate the dependencies of an application in a form that was then easily parseable by probe-scraper. The main pro is that you have the full development environment for the application available in Taskcluster, whereas probe-scraper does not (it's just a vanilla Python install, essentially). We had this working for a while, but backed it out because we needed to take this approach for every app that uses Glean, and each of those apps had a slightly different TC configuration, and the folks who maintain those were (understandably) wary of taking on the extra complexity. That may not be an issue here, because you're talking about a single instance of the artifact-generating thing just for m-c, so YMMV.

(This is explicitly not a thumbs up or thumbs down from me, just providing some experiential context).

I'm torn.

Option 4 makes use counters exactly like histograms... which they both are and are not. Histograms are the current transfer format for use counters. And it shifts how we think about Histograms, which adds risk.

Option 3 has the added flexibility to allow us to iterate on the transfer format used for use counters, which I like. Use counters could easily (and more efficiently) be Scalars instead of Histograms. But optimizing for a future that might not even come to pass is folly.

Option 2 is distasteful to me because checking in compiled files is icky. I'd much prefer Option 3 over this.

Option 1 sounds the most similar to how use counters currently behave. Which doesn't make it nice, but does make it known. We could add another section to probe-scraper's usecounters.py and be done with it.

I'm inclined to follow Option 1. From what I've read of the code so far, we will already have the necessary property lists. It also appears to be the least work. I don't think our plans of shifting collection to Glean on Desktop are close enough to the present to have meaningful impact on our decisions here, but even if they were Option 1 having the least code means the less code we need to tear out later : S

Reading this over, I still feel like I'm missing some context. However, probe-scraper doesn't care about ordering, so follow-up from :chutten's suggestion, it seems like Option 1 makes the most sense. It also aligns with how we scrape current measurements from moz-central.

Does this all need to land and get beta uplift for the webcompat experiments for beta/release 70?
We're heading into beta 8 of 14, so are now at the halfway point in this beta cycle.

This will add a lot of histograms. We plan to spend some time on Nightly diminishing the impact from the most used properties so that telemetry doesn't blow up.

But if we were to uplift it I wouldn't be too concerned, this is mostly build system wrangling.

(And test it.)

So I sent a patch to add a parser for the property names via the devtools database.

I went ahead and tried to poke at integrating it via probe scraper, but I couldn't get it to work. I tried to set it up with both python2 and python3.

Here's the current state of it: https://github.com/mozilla/probe-scraper/compare/master...emilio:master

Errors are partly python 2/3 compat issues, some of which I fixed, others seem like trying to load an allowlist that doesn't exist in probe-scraper, as expected, but I'm not sure about the right fix...

Frank, any idea?

I guess it now bites us that probe-scraper uses a slightly different/out-of-date-but-modified version of the parser, making a port back to it all more complicated.
I do think it's the right way to go though, with the special case that we probably need to restrict that by version (we don't need that version on old versions)

(Thanks for the current work there, Emilio)

We're muddying the waters a bit here. I think we should move the probe-scraper augmentation to a follow-up bug (it doesn't need to ride the trains, and isn't needed for the Data Collection Review now that the in-tree docs have been updated) and land the use counters, which seem to be r+ across the board. Lemme take another look at that Data Review.

https://github.com/mozilla/probe-scraper/pull/118 contains an attempt at updating probe-scraper.

https://hg.mozilla.org/mozilla-central/rev/6dd7a0d914d9
https://hg.mozilla.org/mozilla-central/rev/11e1e8f0a1b7
https://hg.mozilla.org/mozilla-central/rev/dc96d13728e0
https://hg.mozilla.org/mozilla-central/rev/b0db409ada96
https://hg.mozilla.org/mozilla-central/rev/f643262a8c25
https://hg.mozilla.org/mozilla-central/rev/273535aab82d
https://hg.mozilla.org/mozilla-central/rev/db306f1467f7
https://hg.mozilla.org/mozilla-central/rev/c6d64ac858ba
https://hg.mozilla.org/mozilla-central/rev/d16463e5698c

Backed out for lints failure at ServoCSSPropList.py

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=267214714&resultStatus=testfailed%2Cbusted%2Cexception&revision=4dc3da8794c8853a3c3d9d9111b2f60c6687a4f3

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267214714&repo=mozilla-inbound&lineNumber=434

Backout: https://hg.mozilla.org/mozilla-central/rev/a3a081ae714f1123bdc23c9d9ef53dfaa783a8de

Clearing out this :ni, Emilio let's continue the convo for updating the probe-scraper in that PR, sound good?

Backed out for lints failure at ServoCSSPropList

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=267264866&resultStatus=testfailed%2Cbusted%2Cexception&revision=3d51b196866645d7e6f84f1deaed734db3020237

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267264866&repo=autoland&lineNumber=289

Backout: https://hg.mozilla.org/integration/autoland/rev/cd0d966885555845fb41ce9e9baf8cc9763a4075

Huh, for some reason my fix (https://hg.mozilla.org/try/rev/508c0aff33ed32d99dc7576c0bcdbc01d32d101d) didn't make it in the second landing either?

https://hg.mozilla.org/mozilla-central/rev/78c90f9e9eee
https://hg.mozilla.org/mozilla-central/rev/88d988a80de1
https://hg.mozilla.org/mozilla-central/rev/42c4943e569b
https://hg.mozilla.org/mozilla-central/rev/89d7e6fb24b0
https://hg.mozilla.org/mozilla-central/rev/33c306c9020a
https://hg.mozilla.org/mozilla-central/rev/7a5687bebc02
https://hg.mozilla.org/mozilla-central/rev/4a1d45160d60
https://hg.mozilla.org/mozilla-central/rev/6ba2ee45f26e
https://hg.mozilla.org/mozilla-central/rev/b87eebb0cf6e
https://hg.mozilla.org/mozilla-central/rev/b1b5393b1099

Backed out for these linting failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linting%2Copt%2Candroid%2Cgradle%2Ctests%2Csource-test-mozlint-android-lints%2Ca%28lints%29&tochange=b87eebb0cf6e90fc7925b7ac21d8e5b6f2128661&fromchange=96fecf5a0833f92b546f54435b4e7e538c132719&selectedJob=267341282

and these when it got merged to central: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=21aff209f5a9b4c750fea02f4837576a91179625&searchStr=android%2C4.0%2Capi16%2B%2Copt%2Candroid%2Cgradle%2Ctests%2Cbuild-android-geckoview-docs%2Fopt%2Ca%28gv-docs%29&selectedJob=267328118

Backout link: https://hg.mozilla.org/integration/autoland/rev/239aef95e5d160f0e8a3f523e7c152cc371c0b99

Sigh... was missing /export in the left hand side of https://hg.mozilla.org/try/rev/508c0aff33ed32d99dc7576c0bcdbc01d32d101d.

https://hg.mozilla.org/mozilla-central/rev/043a08845fad
https://hg.mozilla.org/mozilla-central/rev/47f7ec8f8f2d
https://hg.mozilla.org/mozilla-central/rev/68cf7571a04f
https://hg.mozilla.org/mozilla-central/rev/709aa04c0580
https://hg.mozilla.org/mozilla-central/rev/34e52256e707
https://hg.mozilla.org/mozilla-central/rev/eba3dd02bd92
https://hg.mozilla.org/mozilla-central/rev/5079b803008f
https://hg.mozilla.org/mozilla-central/rev/4acbea2fb0db
https://hg.mozilla.org/mozilla-central/rev/b03efe57fcd6
https://hg.mozilla.org/mozilla-central/rev/b6326259d31e

Tracking for 70 while we figure out whether this is best left in 71 or what might get uplifted to 70.

Comment on attachment 9093202 [details]
Bug 1578661 - Add a crappy histogram parser for the devtools database, for probe-scraper.

Beta/Release Uplift Approval Request

• User impact if declined: N/A, but it blocks the webcompat team from gathering compat data on a wider audience.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1582814
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): While it's a big set of patches, most of them are very simple. Actual Firefox code changes are really minor updates to existing infrastructure.

These patches have also been in Nightly for a bit already.

• String changes made/needed: none

Comment on attachment 9093202 [details]
Bug 1578661 - Add a crappy histogram parser for the devtools database, for probe-scraper.

Crappy or not, here it comes.
For beta 10 as discussed, for the webcompat use counters experiment.

Got conflicts while uplifting to beta:

raul@raul-VirtualBox ~/mozilla-unified beta(0) $ hg graft -er 043a08845fad::b6326259d31e
grafting 567187:043a08845fad "Bug 1578661 - Remove old CSS use counters. r=boris"
merging dom/base/UseCounter.h
merging dom/base/UseCounters.conf
merging dom/base/gen-usecounters.py
merging dom/base/moz.build
merging dom/base/usecounters.py
merging dom/svg/SVGElement.cpp
merging layout/style/nsCSSProps.cpp
merging layout/style/nsCSSProps.h
merging toolkit/components/telemetry/docs/collection/use-counters.rst
warning: conflicts while merging dom/base/UseCounters.conf! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/svg/SVGElement.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Huh, I didn't realize that this needed bug 1578700. Will request there separately.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #54)

Huh, I didn't realize that this needed bug 1578700. Will request there separately.

More like, I didn't realize that bug didn't land on time for 70.

https://hg.mozilla.org/releases/mozilla-beta/rev/6de47c2961fd
https://hg.mozilla.org/releases/mozilla-beta/rev/eca2be54980c
https://hg.mozilla.org/releases/mozilla-beta/rev/c97de3fc66fa
https://hg.mozilla.org/releases/mozilla-beta/rev/47ceb74a4061
https://hg.mozilla.org/releases/mozilla-beta/rev/60e8eaaf1ccc
https://hg.mozilla.org/releases/mozilla-beta/rev/009960ce4b91
https://hg.mozilla.org/releases/mozilla-beta/rev/21cbbd68b02c
https://hg.mozilla.org/releases/mozilla-beta/rev/ebb670c31f5d
https://hg.mozilla.org/releases/mozilla-beta/rev/0c69c60a5aa2
https://hg.mozilla.org/releases/mozilla-beta/rev/1ebe47575917

Did this end up being gated behind a pref for beta? What's the name of the preference?

Oh, I see, the controlling prefs are described in bug 1575062 -- just to double-check, we'll need to flip both layout.css.use-counters.enabled and layout.css.use-counters-unimplemented.enabled to true in beta?

Only the later should be enough, I made those independent in bug 1582814 so that they could be enabled separately. You can check that enabling the pref and going to about:telemetry#histograms-tab, and confirming that you see something for some of the unimplemented properties.