Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Closes #5175: Add Glean keys for distinguishing between fatal and non-fatal native crashes #5700

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

rocketsroger
Copy link
Contributor


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.

@rocketsroger rocketsroger added the 🕵️‍♀️ needs review PRs that need to be reviewed label Jan 23, 2020
@rocketsroger rocketsroger requested a review from pocmo January 23, 2020 16:22
@pocmo pocmo requested review from travis79 and Dexterp37 January 23, 2020 16:23
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #5700 into master will decrease coverage by 0.87%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5700      +/-   ##
============================================
- Coverage     79.44%   78.56%   -0.88%     
+ Complexity     5389     4427     -962     
============================================
  Files           583      581       -2     
  Lines         26452    20942    -5510     
  Branches       3988     3017     -971     
============================================
- Hits          21014    16453    -4561     
+ Misses         3934     3248     -686     
+ Partials       1504     1241     -263
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/mozilla/components/lib/crash/Crash.kt 97.29% <ø> (ø) 1 <0> (ø) ⬇️
...nts/lib/crash/service/GleanCrashReporterService.kt 80% <100%> (+2.72%) 21 <2> (+3) ⬆️
...components/browser/session/LegacySessionManager.kt 88.26% <0%> (ø) 90% <0%> (ø) ⬇️
...a/components/feature/addons/ui/CustomViewHolder.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...illa/components/feature/prompts/file/FilePicker.kt 67.34% <0%> (ø) 21% <0%> (ø) ⬇️
...ozilla/components/feature/addons/AddonsProvider.kt 100% <0%> (ø) 0% <0%> (ø) ⬇️
...re/pwa/intent/TrustedWebActivityIntentProcessor.kt 51.72% <0%> (ø) 7% <0%> (ø) ⬇️
...ents/browser/session/ext/BrowserStoreExtensions.kt 100% <0%> (ø) 0% <0%> (ø) ⬇️
...ture/sitepermissions/db/SitePermissionsDatabase.kt 27.77% <0%> (ø) 1% <0%> (ø) ⬇️
...service/experiments/ExperimentDownloadException.kt 100% <0%> (ø) 2% <0%> (ø) ⬇️
... and 514 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4211d5...a5ca943. Read the comment docs.

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rocketsroger
Copy link
Contributor Author

bors r=travis79

Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires data review.

@travis79
Copy link
Member

This requires data review.

Good catch!

@pocmo pocmo added the blocked Blocked pull requests and issues label Jan 23, 2020
@pocmo
Copy link
Contributor

pocmo commented Jan 23, 2020

bors r-

@bors
Copy link

bors bot commented Jan 23, 2020

Canceled

@rocketsroger
Copy link
Contributor Author

Request for data collection review

  1. What questions will you answer with this data?
    This data will tell us if a native code crash was fatal or non-fatal.

  2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?
    Yes, this gives us an understanding of user experience since a fatal crash is much more disruptive to the user.

  3. What alternative methods did you consider to answer these questions? Why were they not sufficient?
    No alternative is available to answer this question.

  4. Can current instrumentation answer these questions?
    No, current instrumentation only tells us there is a native code crash.

  5. 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.

Measurement Description Data Collection Category Tracking Bug #
Crash is fatal/non-fatal category 1 - technical data #5175
  1. How long will this data be collected? Choose one of the following:
    I want to permanently monitor this data. (Roger Yang)

  2. What populations will you measure?
    No filters - all locales, channels, etc.

  3. If this data collection is default on, what is the opt-out mechanism for users?
    Default Glean SDK opt-out mechanism.

  4. Please provide a general description of how you will analyze this data.
    When high amount of fatal crashes occurs, we will look at crash reporting to see if there are any recently introduced crashes.

  5. Where do you intend to share the results of your analysis?
    Internally in the fenix/android-components teams.

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

@rocketsroger
Copy link
Contributor Author

@liuche @boek Please review the request for data collection. Thanks!

@rocketsroger rocketsroger requested a review from boek January 23, 2020 19:14
Copy link
Contributor

@liuche liuche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data-review+ only

Data Review Form (to be filled by Data Stewards)

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

Yes, changes to the existing probe is documented in metrics.md, and also updated in README

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

Consumers control data collection, and all Mozilla products that consume AC must have a toggle for controlling data collection.

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

Permanent data collection, is an existing metric, but has tests for the new states.

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

Type 1, type of crash

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

Default on

  1. Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No

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

yes, type of crash, no PII

  1. Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent collection w/ tests

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

No

components/lib/crash/metrics.yaml Show resolved Hide resolved
@liuche liuche removed the request for review from boek January 24, 2020 02:13
Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+! Please consider whether or not adding documentation for the label being remoed is of any use for your case.

@rocketsroger
Copy link
Contributor Author

r+! Please consider whether or not adding documentation for the label being removed is of any use for your case.

Thanks! Agreed, I'll add the documentation.

components/lib/crash/metrics.yaml Outdated Show resolved Hide resolved
components/lib/crash/docs/metrics.md Outdated Show resolved Hide resolved
@rocketsroger rocketsroger removed blocked Blocked pull requests and issues 🕵️‍♀️ needs review PRs that need to be reviewed labels Jan 24, 2020
@rocketsroger
Copy link
Contributor Author

bors r=Dexterp37, travis79

bors bot pushed a commit that referenced this pull request Jan 24, 2020
5700: Closes #5175: Add Glean keys for distinguishing between fatal and non-fatal native crashes r=Dexterp37,travis79 a=rocketsroger



Co-authored-by: Roger Yang <[email protected]>
@bors
Copy link

bors bot commented Jan 24, 2020

Timed out

@rocketsroger
Copy link
Contributor Author

bors retry

bors bot pushed a commit that referenced this pull request Jan 24, 2020
5700: Closes #5175: Add Glean keys for distinguishing between fatal and non-fatal native crashes r=Dexterp37,travis79 a=rocketsroger



Co-authored-by: Roger Yang <[email protected]>
@bors
Copy link

bors bot commented Jan 24, 2020

Build succeeded

  • complete-push

@bors bors bot merged commit a5ca943 into mozilla-mobile:master Jan 24, 2020
@rocketsroger rocketsroger deleted the AC_5175 branch January 24, 2020 19:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants