1950749 - [meta] Improve developer ergonomics of adding new crash annotations to glean pings

Status: ASSIGNED

(Toolkit :: Crash Reporting, enhancement)

Description

[Alex Franchuk [:afranchuk]]

Because Glean didn't support object metric types when the crash ping was originally created, annotations were manually translated to metrics. This is cumbersome. Ideally, a developer who wants to add an annotation can do so by simply editing CrashAnnotations.yaml and adding code to set the annotation. Instead, they must edit CrashAnnotations.yaml (some code paths will still use it for various purposes besides the legacy ping), the desktop and/or android metrics.yaml glean definitions, and the desktop/crashreporter client code to populate the metrics and/or the android client code to populate the metrics (all in different languages!).

There are a few solutions to this problem (numbered here for easy referencing):

1. Bug 1911301 would do a lot to mitigate the overhead here by centralizing the conversion code in one place and in one language. That would arguably help out a lot: asking developers to update multiple programming languages isn't great.
2. By adding a little more information to CrashAnnotations.yaml (where necessary), we may be able to generate the metrics files and/or conversions code.
3. We could change the crash ping to use an object metric for annotations. This makes querying annotation information in e.g. BigQuery slightly more involved, and also removes the ability to store particular annotations in more intelligent ways (quantity metrics with units, string lists, etc). However, it would allow the code that stores annotations in glean metrics to be generic. We could potentially also store some things more intelligently by adding extra information to CrashAnnotations.yaml. object metrics don't allow arbitrary keys in objects, however we could use a definition like:

type: object
properties:
  stringAnnotations:
    type: array
    items:
      type: object
      properties:
        key: string
        value: string
  boolAnnotations:
    type: array
    items:
      type: object
      properties:
        key: string
        value: string
...

4. Crash annotations could be stored using the glean API, dropping CrashAnnotations.yaml altogether. This is unlikely to be tenable since the annotation API is optimized for efficiency and for running in degraded circumstances. The API also features the ability to extract annotations from other processes, which would need to be reworked in some (likely significant) way.

Comment 1

[Alex Franchuk [:afranchuk]]

