For #11876: Bucket native code crashes by process type in GleanCrashReporterService #11908

jamienicol · 2022-03-21T15:52:32Z

Make GleanCrashReporterService count native code crashes based on
their processType field rather than whether they are fatal or
non-fatal.

Persisted fatal and non-fatal crashes will still be submitted for now,
but this code should be removed in a follow-up patch once we have
allowed time for them to be submitted.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

jamienicol · 2022-03-21T15:53:32Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

What questions will you answer with this data?

Which type of process a native code crash occured in.

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses:

We currently track whether a crash is fatal or non-fatal. This further
distinguishes whether a non-fatal crash occured in a background or foreground process.

This will allow us to keep track of crash numbers even when users do
not report crashes. And importantly monitor whether a larger number of
crashes in background process are going unreported, as they use a
different UI to ask the user to report the crash.

What alternative methods did you consider to answer these questions? Why were they not sufficient?

There is no alternative.

Can current instrumentation answer these questions?

No.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.

Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description	Data Collection Category	Tracking Bug #
Crash process type	Category 1 - technical data	#11876

Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.

Often the Privacy Notice for your product will link to where the documentation is expected to be.
Common examples for Mozilla products/services:
- If this collection is Telemetry you can state "This collection is documented in its definitions files Histograms.json, Scalars.yaml, and/or Events.yaml and in the Probe Dictionary at https://probes.telemetry.mozilla.org."
- If this data is collected using the Glean SDK you can state “This collection is documented in the Glean Dictionary at https://dictionary.telemetry.mozilla.org/"
In some cases, documentation is included in the project’s repository.

This collection is documented in the Glean Dictionary at https://dictionary.telemetry.mozilla.org/

How long will this data be collected? Choose one of the following:

This is scoped to a time-limited experiment/project until date MM-DD-YYYY.
I want this data to be collected for 6 months initially (potentially renewable).
I want to permanently monitor this data. (put someone’s name here)

I want to permanently monitor this data. Jamie Nicol

What populations will you measure?

Which release channels?
Which countries?
Which locales?
Any other filters? Please describe in detail below.

All release channels, countries and locales. No other filters.

If this data collection is default on, what is the opt-out mechanism for users?

Default telemetry opt-opt.

Please provide a general description of how you will analyze this data.

If a large number of crashes are detected I will look at socorro to
see if any crashes have been found.

If a large discrepancy is found between the number of crash pings
counted here and the number of reports submitted to socorro, we will
re-evaluate the crash-reporting UI for background child process crashes.

Where do you intend to share the results of your analysis?

On github, bugzilla.

Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection? If so:

Are you using that on the Mozilla backend? Or going directly to the third-party?

No third party tool.

jamienicol · 2022-03-21T15:54:32Z

I'll need to update the yaml file with the new data review link, once completed.

rocketsroger

Looks good! Some questions. Thanks

rocketsroger · 2022-03-22T14:03:31Z

...ts/lib/crash/src/main/java/mozilla/components/lib/crash/service/GleanCrashReporterService.kt

