There are a bunch of crash annotations that are flags where they are in the crash report with a "1" value or they're not in the crash report at all. For example, DOMFissionEnabled works like this. This has type boolean.

There are other crash annotations that may or may not be in the crash report, but they have values of "true" and "false". For example, A11yHandlerRegistered. This has type string.

The problem is that Socorro handles all these boolean-ish things differently and they're stored differently in Elasticsearch as well. In some cases, they're stored as shorts. In some cases, they're stored as booleans and a null_value means False.

This bug covers unifying this mess into a set of conventions and types that works with how people are querying these values and works with future versions of Elasticsearch.

I think we should convert "1" / "true" to true and "0" / "false" to false and store them in Elasticsearch as booleans and not otherwise affect exists/does-not-exist. Other values would be invalid.

So then we need to figure out how to migrate from the old mapping to a new mapping in Elasticsearch. Is Elasticsearch going to be ok with fields that are declared as boolean in one mapping and short in another? Probably need to do some tests with our current version of Elasticsearch to figure out whether this is going to be ok or not.

CrashAnnotations.yaml has different kinds of boolean-ish annotations:

• string "true" and "false" values (or aren't there):
  • AllyHandlerRegistered
  • ProxyStreamValid: only "false" or not there
• boolean true and false values (or aren't there):
  • BackgroundTaskMode
  • ContainsMemoryReport
  • HeadlessMode
  • IsGarbageCollecting: true or not there
  • IsWayland: true or not there
  • IsWebRenderResourcePathOverridden: true or not there
  • LinuxUnderMemoryPressure: true or not there
• boolean "1" or not there:
  • BlocklistInitFailed
  • ContentSandboxEnabled
  • ContentSandboxCapable
  • CrashAddressLikelyWrong
  • DOMFissionEnabled
  • DOMIPCEnabled
  • EMCheckCompatibility
  • GMPPlugin
  • GraphicsStartupTest
  • HasDeviceTouchScreen -- note that on mobile, we always assume a touch-screen is present, but this is probably not set; this is an example where we probably want to normalize this differently depending on the product
  • SafeMode
  • StartupCrash
  • SubmittedFromInfobar
  • User32BeforeBlocklist
  • WasmLibrarySandboxMallocFailed
  • WindowsErrorReporting

All of these fields feel boolean-ish even if the logic around the value is different. I think we should handle these fields as follows:

1. validate and normalize annotation value:
   • "1", "true", 1, true -> True
   • "0", "false", 0, false -> False
   • anything else results in a processor note and the field omitted from the processed crash
2. if the field is in the processed crash, it is indexed as a boolean (true, false)
3. if the field is not in the processed crash, then the field is not indexed
4. if the field is not in the processed crash, then it is not included in the telemetry export and it'll end up as null in telemetry.socorro_crash

In supersearch, you'll be able to do "exists", "does not exist", "is true", and "is false" filters.

I need to figure out if supersearch will be ok searching across indexes with the old mapping and a new mapping.

Gabriele: Does the above seem ok?

Yes, that's fine. Note that the list of values you called "boolean true and false values" should appear in the JSON as "1" and "0" strings (per the description here). If they're being written out in another form then something funny must be happening. Still, the this doesn't change much. All those fields are indeed booleans and your plan is fine. It's doubly fine because if we decide to enforce the types in CrashAnnotations.yaml we could make all the boolean annotations appear as real booleans in the JSON (instead of the current mish-mash of numbers and string values) and Socorro would accept them. That means we'd already have a forward-compatible road towards making this stuff cleaner.

Interesting! I built that list from the the CrashAnnotations.yaml specifications for the fields. For example, BackgroundTaskMode is this:

https://searchfox.org/mozilla-central/rev/ad7ecfa618ec3a65db8405d9f1125059fe4a6a15/toolkit/crashreporter/CrashAnnotations.yaml#146-149

BackgroundTaskMode:
  description: >
    True if the app was invoked in background task mode via `--backgroundtask ...`, false otherwise.
  type: boolean

I wonder if it'd be good to do a pass on CrashAnnotations.yaml and improve descriptions like this. I know I'm encouraging other products to reuse fields where appropriate and it's likely the case that new annotations start with copy-and-paste of an existing annotation.

Thank you for looking this over!

Well, the code that generates the list of annotations in C++ doesn't use the type fields yet. It just turns everything into strings, or uses the strings that are passed to it. The proper fix would be to take those types into account and refactor the calling code so that everything that's supposed to be a boolean is recorded and stored as such; and callers are prevented from passing a string or number instead.

willkg merged PR #6039: "bug 1755528: fix flag/boolean fields" in f927290.

willkg merged PR #6040: "bug 1755528: fix flag/boolean fields that are shorts" in 8e707a2.

If this sticks and gets to prod, then I'll write up a bug to remove the deprecated bits in August 2022.

willkg merged PR #6043: "bug 1755528: fix CopyFromRawCrashRule boolean test" in a76b7ee.

This went to production in bug #1763234 just now. I'll keep this open until Monday when we've created a new Elasticsearch index and can verify things.

I verified several of the fields just now and I'm able to search across indexes and fields are working as I'm expecting them to. Marking as FIXED.