1993525 - Add a unique crash id to crash annotations

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Alex Franchuk [:afranchuk]]

Crash pings and reports cannot be correlated in a good way. You can use the minidump hash as a proxy for a unique id, however this doesn't work if the minidump is missing (whether due to error or due to the crash not being a native crash). Having a unique id created when the crash occurs would also allow us to have the highest confidence that we can deduplicate submissions and truly count individual crash events.

Comment 1

[Gabriele Svelto [:gsvelto]]

I think we'd need to talk about this with a data steward first. In the past we explicitly didn't want to correlate two distinct data sets, especially when one of the two might contain PII. The minidump hash was meant for us to be able to compare signatures between crash pings and crash reports and make sure signature generation worked correctly. At the time we didn't want a direct correlation so that was the best next thing.

Comment 2

[Alex Franchuk [:afranchuk]]

Right. I hope that in this case it will be acceptable, since the reports are strictly augmenting the information (i.e., the ping data is a subset of the report data). While you could correlate the reports, on crash-stats you shouldn't be able to access any more information than was already available in the crash ping (without having special access, of course). My long-term goal is for crash-stats to have all ping and report data, so you can see all crashes that have occurred.

Comment 3

[Gabriele Svelto [:gsvelto]]

Keep in mind that the minidump hash - which doesn't provide a 1-to-1 correlation with the crash ping dataset - is still considered protected data. Whatever we go for it won't be public and will require protected data access. We don't want correlations to be possible between public datasets.

Comment 4

[Chris H-C :chutten]

(( :gsvelto's recollection matches mine. Gosh, that was a while ago, wasn't it. ))

Generally speaking, if you need a piece of data to do a job, you propose to collect it. In doing so you think about whether you can do without it, what privacy implications we need to consider, and write it down. Then you put it up for data collection review. This proposed data collection would be popped up to Sensitive Review because it adds a new identifier, and that's one of the criteria we look for when deciding whether it's something a Steward can give a fast "yes" to.

I think sending a UUID that is protected on the report side, but 'public' (MoCo-conf, anyway) on the "crash" ping side is likely to be approved given the strong, proven risk mitigations (technical and process) we have around Crash Report data. I also believe it's the least data we could collect to do what you want to do with it.

But that's for the panel to decide and give permission, not me : )

Comment 5

[Alex Franchuk [:afranchuk]]

Attachment: Bug 1993525 - Add a unique crash id to crash annotations r=gsvelto,#data-stewards

For native crashes, we could use the UUID name of the minidump file for
this (it would make it easy to correlate the files with the
submissions/pings), however since minidump writing and extra file
writing are not done in lockstep, I chose not to, for now. I didn't want
this id to rely on reading the filename.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e144a9353f2e
https://hg.mozilla.org/integration/autoland/rev/8d2a4db9be86
Add a unique crash id to crash annotations r=gsvelto,android-reviewers,nalexander

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8d2a4db9be86

Comment 8

[Bianca Danforth [:bdanforth]]

afranchuk: I know I mentioned this in Slack a while ago, and sorry we didn't catch it before this landed, but I do think the name of this field needs to change. CrashID is already a well-established term in Socorro for the uuid of the crash report, which has a different meaning from your crash ID. Here's all the places we use our CrashID everywhere in the Antenna codebase: https://github.com/search?q=repo%3Amozilla-services%2Fantenna%20crashid&type=code and in Socorro: https://socorro.readthedocs.io/en/latest/glossary.html#term-crash-id.

You had asked about using your ID as the ID in Socorro, but as I mentioned in the RFC comment, this would open us up to overwriting crash reports from malicious submissions and we use a different format that we validate in places.

Comment 9

[Alex Franchuk [:afranchuk]]

WRT using the ID as the Socorro ID: I don't mind if we decide to not do that, but I don't think it would be providing an opening for overwriting crash reports: Socorro should refuse to overwrite them in any situation, in my opinion. That comes down to how ingestion works, which would change a bit if we were to adopt the ID from the annotation anyway, so I think those changes would occur together to secure the behavior.

I feel somewhat strongly that the field could remain named as-is, on the premise that the name makes sense in the context of Firefox crash reporting in the client. Could we alternatively rename it or hide it on Socorro, if the concern is user confusion? Or do you think it may be too ripe for confusion for developers, too?

I'd prefer not to lose the ability to correlate the month of pings/reports we have so far (insofar as it'd be annoying for queries to try to correlate on the old name when we do a backfill, so we'd probably just use the new name and lose that data). However, in the grand scheme of things, clarity to users and developers is more important.

Would CrashEventID be acceptable?

Comment 10

[Bianca Danforth [:bdanforth]]

I don't think it would be providing an opening for overwriting crash reports: Socorro should refuse to overwrite them in any situation, in my opinion. That comes down to how ingestion works, which would change a bit if we were to adopt the ID from the annotation anyway, so I think those changes would occur together to secure the behavior.

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=2032276, which I CCed you on yesterday on this point. We don't check for duplicate submissions. It is possible, but the data from the client generally can't be trusted as a public endpoint.

That reason aside, we also encode the date and throttle result in the crash report uuid (our crash id). Aside from all the references and terminology already having a definition of the server-generated crash id, we partition storage based on the submission date in our crash id as one example of how we use this information internally. So a randomly generated uuid will not be equivalent.

Could we alternatively rename it or hide it on Socorro, if the concern is user confusion? Or do you think it may be too ripe for confusion for developers, too?

This would certainly confuse me, and I don't think there's a precedent for renaming a crash annotation that we simply copy into the processed crash report. This would be additional overhead for developers who are trying to search on that field as well.

I'd prefer not to lose the ability to correlate the month of pings/reports we have so far (insofar as it'd be annoying for queries to try to correlate on the old name when we do a backfill, so we'd probably just use the new name and lose that data). However, in the grand scheme of things, clarity to users and developers is more important.

I know it's inconvenient, but one month in the grand scheme of things (think: Socorro is 20+ years old) is a relatively small period of time!

Would CrashEventID be acceptable?
Naming things is hard. I don't see any references to such an annotation, title case or snake case, in either Socorro or Antenna, so that SGTM if that works for you.

Comment 11

[(not currently active) Ted Mielczarek]

I just want to note that there has always been a UUID associated with each minidump written, it's the basename of the minidump file:
https://searchfox.org/firefox-main/rev/3934f7874da5d43c3eac71e6f44e0afe79d9071f/toolkit/crashreporter/nsExceptionHandler.cpp#1504