Replace Fabric Crashlytics with Sentry #1376

Gudahtt · 2020-02-17T14:42:38Z

Remove Fabric and Crashlytics
Add Sentry reporting
Errors are reported using captureException, and non-errors are added as breadcrumbs. This means the non-error events won't be directly sent to Sentry, but they will be included as context for any errors that do occur.

The sentry configuration is mainly in the sentry.debug.properties file. The only config outside of that file is the DSN used for reporting errors, which is hard-coded in the setupSentry.js module.

There are three separate sets of config: default, debug, and release. The default config is missing the auth token, so it can't be used to publish source maps. However, builds that use the default config will still correctly report errors to the test-metamask-mobile Sentry project.

The debug config is identical to the default config except that it includes the auth token. The debug config is for publishing any non-production builds (e.g. testing, pre-releases).

The release configuration is to be used for production builds only. This is the only config that sends errors to the metemask-mobile Sentry project.

The debug and release config files have not been added to the repo. They are automatically instantiated from the example files if the MM_SENTRY_AUTH_TOKEN environment variable is set. Setting this environment variable in CI should allow publishing from CI.

Closes #1321

android/sentry.properties

app/util/setupSentry.js

android/app/src/main/java/io/metamask/MainApplication.java

Gudahtt · 2020-02-20T03:16:30Z

