1423715 - Add CrashType keys to Crash Manager Histogram Definitions

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Comment 1

[Georg Fritzsche [:gfritzsche]]

David, do you know who could fill these out?

Comment 2

[David Durst [:ddurst]]

I don't, but I assume that gsvelto can speak to this?

Comment 3

[Gabriele Svelto [:gsvelto]]

I'd be happy to help if you point me to what needs fixing. I'm not super-familiar with telemetry so I'm not sure where to find these definitions. Leaving the NI? for now.

Comment 4

[Frank Bertsch [:frank]]

(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> I'd be happy to help if you point me to what needs fixing. I'm not
> super-familiar with telemetry so I'm not sure where to find these
> definitions. Leaving the NI? for now.

What needs updating is just the probe definitions [0]. That links to `PROCESS_CRASH_SUBMIT_ATTEMPT`, but we need any probes that are keyed on CrashManager's Crash.Type. They need to add a `keys` element, with a list of allowed keys [1]. Make sense?

[0] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#11169
[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#7444

Comment 5

[Gabriele Svelto [:gsvelto]]

Yup, let me cook a patch for this!

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1423715 - Add crash type keys to the crash manager histogram definitions;

This patch the list of process and crash types that can be recorded in a keyed
histogram by the crash manager; the relevant pattern was updated to match
these kesy. A test was also added to ensure that no new  crash types can be
added without also updating Histograms.json. This also fixes a related
existing test.

Review commit: https://reviewboard.mozilla.org/r/228276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228276/

Comment 7

[Gabriele Svelto [:gsvelto]]

Patch attached, I've added a test to ensure we can't accidentally add a new type to the crash manager w/o updating the list in Histograms.json and since I was at it I fixed the related test, it was a simple type that caused a check to always evaluate as true. I've also had to update the regular expression used to match the keys because it didn't allow for dashes in the names (and the crash manager uses them).

Comment 8

[Frank Bertsch [:frank]]

Comment on attachment 8959485 [details]
Bug 1423715 - Add crash type keys to the crash manager histogram definitions;

https://reviewboard.mozilla.org/r/228276/#review234156

::: toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:629
(Diff revision 1)
> +  let processTypes = [];
> +  let crashTypes = [];
> +
> +  // Gather all process and crash types
> +  for (let field in m) {
> +    if (field.startsWith("PROCESS_TYPE_")) {

Are this and `"CRASH_TYPE_"` available as constants?

::: toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:647
(Diff revision 1)
> +    }
> +  }
> +
> +  // Check that we have the expected keys.
> +  let snap = h.snapshot();
> +  Assert.equal(Object.keys(snap).length, keysCount,

Should this actually test that the values in the arrays are the same, not just the lengths?

::: toolkit/components/telemetry/Histograms.json:11208
(Diff revision 1)
>      "record_in_processes": ["main", "content"],
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "keyed": true,
>      "releaseChannelCollection": "opt-out",
>      "description": "The submission status when main/plugin/content crashes are submitted. 1 is success, 0 is failure. Keyed on the CrashManager Crash.type."

We want to also add the keys to every other probe that is keyed on `CrashManager Crash.Type`, including this one. Are there any others?

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:1022
(Diff revision 1)
>    h.add("thirdKey", false);
>    h.add("not-allowed", true);
>  
>    // Check that we have the expected keys.
>    let snap = h.snapshot();
> -  Assert.ok(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");
> +  Assert.equal(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");

This should be `3` now, right? Since `not-allowed` is now an allowed key? I recommend adding a disallowed char to `"not-allowed"`.

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8959485 [details]
Bug 1423715 - Add crash type keys to the crash manager histogram definitions;

https://reviewboard.mozilla.org/r/228276/#review234170

::: toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:629
(Diff revision 1)
> +  let processTypes = [];
> +  let crashTypes = [];
> +
> +  // Gather all process and crash types
> +  for (let field in m) {
> +    if (field.startsWith("PROCESS_TYPE_")) {

No, they're declared as fields in the CrashManager object: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/toolkit/components/crashes/CrashManager.jsm#181

I want to make sure that the test will break if somebody adds a new type w/o also adding it to Histograms.json.

::: toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:647
(Diff revision 1)
> +    }
> +  }
> +
> +  // Check that we have the expected keys.
> +  let snap = h.snapshot();
> +  Assert.equal(Object.keys(snap).length, keysCount,

Good point, I'll add those checks.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:1022
(Diff revision 1)
>    h.add("thirdKey", false);
>    h.add("not-allowed", true);
>  
>    // Check that we have the expected keys.
>    let snap = h.snapshot();
> -  Assert.ok(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");
> +  Assert.equal(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");

`"not-allowed"` is rejected because it's not part of the listed keys not because it has a dash in the name, the call fails here:

https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/toolkit/components/telemetry/TelemetryHistogram.cpp#479

The change to allow the dash in the pattern is required only at build time when parse_histograms.py is run. That being said your comment made me have a second look at that file and I found this comment:

https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/toolkit/components/telemetry/parse_histograms.py#290

So in hindsight adding a dash there is not a good idea.... Shall I change the way the crash manager assembles those keys to using an underscore instead of a dash?

Comment 10

[Gabriele Svelto [:gsvelto]]

Back from parental leave and ready to give this another go. Quick question before I proceed though, what should I do about the regexp in CPP_IDENTIFIER_PATTERN? That's supposed to keep the data in a format that makes it easy to turn it into C++ identifiers, however the existing code will not conform to it since it uses dashes in between the process type and crash type. Shall I change [1] to use an underscore instead? Or should I relax the regexp like I did in the patch?

[1] https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/components/crashes/CrashManager.jsm#1349

Comment 11

[Frank Bertsch [:frank]]

(In reply to Gabriele Svelto [:gsvelto] from comment #10)
> Back from parental leave and ready to give this another go. Quick question
> before I proceed though, what should I do about the regexp in
> CPP_IDENTIFIER_PATTERN? That's supposed to keep the data in a format that
> makes it easy to turn it into C++ identifiers, however the existing code
> will not conform to it since it uses dashes in between the process type and
> crash type. Shall I change [1] to use an underscore instead? Or should I
> relax the regexp like I did in the patch?

Okay great! And congratulations :)

We don't actually generate CPP Identifiers on histogram keys, so we can remove that check. We should replace it with a similar check (for e.g. length and less constraining special chars), but can definitely include dashes and underscores both.

Comment 12

[Gabriele Svelto [:gsvelto]]

Thanks! Should I remove that check in this patch or file another bug for that?

Comment 13

[Frank Bertsch [:frank]]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> Thanks! Should I remove that check in this patch or file another bug for
> that?

I think it makes sense to file another bug with that patch, have one of the client engineers review it, and once that is merged we can then add these keys to the definition. Sound good?

Comment 14

[Gabriele Svelto [:gsvelto]]

Alright, will do.

Comment 15

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8959485 [details]
Bug 1423715 - Add crash type keys to the crash manager histogram definitions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228276/diff/1-2/

Comment 16

[Gabriele Svelto [:gsvelto]]

Updated patch with comments addressed. Sorry for the garbage in the interdiff but I had to rebase to pick up the changes in bug 1451288.

Comment 17

[Frank Bertsch [:frank]]

Comment on attachment 8959485 [details]
Bug 1423715 - Add crash type keys to the crash manager histogram definitions;

https://reviewboard.mozilla.org/r/228276/#review234180

::: toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:629
(Diff revision 1)
> +  let processTypes = [];
> +  let crashTypes = [];
> +
> +  // Gather all process and crash types
> +  for (let field in m) {
> +    if (field.startsWith("PROCESS_TYPE_")) {

Got it, that makes a sense to me.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:1022
(Diff revision 1)
>    h.add("thirdKey", false);
>    h.add("not-allowed", true);
>  
>    // Check that we have the expected keys.
>    let snap = h.snapshot();
> -  Assert.ok(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");
> +  Assert.equal(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");

Okay, let's do this: Remove the dash from `CPP_INDENTIFER_PATTERN`, but also remove the check using that pattern on histogram keys (since CPP identifiers are not generated from histogram keys).

Comment 18

[Frank Bertsch [:frank]]

Whoops, that was an old comment -- you can ignore it.

Comment 19

[Gabriele Svelto [:gsvelto]]

Thanks! Landing this.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c783649db622
Add crash type keys to the crash manager histogram definitions; r=frank

Comment 21

[Cristian Brindusan [:cbrindusan]]

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