1365529 - Allow restricting scalar keys to a known set in Scalars.yaml

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

As done in bug 1343855 for keyed histograms, we should permit restricting the set of allowed keys in keyed scalars.

We should:
- for Scalars.yaml
  - add an optional property 'keys'
  - only available for keyed scalars
  - value is a list of strings, specifying the valid keys for the scalar
- in the Telemetry implementation
  - have a list of valid keys per scalar
  - ignore adding to a scalar with an unspecified key
    - print a warning
    - accumulate in a keyed scalar with the key being the name of the offending scalar

Comment 1

[Beatriz Rizental [:brizental]]

Is there a list of ALWAYS_ALLOWED_KEYS for scalars, like there is one for histograms?

Also, are there MAX_KEY_LENGTH and MAX_KEY_COUNT?

Comment 2

[Chris H-C :chutten]

ALWAYS_ALLOWED_KEYS for histograms is a list of keys for the histogram definition, not keys that are provided as part of a keyed histogram.

As for MAX_KEY_{LENGTH|COUNT} we should define those limits for Scalars. I'm of the opinion we should use the same limits we have for histograms' keys property, but if you have a different set of values you'd prefer let's by all means discuss them.

Comment 3

[Beatriz Rizental [:brizental]]

Nope, I was thinking the same limits for MAX_KEY_{LENGTH|COUNT} too. I will continue with that.

Thanks for the clarification on ALWAYS_ALLOWED_KEYS.

Comment 4

[Beatriz Rizental [:brizental]]

Attachment: Bug 1365529 Allow restricting scalar keys to a known set in Scalars.yaml r=chutten

Comment 5

[Beatriz Rizental [:brizental]]

:chutten, I sent a partial patch for this. It it not working and I am having a hard time figuring out why 🤔

When I try running the test it crashes Firefox. I was able to figure out that it is JS_GetProperty that is crashing inside the test I created, but that didn't really help me understand. I am sending this partial fix so you can look at the code :)

You'll see that I changed MAX_KEY_{LENGTH|COUNT} since we discussed it, because, I saw that there were values for these constants in the cpp implementation of Scalars already (https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/TelemetryScalar.cpp#103-104), let me know if that makes sense.

Finally, I still have to implement this part, but I didn't quite understand it, would you mind explaining it a little bit more?

accumulate in a keyed scalar with the key being the name of the offending scalar

As of now I just error when adding a scalar with an unspecified key.

Comment 6

[Chris H-C :chutten]

Finally, I still have to implement this part, but I didn't quite understand it, would you mind explaining it a little bit more?

accumulate in a keyed scalar with the key being the name of the offending scalar

As of now I just error when adding a scalar with an unspecified key.

We can collect meta-Telemetry, say telemetry.invalid_scalar_key which could be a keyed uint Scalar where the keys would be the names of keyed scalars that tried to accumulate invalid keys.

So if I had a keyed uint scalar category.keyed_scalar that had a keys: [KeyOne, KeyTwo] and I tried to use Telemetry.keyedScalarAdd("category.keyed_scalar", "KeyThree", 2) I would expect that error to be returned and for TelemetryScalar itself to add(1) to telemetry.invalid_scalar_key with the key category.keyed_scalar.

But that's bonus points at the moment. Let's look at the crash.

...The code looks fine... except. Did you define telemetry.test.keyed_with_keys in Scalars.yaml? If you didn't, maybe this is gtest's rather uninformative way of mentioning that.

Comment 7

[Beatriz Rizental [:brizental]]

Did you define telemetry.test.keyed_with_keys in Scalars.yaml?

Hm, actually I did. I'll update the patch with it. I had some mercurial trouble while opening this patch and didn't add the Scalars.yaml file... oops.

Comment 8

[Beatriz Rizental [:brizental]]

Attachment: Bug 1365529 Allow restricting scalar keys to a known set in Scalars.yaml r=chutten

Comment 9

[Chris H-C :chutten]

Okidoki. I'm trying this on my Windows laptop, so please bear with any platform-specific nonsense I might accidentally dredge up.

I am getting a crash as a result of a MOZ_ASSERT for the test TelemetryTestFixture.TestKeyedKeysScalar. Specifically this MOZ_ASSERT. Though the assert message talks about the key being too long, the code that triggers it is only looking at the ScalarResult from GetScalarForKey and hasn't been updated to handle the new ScalarResult::KeyNotAllowed you added.

So my theory is that we're getting a KeyNotAllowed for the key of testing in your test for that Scalar, and so the assert is tripping, and thus we're getting a crash. Which, if true, would be half of a good thing: it is correctly exercising code you wrote, though it does seem to be mistaking that testing isn't allowed when it should be.

You should be able to confirm this with logging (Use of printf_stderr should get to your console even in gtests) or through use of a debugger (./mach gtest --debugger=gdb TelemetryTestFixture.TestKeyedKeysScalar should attach gdb (if you have it installed and on your PATH). You then need to run run in order for it to go, if I recall correctly). I'd do it myself but Windows and gdb don't mix very well, and the Visual Studio debugger refuses to break on the assert failure no matter what I do.

One additional tip might be to build in debug mode so that you have a better chance at getting good stack traces and messages. Add the following to your .mozconfig:

ac_add_options --enable-debug
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-debug-@CONFIG-GUESS@

(The second line makes ./mach put its build artefacts in a new location. Debug artefacts aren't interchangeable with release ones, so we might as well keep them separated. This way if you switch back and forth you shouldn't need to fully rebuild as often. If you're lucky.)

Comment 10

[Beatriz Rizental [:brizental]]

Comment on attachment 9106196 [details]
Bug 1365529 Allow restricting scalar keys to a known set in Scalars.yaml r=chutten

I was able to fix the problem and the tests pass (Turns out I only had a missing ! in one of my conditions). The only thing that is still missing is:

accumulate in a keyed scalar with the key being the name of the offending scalar

Right now I just error when the user tries to use a key that is not allowed.

I wonder if this still a necessary thing, since this is an old bug. Let me know :)

Comment 11

[Chris H-C :chutten]

The problem we have with erroring out is that we can't really do that in a release build. (MOZ_ASSERT only crashes on debug builds) So instead we fail silently and hope the data wasn't important. With keyed things where we can't rely on the type system enforcing that only the correct ones are being used (because of historical reasons, and JavaScript), we have to assume that just about any string possible might come through there at any time.

Thus, to have a chance at noticing that this is happening, we should probably record a scalar (like the key too long one) any time someone uses an invalid key on us.

If you want to file a separate bug for that and put it in the backlog, that's fine.

Comment 12

[Chris H-C :chutten]

Oh, and another problem that failing causes is that it's harder to test. MOZ_ASSERT crashing on debug builds means we can't write a test that exercises it in the negative way because debug builds will explode.

Comment 13

[Beatriz Rizental [:brizental]]

Yes, it makes sense. Yesterday I had a little spare time, was looking at this again and realized I was making the whole "accumulate in a keyed scalar with the key being the name of the offending scalar" much more complex than I should, in my mind... I have updated the revision with that and I think it is finished :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a3ca2b832f6
Allow restricting scalar keys to a known set in Scalars.yaml r=chutten

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6a3ca2b832f6