1376359 - stylo: Annotate crash reports

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

Bill and Andrew suggested we do this to make it easier to triage crashes. Seems simple enough.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Annotate crash reports for stylo. v1

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8881321 [details] [diff] [review]
Annotate crash reports for stylo. v1

Review of attachment 8881321 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +13143,5 @@
> +#ifdef MOZ_CRASHREPORTER
> +      static bool sAnnotatedCrashReport = false;
> +      if (!sAnnotatedCrashReport) {
> +        CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("StyloIsEnabled"),
> +                                           NS_LITERAL_CSTRING("Yes"));

I guess this is OK, although it doesn't (and I suppose we can't) distinguish between crashes that occur due to non-stylo-backed documents in the same process that also loaded a stylo-backed document previously.  I wonder how the crash reporter determines which document is active, so that it can report the crashing URL (and whether we could use the same mechanism)?

Comment 3

[Gabriele Svelto [:gsvelto]]

Besides crash reports, for the field to also end up in the crash ping you should add it to the whitelist here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/components/crashes/CrashManager.jsm#227

... and here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/crashreporter/client/ping.cpp#124

And also add an entry to the documentation here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/components/telemetry/docs/data/crash-ping.rst#42

Comment 4

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> I guess this is OK, although it doesn't (and I suppose we can't) distinguish
> between crashes that occur due to non-stylo-backed documents in the same
> process that also loaded a stylo-backed document previously.

I think this use-case is rare enough that it probably doesn't warrant spending time on.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Annotate crash reports for stylo.

Comment 6

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8881428 [details] [diff] [review]
Annotate crash reports for stylo.

LGTM though you shouldn't need to check if you're annotating the crash report more than once. AnnotateCrashReport() replaces the previous entry anyway so it's safe to call it more than once for the same annotation:

https://dxr.mozilla.org/mozilla-central/rev/53477d584130945864c4491632f88da437353356/toolkit/crashreporter/nsExceptionHandler.cpp#2328

Also, since we're adding stuff to the crash ping you probably need a data review too.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Comment on attachment 8881428 [details] [diff] [review]
> Annotate crash reports for stylo.
> 
> LGTM though you shouldn't need to check if you're annotating the crash
> report more than once. AnnotateCrashReport() replaces the previous entry
> anyway so it's safe to call it more than once for the same annotation:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 53477d584130945864c4491632f88da437353356/toolkit/crashreporter/
> nsExceptionHandler.cpp#2328

Yeah, but I wanted to avoid the extra hashtable lookup on every document load.

> 
> Also, since we're adding stuff to the crash ping you probably need a data
> review too.

I'm not sure who to flag for that. Can you flag the appropriate person?

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Yeah, but I wanted to avoid the extra hashtable lookup on every document
> load.

OK, makes sense.

> I'm not sure who to flag for that. Can you flag the appropriate person?

NI'ing Benjamin for the data review.

Comment 9

[Benjamin Smedberg]

data-r=me, although I suggest you should do this differently. Recording this *only* for crashes and not for other telemetry would be unfortunate, and assuming that stylo is pref-controlled, we could instead just add that pref to environment.settings.userPrefs, by adding it to this list:

https://dxr.mozilla.org/mozilla-central/rev/53477d584130945864c4491632f88da437353356/toolkit/components/telemetry/TelemetryEnvironment.jsm#158

Comment 10

[Bobby Holley (:bholley)]

Attachment: Include the stylo pref in telemetry pings. v1

Like this?

Comment 11

[Cameron McCormack (:heycam)]

It seems that prefs recorded here are only visible when logged in to crash-stats.  Will that be convenient enough for us?  Can we search for crashes based on the value in the TelemetryEnvironment JSON blob, or does the CrashReporter::AnnotateCrashReport from the previous patch allow that?

Comment 12

[Astley Chen (inactive)]

I will be also interested to know whether we could leverage the annotation to monitor the trend of crash rate when we start to enable stylo by default and by level of population.

Comment 13

[Benjamin Smedberg]

We can file a socorro issues to make this value a toplevel searchable field.

In terms of crash *rates*, I believe we already aggregate crash rates by experiments/pref rollouts, but not by pref values. My understanding of your rollout plan is that you're using pref experiments for initial testing, so that should be fine.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d71a65e4af01
Include the stylo pref in telemetry pings. r=gsvelto

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Marco Castelluccio [:marco]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> We can file a socorro issues to make this value a toplevel searchable field.

I'm not sure if this is easily feasible. Adrian, do you know?
This is a value in the TelemetryEnvironment field.

Comment 17

[[DEACTIVATED] Adrian Gaudebert]

(In reply to Marco Castelluccio [:marco] from comment #16)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> > We can file a socorro issues to make this value a toplevel searchable field.
> 
> I'm not sure if this is easily feasible. Adrian, do you know?
> This is a value in the TelemetryEnvironment field.

Given that the TelemetryEnvironment is a JSON blob encoded as a string, we will need to write some code to make that usable in Elasticsearch first. That makes it not trivial, but still very feasible. Please file a bug and assign it to either :willkg or :peterbe.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also don't see a way, even when logged in, to view the telemetry field in the reports list, so it's hard to see if a crash signature is stylo-only -- it requires two clicks for each crash checked.

Comment 19

[Peter Bengtsson [:peterbe]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #18)
> I also don't see a way, even when logged in, to view the telemetry field in
> the reports list, so it's hard to see if a crash signature is stylo-only --
> it requires two clicks for each crash checked.

For the record (because there are multiple bugs going on about this), the TelemetryEnvironment field is now always whitelisted and no longer depends on PII access privileges. 

It's under the Metadata tab. E.g. https://crash-stats.mozilla.com/report/index/a90aa729-63b4-4223-9d4c-133c70170711#tab-metadata