1842607 - FOG: Log attempts to access category/metric NamedGetter using underscores

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Perry McManis [:perry.mcmanis]]

One of the complexities of working in Desktop with Glean is that it's multi-language. Because FOG aims to conform to individual language standards, accessing categories and metrics has to contend with differing conjugations, e.g. an_example_metric for Rust versus anExampleMetric for Javascript.

Accepting all conjugations in all languages might lead to code inconsistencies, which is not a desired outcome. What we can do to ease this process is WARN or INFO when someone has attempted to use the non-JS conjugation in a Javascript test, such as XPCshell

Comment 1

[wilsu]

I'd like to work on this, sounds interesting. I've been reading the Glean documentation (not all of it yet) and I believe I did find the NamedGetter that is mentioned in the title, but I am new to this.
Could you perhaps let me know a bit more specifically what I'd have to familiarize myself with? Despite my efforts I feel like I'm tapping in the dark.

Comment 2

[Chris H-C :chutten]

(In reply to wilsu from comment #1)

I'd like to work on this, sounds interesting. I've been reading the Glean documentation (not all of it yet) and I believe I did find the NamedGetter that is mentioned in the title, but I am new to this.
Could you perhaps let me know a bit more specifically what I'd have to familiarize myself with? Despite my efforts I feel like I'm tapping in the dark.

That is indeed one of the NamedGetters for this bug. The other is this one in Category. Both of them expect their nsAString& aName arguments to be in camelCase (which is to say, containing only alphanumeric ([a-zA-Z0-9]) ASCII characters, with the additional restriction that the first character be a lowercase alphabetic character ([a-z])) because that's how we conjugate metric names for JavaScript uses (which these NamedGetters are implementing). The unconjugated metric names often have . or _ characters in them, so it's possible that someone unfamiliar with this system might end up passing an aName with a _ or . in it.

You can see this in the documentation for e.g. [the Glean counter metric type]https://mozilla.github.io/glean/book/reference/metrics/counter.html#add) where the Firefox Desktop support uses cameCase identifiers for JS, and snake_case for C++.

This bug would be about introducing a check in those NamedGetters to try and catch these sorts of misunderstandings. The check:

• Could just see if there's a _ or . present in the string
• Could instead assert that the string is in camelCase
• Must only run on developer builds, to avoid introducing performance impacts (could narrow this further to debug builds only to make our lives easier)
• Must tell the developer what they got wrong

One possible solution would be to use the MOZ_ASSERT macro (restricted to debug-only builds, so satisfies the third restriction) to check the condition and supply a helpful error message.

Does this help?

Comment 3

[wilsu]

Yes, this helps immensely, actually. Thank you! I believe I can start working based on this.

Comment 4

[wilsu]

Attachment: Bug 1842607 - Added MOZ_ASSERT to NamedGetters to check for correct formatting style. r=chutten

Comment 5

[wilsu]

Finally had some time to look at this in more detail. Still took a bit of reading to make sure I understood the context right and I was unfamiliar with nsAString. Think I got it now, though.
I've submitted my changes to treeherder with auto configured tests in addition to the phabricator submit linked above.
One question remains: how do I actually test this assert locally? Is there a unit test I can hijack for this or is there a better way?

Comment 6

[Chris H-C :chutten]

Good job working out nsAString. We have a whole guide on string classes in use in Firefox. (The perils of a codebase where strings are common, weird, and performance-critical.)

Testing it locally will require building in debug (if you create a file called .mozconfig in the source root of your firefox checkout (next to AUTHORS and CODE_OF_CONDUCT.md and the rest) you can put ac_add_options --enable-debug in it (docs). Then, ./mach build && ./mach run, load about:glean (since it is a privileged page), pop open the devtools console (Ctrl+Shift+K), then try typing e.g. Glean.some_category.some_metric. It might be that merely typing it will be enough to run the NamedGetters, or you might have to hit Enter first.

If your assert works, you should see the message you wrote in the console you built and ran in... and also probably quite a lot of other stuff, as Firefox will crash. If you want Firefox to keep running instead, try NS_WARN_IF(<condition>) instead of MOZ_ASSERT.

Alas, there's no way to unit test "the application crashes"... at least none I'm familiar with. That's by definition an integration test sort of thing.

You could write a unit test for the string matching logic, though. See TestFog.cpp for an existing C++-language test file you can adapt.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9c5fe0f84452
https://hg.mozilla.org/integration/autoland/rev/f4b596339a8e
Added MOZ_ASSERT to NamedGetters to check for correct formatting style. r=chutten

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f4b596339a8e