app/util/Logger.js

 		// Check if user passed accepted opt-in to metrics
 		const metricsOptIn = await AsyncStorage.getItem('@MetaMask:metricsOptIn');
 		if (__DEV__) {
 			args.unshift('[MetaMask DEBUG]:');
 			console.log.apply(null, args); // eslint-disable-line no-console
 		} else if (metricsOptIn === 'agreed') {
-			Fabric.Crashlytics.log(JSON.stringify(args));
+			addBreadcrumb({
+				message: JSON.stringify(args)


There is a lot more useful info we could add here to the breadcrumb, but this is a good start at least. I might improve this later by changing the arguments to allow logging structured data, rather than an arbitrary array

Gudahtt · 2020-02-20T03:19:14Z

To test this locally, I added the line throw new Error('TEST') to the constructor of app/components/Views/Root/index.js. It should then blow up immediately when the app opens.

You can test the breadcrumbs by adding a Logger.log call as well, right above the error.

I wasn't sure how to replicate a native crash offhand - that's something we should test as well.

danjm · 2020-03-03T15:13:50Z

sentry.properties

+defaults.url=https://sentry.io/
+defaults.org=metamask
+defaults.project=test-metamask-mobile
+auth.token=


How does the auth token work? When is it needed?

It is used to publish sourcemaps, which happens as part of the release build

A similar command is in place for the extension (sentry:publish) which also needs a token; any submission of release artifacts requires this token.

Unfortunately with React Native, this command is integrated with the normal build process; I'm not sure how to pull it into a separate step. The modifications to the build process were done automatically when linking @sentry/react-native, and I wasn't eager to customize them. At least it's automatically skipped in dev mode though, so it shouldn't affect dev environments

danjm · 2020-03-03T15:18:39Z

app/util/Logger.js

@@ -18,14 +17,15 @@ export default class Logger {
 	 * @returns - void
 	 */
 	static async log(...args) {
-		// TODO use crashlytics opt-in
 		// Check if user passed accepted opt-in to metrics
 		const metricsOptIn = await AsyncStorage.getItem('@MetaMask:metricsOptIn');
 		if (__DEV__) {
 			args.unshift('[MetaMask DEBUG]:');
 			console.log.apply(null, args); // eslint-disable-line no-console
 		} else if (metricsOptIn === 'agreed') {


Does mobile metrics opt-in include a note about error reporting?

Not sure. I was curious about this as well; on the extension we don't ask for permission to collect error information at all.

danjm · 2020-03-03T15:33:37Z

I've completed code review.

sentry api usage seems correct, and replacement of fabric calls seem correct
javascript code looks sound
code related to fabric and crashlytics has been deleted
build.sh looks fine to my not so bash trained eyes

estebanmino

working on IOS! 🥇

The Logger now logs to `console.log` exclusively.

Errors are reported using `captureException`, and non-errors are added as breadcrumbs. This means the non-error events won't be directly sent to Sentry, but they will be included as context for any errors that do occur. The sentry configuration is mainly in the `sentry.debug.properties` file. The only config outside of that file is the DSN used for reporting errors, which is hard-coded in the `setupSentry.js` module. There are three separate sets of config: default, debug, and release. The default config is missing the auth token, so it can't be used to publish source maps. However, builds that use the default config will still correctly report errors to the `test-metamask-mobile` Sentry project. The debug config is identical to the default config except that it includes the auth token. The debug config is for publishing any non-production builds (e.g. testing, pre-releases). The release configuration is to be used for production builds only. This is the only config that sends errors to the `metemask-mobile` Sentry project. The debug and release config files have not been added to the repo. They are automatically instantiated from the example files if the `MM_SENTRY_AUTH_TOKEN` environment variable is set. Setting this environment variable in CI should allow publishing from CI. Closes #1321

The SENTRY_PROPERTIES environment variable is used at different directory levels in the Android and iOS builds respectively; it's used one level down with iOS, but two levels down for Android. This means the properties path used is incorrect on iOS at the moment. Instead an absolute path is now used, to prevent any such errors in the future. The error message for the missing auth token has been improved as well.

The properties file is now checked to ensure the auth token is present, so the build can fail early rather than partway through. The logic for handling the auth token has been moved to a separate function as well, so it can be shared between the release and prerelease builds.

The Sentry environment is set to `local` by default (e.g. for dev builds), but the build script now requires it to be explicitly set for release builds. This is to ensure it isn't missed in the production builds, which are done manually.

Whenever using the `throw` keyword or calling `reject` in a Promise, the value passed should be an Error. Various tools expect thrown "errors" to be errors; this is the convention. Using Error types is also more useful for debugging, as they have stack traces.

`Logger.error` now expects an actual error object as the first argument. This allows us to submit proper Errors with stack traces to Sentry. The second parameter is an optional set of extra data, which gets attached to the Error and is viewable on Sentry in the "Additional Data" section of the error report. Unfortunately any messages added to contextualize errors had to be submitted as extra data instead of as a wrapper Error. Wrapper error types provide for a richer debugging experience in general, because you get additional context for the error plus an additional stack trace to help trace the flow of the error. Sentry even has an Integration meant to facilitate this pattern, the `LinkedErrors` integration. But unfortunately that integration doesn't seem to work with React Native, so that option was off the table.

The most recent Sentry packages have better support for capturing native crashes on Android. I had held off on this update at first beceause I thought it required React Native >= 0.60, but apparently not. It still works on 0.59, just not as well (no sourcemaps)

It was expecting an alphabetical key instead of an alphanumeric one.

This reverts commit 2375f80.

* Remove Fabric and Crashlytics The Logger now logs to `console.log` exclusively. * Add Sentry reporting Errors are reported using `captureException`, and non-errors are added as breadcrumbs. This means the non-error events won't be directly sent to Sentry, but they will be included as context for any errors that do occur. The sentry configuration is mainly in the `sentry.debug.properties` file. The only config outside of that file is the DSN used for reporting errors, which is hard-coded in the `setupSentry.js` module. There are three separate sets of config: default, debug, and release. The default config is missing the auth token, so it can't be used to publish source maps. However, builds that use the default config will still correctly report errors to the `test-metamask-mobile` Sentry project. The debug config is identical to the default config except that it includes the auth token. The debug config is for publishing any non-production builds (e.g. testing, pre-releases). The release configuration is to be used for production builds only. This is the only config that sends errors to the `metamask-mobile` Sentry project. The debug and release config files have not been added to the repo. They are automatically instantiated from the example files if the `MM_SENTRY_AUTH_TOKEN` environment variable is set. Setting this environment variable in CI should allow publishing from CI. Closes #1321 * Use absolute path for Sentry file The SENTRY_PROPERTIES environment variable is used at different directory levels in the Android and iOS builds respectively; it's used one level down with iOS, but two levels down for Android. This means the properties path used is incorrect on iOS at the moment. Instead an absolute path is now used, to prevent any such errors in the future. The error message for the missing auth token has been improved as well. * Improve handling of missing auth token The properties file is now checked to ensure the auth token is present, so the build can fail early rather than partway through. The logic for handling the auth token has been moved to a separate function as well, so it can be shared between the release and prerelease builds. * Force setting METAMASK_ENVIRONMENT explicitly for release builds The Sentry environment is set to `local` by default (e.g. for dev builds), but the build script now requires it to be explicitly set for release builds. This is to ensure it isn't missed in the production builds, which are done manually. * Ensure only Errors are thrown/rejected Whenever using the `throw` keyword or calling `reject` in a Promise, the value passed should be an Error. Various tools expect thrown "errors" to be errors; this is the convention. Using Error types is also more useful for debugging, as they have stack traces. * Update `Logger.error` function signature `Logger.error` now expects an actual error object as the first argument. This allows us to submit proper Errors with stack traces to Sentry. The second parameter is an optional set of extra data, which gets attached to the Error and is viewable on Sentry in the "Additional Data" section of the error report. Unfortunately any messages added to contextualize errors had to be submitted as extra data instead of as a wrapper Error. Wrapper error types provide for a richer debugging experience in general, because you get additional context for the error plus an additional stack trace to help trace the flow of the error. Sentry even has an Integration meant to facilitate this pattern, the `LinkedErrors` integration. But unfortunately that integration doesn't seem to work with React Native, so that option was off the table. * Update `@sentry` packages to latest The most recent Sentry packages have better support for capturing native crashes on Android. I had held off on this update at first because I thought it required React Native >= 0.60, but apparently not. It still works on 0.59, just not as well (no sourcemaps)

Gudahtt commented Feb 17, 2020

View reviewed changes

android/sentry.properties Outdated Show resolved Hide resolved

app/util/setupSentry.js Outdated Show resolved Hide resolved

app/util/setupSentry.js Outdated Show resolved Hide resolved

Gudahtt force-pushed the sentry branch 2 times, most recently from 3022bcb to aa8ee4d Compare February 18, 2020 18:12

rickycodes reviewed Feb 18, 2020

View reviewed changes

android/app/src/main/java/io/metamask/MainApplication.java Outdated Show resolved Hide resolved

Gudahtt force-pushed the sentry branch from aa8ee4d to b507698 Compare February 19, 2020 05:51

Gudahtt marked this pull request as ready for review February 19, 2020 06:15

Gudahtt force-pushed the sentry branch 5 times, most recently from 9e8d773 to 3daeba3 Compare February 20, 2020 03:14

Gudahtt commented Feb 20, 2020

View reviewed changes

Gudahtt force-pushed the sentry branch from 3daeba3 to bf65d09 Compare February 26, 2020 14:18

danjm reviewed Mar 3, 2020

View reviewed changes

Gudahtt force-pushed the sentry branch 4 times, most recently from 4a3f7e3 to 8c4334e Compare March 6, 2020 16:41

Gudahtt force-pushed the sentry branch from 8c4334e to b46c78e Compare March 16, 2020 20:39

estebanmino approved these changes Mar 16, 2020

View reviewed changes

Gudahtt force-pushed the sentry branch 2 times, most recently from 24475fa to 656e725 Compare March 17, 2020 17:47

Gudahtt added 5 commits March 23, 2020 12:42

Remove Fabric and Crashlytics

8a246eb

The Logger now logs to `console.log` exclusively.

Gudahtt added 4 commits March 23, 2020 12:42

Fix bug with build script

e16061c

It was expecting an alphabetical key instead of an alphanumeric one.

Gudahtt force-pushed the sentry branch from 4aca574 to e16061c Compare March 23, 2020 15:42

Merge branch 'develop' into sentry

f6a6c1d

Gudahtt merged commit 2375f80 into develop Mar 24, 2020

Gudahtt deleted the sentry branch March 24, 2020 15:35

estebanmino mentioned this pull request Mar 24, 2020

bugfix: build script #1456

Closed

3 tasks

Gudahtt mentioned this pull request Mar 24, 2020

Skip Sentry upload during iOS Debug build #1458

Merged

estebanmino added a commit that referenced this pull request Apr 14, 2020

Revert "Replace Fabric Crashlytics with Sentry (#1376)"

0762c8f

This reverts commit 2375f80.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Fabric Crashlytics with Sentry #1376

Replace Fabric Crashlytics with Sentry #1376

Gudahtt commented Feb 17, 2020 •

edited

Loading

Gudahtt Feb 20, 2020

Gudahtt commented Feb 20, 2020 •

edited

Loading

danjm Mar 3, 2020

Gudahtt Mar 3, 2020

Gudahtt Mar 3, 2020

danjm Mar 3, 2020

Gudahtt Mar 3, 2020

danjm commented Mar 3, 2020

estebanmino left a comment

Replace Fabric Crashlytics with Sentry #1376

Replace Fabric Crashlytics with Sentry #1376

Conversation

Gudahtt commented Feb 17, 2020 • edited Loading

Gudahtt Feb 20, 2020

Choose a reason for hiding this comment

Gudahtt commented Feb 20, 2020 • edited Loading

danjm Mar 3, 2020

Choose a reason for hiding this comment

Gudahtt Mar 3, 2020

Choose a reason for hiding this comment

Gudahtt Mar 3, 2020

Choose a reason for hiding this comment

danjm Mar 3, 2020

Choose a reason for hiding this comment

Gudahtt Mar 3, 2020

Choose a reason for hiding this comment

danjm commented Mar 3, 2020

estebanmino left a comment

Choose a reason for hiding this comment

Gudahtt commented Feb 17, 2020 •

edited

Loading

Gudahtt commented Feb 20, 2020 •

edited

Loading