@@ -36,6 +36,12 @@ class GleanCrashReporterService(
        // as the persisted crashes in the crash count file (see above comment)
        const val UNCAUGHT_EXCEPTION_KEY = "uncaught_exception"
        const val CAUGHT_EXCEPTION_KEY = "caught_exception"
+        const val MAIN_PROCESS_NATIVE_CODE_CRASH_KEY = "main_proc_native_code_crash"
+        const val FOREGROUND_CHILD_PROCESS_NATIVE_CODE_CRASH_KEY = "fg_proc_native_code_crash"


nit: for me just using foreground / background (vs fg / bg) here is more readable

The build fails if the keys are too long, unfortunately

👍 That's unfortunate. Thanks!

I think foreground_native_code_crash (without the proc) would fit. Would you prefer that?

Sure! That sounds good too.

Or perhaps remove the code, ie foreground_proc_native_crash?

Ah, no. Unfortunately none of those options work. I think we'll need to leave it as it is...

Oh yeah that's even better. then it matches main_proc_native_crash.

No worries, just a small nit. Thanks

rocketsroger · 2022-03-22T14:04:26Z

...ts/lib/crash/src/main/java/mozilla/components/lib/crash/service/GleanCrashReporterService.kt

@@ -132,6 +144,9 @@ class GleanCrashReporterService(
            when (line) {
                UNCAUGHT_EXCEPTION_KEY -> ++uncaughtExceptionCount
                CAUGHT_EXCEPTION_KEY -> ++caughtExceptionCount
+                MAIN_PROCESS_NATIVE_CODE_CRASH_KEY -> ++mainProcessNativeCodeCrashCount
+                FOREGROUND_CHILD_PROCESS_NATIVE_CODE_CRASH_KEY -> ++foregroundChildProcessNativeCodeCrashCount
+                BACKGROUND_CHILD_PROCESS_NATIVE_CODE_CRASH_KEY -> ++backgroundChildProcessNativeCodeCrashCount
                FATAL_NATIVE_CODE_CRASH_KEY -> ++fatalNativeCodeCrashCount


Should these be removed?

@Dexterp37 suggested here that we keep the deprecated keys around for a short while so that the pending data can still be submitted correctly.

Understood. Please create an issue to track removing the deprecated keys. Thanks

rocketsroger · 2022-03-22T14:13:45Z

...ts/lib/crash/src/main/java/mozilla/components/lib/crash/service/GleanCrashReporterService.kt

+        var foregroundChildProcessNativeCodeCrashCount = 0
+        var backgroundChildProcessNativeCodeCrashCount = 0
+        // These keys are deprecated and should be removed after a period to allow for persisted
+        // crashes to be submitted.
        var fatalNativeCodeCrashCount = 0


Should these be removed?

rocketsroger · 2022-03-22T14:14:04Z

...ts/lib/crash/src/main/java/mozilla/components/lib/crash/service/GleanCrashReporterService.kt

+        const val BACKGROUND_CHILD_PROCESS_NATIVE_CODE_CRASH_KEY = "bg_proc_native_code_crash"
+
+        // These keys are deprecated and should be removed after a period to allow for persisted
+        // crashes to be submitted.
        const val FATAL_NATIVE_CODE_CRASH_KEY = "fatal_native_code_crash"


Should these be removed?

rocketsroger · 2022-03-22T14:20:07Z

Jamie Nicol

Data Review

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, through the metrics.yaml file and the Glean Dictionary

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, through the "Send Usage Data" preference in the application settings

If the request is for permanent data collection, is there someone who will monitor the data over time?

Jamie Nicol

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical data

Is the data collection request for default-on or default-off?

default-on

Does the instrumentation include the addition of any new identifiers?

No

Is the data collection covered by the existing Firefox privacy notice?

Yes

Does the data collection use a third-party collection tool?

No

Result

data-review+

rocketsroger · 2022-03-22T14:21:30Z

I'll need to update the yaml file with the new data review link, once completed.

Please also add your email to the notification_emails: section. Thanks!

rocketsroger · 2022-03-22T14:31:52Z

Adding label till yaml is updated. Thanks

…in GleanCrashReporterService Make GleanCrashReporterService count native code crashes based on their processType field rather than whether they are fatal or non-fatal. Persisted fatal and non-fatal crashes will still be submitted for now, but this code should be removed in a follow-up patch once we have allowed time for them to be submitted.

rocketsroger

🚢

jamienicol requested a review from rocketsroger as a code owner March 21, 2022 15:52

rocketsroger reviewed Mar 22, 2022

View reviewed changes

rocketsroger approved these changes Mar 22, 2022

View reviewed changes

rocketsroger added the do not land PRs that requires coordination before landing label Mar 22, 2022

jamienicol force-pushed the glean-process-type branch from bdc2742 to 7334793 Compare March 22, 2022 14:35

jamienicol mentioned this pull request Mar 22, 2022

Remove deprecated fatal and non-fatal native code crash keys from GleanCrashReporterService #11912

Closed

rocketsroger removed the do not land PRs that requires coordination before landing label Mar 22, 2022

rocketsroger approved these changes Mar 22, 2022

View reviewed changes

rocketsroger added the 🛬 needs landing PRs that are ready to land label Mar 24, 2022

Merge branch 'main' into glean-process-type

e0d2d31

mergify bot merged commit efe0859 into mozilla-mobile:main Mar 24, 2022

csadilek mentioned this pull request Apr 13, 2022

Amend crash pings to distinguish between FOREGROUND_CHILD and BACKGROUND_CHILD #11876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #11876: Bucket native code crashes by process type in GleanCrashReporterService #11908

For #11876: Bucket native code crashes by process type in GleanCrashReporterService #11908

jamienicol commented Mar 21, 2022

jamienicol commented Mar 21, 2022

jamienicol commented Mar 21, 2022

rocketsroger left a comment

rocketsroger Mar 22, 2022

jamienicol Mar 22, 2022

rocketsroger Mar 22, 2022

jamienicol Mar 22, 2022

rocketsroger Mar 22, 2022

jamienicol Mar 22, 2022

jamienicol Mar 22, 2022

rocketsroger Mar 22, 2022

rocketsroger Mar 22, 2022

rocketsroger Mar 22, 2022

jamienicol Mar 22, 2022

rocketsroger Mar 22, 2022

jamienicol Mar 22, 2022

rocketsroger Mar 22, 2022

rocketsroger Mar 22, 2022

rocketsroger commented Mar 22, 2022

rocketsroger commented Mar 22, 2022

rocketsroger commented Mar 22, 2022

rocketsroger left a comment

For #11876: Bucket native code crashes by process type in GleanCrashReporterService #11908

For #11876: Bucket native code crashes by process type in GleanCrashReporterService #11908

Conversation

jamienicol commented Mar 21, 2022

Pull Request checklist

After merge

jamienicol commented Mar 21, 2022

Request for data collection review form

jamienicol commented Mar 21, 2022

rocketsroger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger commented Mar 22, 2022

Data Review

Result

rocketsroger commented Mar 22, 2022

rocketsroger commented Mar 22, 2022

rocketsroger left a comment

Choose a reason for hiding this comment