The ping scope in CrashAnnotations.yaml is manually verified when adding Glean crash ping metrics (i.e., we don't have any way to programmatically verify that annotations put in the metrics are of the scope described in CrashAnnotations.yaml). We could add a lint for this, though it would require enumerating the mapping of annotations to metrics. This is something that would likely be fixed by reworking the ergonomics here.

Another possible solution:

5. Use the metrics.yaml as the new CrashAnnotations.yaml. I.e., it may be possible to define all the annotations within Glean definitions and change the crash annotations codegen to read from the metrics file rather than from the CrashAnnotations.yaml format. This is similar in spirit to solution 2: combining two separate definitions into one.

Comment 2

[Alex Franchuk [:afranchuk]]

Plan

After some thought, I've come up with the following plan:

1. Change CrashAnnotations.yaml to instead be a Rust source file, so that the more complicated conversion logic from annotations to Glean metrics (e.g., stack traces) can be written once.
2. Use this new Rust source in:
   • a build-time program to generate the language bindings for annotations (we still want language-native annotation enums),
   • the crash reporter client,
   • either linked into firefox or invoked through the crash reporter client binary, and
   • linked into Android. This will be either combined into the same library that has minidump_analyzer, or the same one as crashhelper, to avoid yet another library (we will combine those in the future at some point anyway, when the crash helper does the minidump analysis itself).
3. Because Glean uses global statics, also send the crash ping from this new module.

Reasoning

• This allows conversion code to be written in one language, rather than being generated in multiple languages (which would be higher maintenance to test, and more brittle to changes in Glean, language lints, etc).
• Developers adding a crash annotation can do so by adding a definition to the Rust source file of annotations (which can be designed to be fairly declarative). If there is special logic to convert the annotation to a Glean metric, it can be written there, but otherwise default behaviors can handle the rest, and the developers would need only to then set the annotation in their code (no touching Glean metrics.yaml files, nor setting up conversions in many languages).
• This is an incremental step toward an improved crash lifecycle. This code could be easily integrated into the crash helper in the future (which would be handling crash pings and reports).

Concession

I have a fairly clear idea of what should be done to implement this, however we could instead keep annotations in CrashAnnotations.yaml and generate all the language-specific code. I estimate this would take more time to develop/test (the only-Rust solution can piggyback off of the existing infrastructure like minidump-analyzer or crashhelper for "distribution" among the language runtimes), however the real cost would be future maintenance.

Comment 3

[Gabriele Svelto [:gsvelto]]

I like this idea, and I especially like the fact that we could use the same rust code both at runtime and at compile time. Testing coverage is the cherry on top. We could use this to also further enforce static constraints on crash annotations (such as types and possibly even values, e.g. ensuring some annotation can only hold a subset of values, or the value must have certain constraints, etc...).

Comment 4

[Alex Franchuk [:afranchuk]]

I have been working on this, however the build system does not have much support for running compile-time Rust (aside from, perhaps, using a build script to produce side effects). The #build folks advised that I stick to using Python (at least for the time being) for the compile-time aspects. In the future there may be first-class support for running compile-time Rust: it sounded as if they were kicking around ideas to get it to work, because application-services developers need it, too. We might be able to get away with a cargo build script doing it, but for this bug I'm sticking with the Python. There still will be one Rust library that does the work across all platform environments, so we'll have reduced maintenance burden from that.

FYI the main issue #build were concerned with was supporting artifact builds.

Comment 5

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1950749 p1 - Move annotations-related things to their own folder. r=gsvelto

This leaves CrashAnnotations.yaml where it was to not confuse people
and tooling looking for it.

Comment 6

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1950749 p2 - Refactor generation script to have one entry point. r=gsvelto

Since the Java/Kotlin implementation worked based on the file extension,
I figured it could work that way for everthing.

Comment 7

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1950749 p3 - Update the CrashAnnotations.yaml format to include Glean information. r=gsvelto

Comment 8

[Alex Franchuk [:afranchuk]]

Attachment: WIP: Bug 1950749 p4 - Create the pingsender crate. r=gsvelto

This crate handles converting crash annotations to Glean metrics and
sending the crash ping.

Glean metrics are generated directly from CrashAnnotations.yaml.

Comment 9

[Alex Franchuk [:afranchuk]]

Attachment: WIP: Bug 1950749 p5 - Use the pingsender crate in the crashreporter client. r=gsvelto

Comment 10

[Alex Franchuk [:afranchuk]]

Attachment: WIP: Bug 1950749 p6 - Use the pingsender crate for firefox desktop crashes. r=gsvelto

This uses the crash reporter client to send the ping. This will be
cleaner once we remove the CrashService/CrashManager, so that was can
immediately pass the ping ownership to the crash reporter client (and it
can handle storage, minidump analysis, etc). For now, we take this less
elegant approach so to preserve the existing behavior.

Comment 11

[Alex Franchuk [:afranchuk]]

Attachment: WIP: Bug 1950749 p7 - Use the pingsender crate for Android crashes. r=gsvelto

We combine the minidump-analyzer and pingsender crates into one JNI
shared library to be loaded by lib-crash.

Comment 12

[Gabriele Svelto [:gsvelto]]

There's a lot of independent bits here. I'd prefer if you'd split them into different bugs which we'll land one at a time instead of as a set given that they don't strictly depend on each other. In particular it feels to me that pt 1 & 2 could land together, then pt 3 and finally pt 4-7. You could turn this into a meta bug given it's broader scope and then move the patches into dependent bugs, each scoping out one of the changes.

Comment 13

[Phabricator Automation]

Comment on attachment 9513507 [details]
Bug 1950749 p1 - Move annotations-related things to their own folder. r=gsvelto

Revision D264966 was moved to bug 1989407. Setting attachment 9513507 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9513508 [details]
Bug 1950749 p2 - Refactor generation script to have one entry point. r=gsvelto

Revision D264967 was moved to bug 1989407. Setting attachment 9513508 [details] to obsolete.

Comment 15

[Phabricator Automation]

Comment on attachment 9513510 [details]
WIP: Bug 1950749 p4 - Create the pingsender crate. r=gsvelto

Revision D264969 was moved to bug 1989439. Setting attachment 9513510 [details] to obsolete.

Comment 16

[Phabricator Automation]

Comment on attachment 9513511 [details]
WIP: Bug 1950749 p5 - Use the pingsender crate in the crashreporter client. r=gsvelto

Revision D264970 was moved to bug 1989439. Setting attachment 9513511 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9513513 [details]
WIP: Bug 1950749 p6 - Use the pingsender crate for firefox desktop crashes. r=gsvelto

Revision D264971 was moved to bug 1989439. Setting attachment 9513513 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9513514 [details]
WIP: Bug 1950749 p7 - Use the pingsender crate for Android crashes. r=gsvelto

Revision D264972 was moved to bug 1989439. Setting attachment 9513514 [details] to obsolete.