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.
MetaMask · Mar 6, 2020 · b9e928c · b9e928c
1 parent f9072c1
commit b9e928c
Show file tree

Hide file tree

Showing 18 changed files with 57 additions and 54 deletions.
diff --git a/app/components/UI/AccountList/index.js b/app/components/UI/AccountList/index.js
@@ -182,7 +182,7 @@ class AccountList extends PureComponent {
 			} catch (e) {
 				// Restore to the previous index in case anything goes wrong
 				this.mounted && this.setState({ selectedAccountIndex: previousIndex });
-				Logger.error('error while trying change the selected account', e); // eslint-disable-line
+				Logger.error(e, 'error while trying change the selected account'); // eslint-disable-line
 			}
 			InteractionManager.runAfterInteractions(() => {
 				setTimeout(() => {
@@ -217,7 +217,7 @@ class AccountList extends PureComponent {
 				this.mounted && this.setState({ orderedAccounts });
 			} catch (e) {
 				// Restore to the previous index in case anything goes wrong
-				Logger.error('error while trying to add a new account', e); // eslint-disable-line
+				Logger.error(e, 'error while trying to add a new account'); // eslint-disable-line
 				this.mounted && this.setState({ loading: false });
 			}
 		});

diff --git a/app/components/UI/TransactionElement/TransactionDetails/index.js b/app/components/UI/TransactionElement/TransactionDetails/index.js
@@ -232,7 +232,7 @@ class TransactionDetails extends PureComponent {
 			}
 		} catch (e) {
 			// eslint-disable-next-line no-console
-			Logger.error(`can't get a block explorer link for network `, networkID, e);
+			Logger.error(e, { message: `can't get a block explorer link for network `, networkID });
 		}
 	};
 

diff --git a/app/components/UI/UrlAutocomplete/index.js b/app/components/UI/UrlAutocomplete/index.js
@@ -7,7 +7,6 @@ import { connect } from 'react-redux';
 import WebsiteIcon from '../WebsiteIcon';
 import { colors, fontStyles } from '../../../styles/common';
 import { getHost } from '../../../util/browser';
-import Logger from '../../../util/Logger';
 
 const styles = StyleSheet.create({
 	wrapper: {
@@ -130,11 +129,7 @@ class UrlAutocomplete extends PureComponent {
 	}
 
 	updateResults(results) {
-		try {
-			this.mounted && this.setState({ results });
-		} catch (e) {
-			Logger.error('Autocomplete crash', results);
-		}
+		this.mounted && this.setState({ results });
 	}
 
 	onSubmitInput = () => this.props.onSubmit(this.props.input);

diff --git a/app/components/Views/Browser/index.js b/app/components/Views/Browser/index.js
@@ -270,7 +270,7 @@ class Browser extends PureComponent {
 					resolve(true);
 				},
 				error => {
-					Logger.error(`Error saving tab ${url}`, error);
+					Logger.error(error, `Error saving tab ${url}`);
 					reject(error);
 				}
 			);

diff --git a/app/components/Views/BrowserTab/index.js b/app/components/Views/BrowserTab/index.js
@@ -778,7 +778,7 @@ export class BrowserTab extends PureComponent {
 	handleDeeplinks = async ({ error, params }) => {
 		if (!this.isTabActive()) return false;
 		if (error) {
-			Logger.error('Error from Branch: ', error);
+			Logger.error(error, 'Error from Branch');
 			return;
 		}
 		if (params['+non_branch_link']) {
@@ -1004,7 +1004,7 @@ export class BrowserTab extends PureComponent {
 				this.go(fullUrl);
 				return { url: null };
 			}
-			Logger.error('Failed to resolve ENS name', err);
+			Logger.error(err, 'Failed to resolve ENS name');
 			Alert.alert(strings('browser.error'), strings('browser.failed_to_resolve_ens_name'));
 			this.goBack();
 		}
@@ -1139,7 +1139,7 @@ export class BrowserTab extends PureComponent {
 						try {
 							SearchApi.indexSpotlightItem(item);
 						} catch (e) {
-							Logger.error('Error adding to spotlight', e);
+							Logger.error(e, 'Error adding to spotlight');
 						}
 					}
 					const analyticsEnabled = Analytics.getEnabled();
@@ -1269,7 +1269,7 @@ export class BrowserTab extends PureComponent {
 					break;
 			}
 		} catch (e) {
-			Logger.error(`Browser::onMessage on ${this.state.inputValue}`, e.toString());
+			Logger.error(e, `Browser::onMessage on ${this.state.inputValue}`);
 		}
 	};
 

diff --git a/app/components/Views/Entry/index.js b/app/components/Views/Entry/index.js
@@ -103,7 +103,7 @@ class Entry extends PureComponent {
 
 	handleDeeplinks = async ({ error, params }) => {
 		if (error) {
-			Logger.error('Error from Branch: ', error);
+			Logger.error(error, 'Error from Branch');
 			return;
 		}
 		if (params['+non_branch_link']) {

diff --git a/app/components/Views/ImportPrivateKeySuccess/index.js b/app/components/Views/ImportPrivateKeySuccess/index.js
@@ -87,7 +87,7 @@ class ImportPrivateKeySuccess extends PureComponent {
 			const accountsOrdered = allKeyrings.reduce((list, keyring) => list.concat(keyring.accounts), []);
 			PreferencesController.setSelectedAddress(accountsOrdered[accountsOrdered.length - 1]);
 		} catch (e) {
-			Logger.error('Error while refreshing imported pkey', e);
+			Logger.error(e, 'Error while refreshing imported pkey');
 		}
 		InteractionManager.runAfterInteractions(() => {
 			BackHandler.addEventListener('hardwareBackPress', this.handleBackPress);

diff --git a/app/components/Views/ImportWallet/index.js b/app/components/Views/ImportWallet/index.js
@@ -184,9 +184,7 @@ class ImportWallet extends PureComponent {
 					);
 				}
 			}
-			Logger.log('Sync::startSync', firstAttempt);
-			Logger.log('Sync::startSync', e.toString());
-			Logger.error('Sync::startSync', e);
+			Logger.error(e, { message: 'Sync::startSync', firstAttempt });
 			return false;
 		}
 	};
@@ -315,7 +313,7 @@ class ImportWallet extends PureComponent {
 					await AsyncStorage.setItem('@MetaMask:biometryChoice', opts.biometryType);
 				}
 			} catch (e) {
-				Logger.error('User cancelled biometrics permission', e);
+				Logger.error(e, 'User cancelled biometrics permission');
 				await AsyncStorage.removeItem('@MetaMask:biometryChoice');
 				await AsyncStorage.setItem('@MetaMask:biometryChoiceDisabled', 'true');
 				await AsyncStorage.setItem('@MetaMask:passcodeDisabled', 'true');
@@ -338,7 +336,7 @@ class ImportWallet extends PureComponent {
 			this.dataToSync = null;
 			this.props.navigation.push('SyncWithExtensionSuccess');
 		} catch (e) {
-			Logger.error('Sync::disconnect', e);
+			Logger.error(e, 'Sync::disconnect');
 			Alert.alert(strings('sync_with_extension.error_title'), strings('sync_with_extension.error_message'));
 			this.setState({ loading: false });
 			this.props.navigation.goBack();

diff --git a/app/components/Views/LockScreen/index.js b/app/components/Views/LockScreen/index.js
@@ -151,7 +151,7 @@ class LockScreen extends PureComponent {
 			if (this.unlockAttempts <= 3) {
 				this.attemptUnlock();
 			} else {
-				Logger.error('Lockscreen:maxAttemptsReached', { attemptNumber: this.unlockAttempts, error });
+				Logger.error(error, { message: 'Lockscreen:maxAttemptsReached', attemptNumber: this.unlockAttempts });
 				this.props.navigation.navigate('Login');
 			}
 		}

diff --git a/app/components/Views/PaymentChannel/index.js b/app/components/Views/PaymentChannel/index.js
@@ -323,8 +323,7 @@ class PaymentChannel extends PureComponent {
 			!this.state.connextStateDisabled &&
 				Alert.alert(strings('payment_channel.error_title'), strings('payment_channel.error_desc'));
 			this.setState({ connextStateDisabled: true });
-			Logger.log('InstaPay:ChainSawError', channelState);
-			Logger.error('InstaPay:ChainSawError');
+			Logger.error(new Error('InstaPay:ChainSawError'), { channelState });
 		}
 	};
 

diff --git a/app/components/Views/Settings/AdvancedSettings/index.js b/app/components/Views/Settings/AdvancedSettings/index.js
@@ -225,7 +225,7 @@ class AdvancedSettings extends PureComponent {
 				url
 			});
 		} catch (err) {
-			Logger.error('State log error', err);
+			Logger.error(err, 'State log error');
 		}
 	};
 
@@ -253,7 +253,7 @@ class AdvancedSettings extends PureComponent {
 				url
 			});
 		} catch (err) {
-			Logger.error('Instapay log error', err);
+			Logger.error(err, 'Instapay log error');
 		}
 	};
 

diff --git a/app/components/Views/Settings/SecuritySettings/index.js b/app/components/Views/Settings/SecuritySettings/index.js
@@ -368,7 +368,7 @@ class Settings extends PureComponent {
 			if (e.message === 'Invalid password') {
 				Alert.alert(strings('app_settings.invalid_password'), strings('app_settings.invalid_password_message'));
 			}
-			Logger.error('SecuritySettings:biometrics', e);
+			Logger.error(e, 'SecuritySettings:biometrics');
 			// Return the switch to the previous value
 			if (type === 'biometrics') {
 				this.setState({ biometryChoice: !enabled });

diff --git a/app/components/Views/SyncWithExtension/index.js b/app/components/Views/SyncWithExtension/index.js
@@ -163,9 +163,7 @@ class SyncWithExtension extends PureComponent {
 					);
 				}
 			}
-			Logger.log('Sync::startSync', firstAttempt);
-			Logger.log('Sync::startSync', e.toString());
-			Logger.error('Sync::startSync', e);
+			Logger.error(e, 'Sync::startSync', { firstAttempt });
 			return false;
 		}
 	};
@@ -265,7 +263,7 @@ class SyncWithExtension extends PureComponent {
 					}
 					await AsyncStorage.setItem('@MetaMask:biometryChoice', this.state.biometryType);
 				} catch (e) {
-					Logger.error('User cancelled biometrics permission', e);
+					Logger.error(e, 'User cancelled biometrics permission');
 					await AsyncStorage.removeItem('@MetaMask:biometryChoice');
 				}
 			}
@@ -283,7 +281,7 @@ class SyncWithExtension extends PureComponent {
 			this.dataToSync = null;
 			this.props.navigation.push('SyncWithExtensionSuccess');
 		} catch (e) {
-			Logger.error('Sync::disconnect', e);
+			Logger.error(e, 'Sync::disconnect');
 			Alert.alert(strings('sync_with_extension.error_title'), strings('sync_with_extension.error_message'));
 			this.setState({ loading: false });
 			this.props.navigation.goBack();

diff --git a/app/components/Views/WalletConnectSessions/index.js b/app/components/Views/WalletConnectSessions/index.js
@@ -115,7 +115,7 @@ export default class WalletConnectSessions extends PureComponent {
 			);
 			this.loadSessions();
 		} catch (e) {
-			Logger.error('WC: Failed to kill session', e);
+			Logger.error(e, 'WC: Failed to kill session');
 		}
 	};
 

diff --git a/app/core/PaymentChannelsClient.js b/app/core/PaymentChannelsClient.js
@@ -156,7 +156,7 @@ class PaymentChannelsClient {
 								this.setState({ blocked: false });
 							}, 60 * BLOCKED_DEPOSIT_DURATION_MINUTES * 1000);
 						}
-						Logger.error('ExternalWallet::sign', e);
+						Logger.error(e, 'ExternalWallet::sign');
 						throw e;
 					}
 				}
@@ -179,7 +179,7 @@ class PaymentChannelsClient {
 			});
 		} catch (e) {
 			this.logCurrentState('PC::createClient');
-			Logger.error('PC::createClient', e);
+			Logger.error(e, 'PC::createClient');
 			throw e;
 		}
 	}
@@ -203,7 +203,7 @@ class PaymentChannelsClient {
 			Logger.log('PC::pollConnextState connext.start succesful');
 		} catch (e) {
 			this.logCurrentState('PC::start');
-			Logger.error('PC::start', e);
+			Logger.error(e, 'PC::start');
 		}
 		// register connext listeners
 		connext.on('onStateChange', async state => {
@@ -229,7 +229,7 @@ class PaymentChannelsClient {
 				}
 			} catch (e) {
 				this.logCurrentState('PC::onStateChange');
-				Logger.error('PC::onStateChange', e);
+				Logger.error(e, 'PC::onStateChange');
 			}
 		});
 	}
@@ -265,7 +265,7 @@ class PaymentChannelsClient {
 			await this.autoSwap();
 		} catch (e) {
 			this.logCurrentState('PC::autoswap');
-			Logger.error('PC::autoswap', e);
+			Logger.error(e, 'PC::autoswap');
 			this.setState({ swapPending: false });
 		}
 		this.autoswapHandler = setTimeout(() => {
@@ -380,7 +380,7 @@ class PaymentChannelsClient {
 			this.setState({ depositPending: true });
 		} catch (e) {
 			this.logCurrentState('PC::deposit');
-			Logger.error('PC::deposit', e);
+			Logger.error(e, 'PC::deposit');
 			throw e;
 		}
 	};
@@ -419,7 +419,7 @@ class PaymentChannelsClient {
 			await connext.buy(data);
 		} catch (e) {
 			this.logCurrentState('PC::buy');
-			Logger.error('PC::buy', e);
+			Logger.error(e, 'PC::buy');
 		}
 	};
 
@@ -443,7 +443,7 @@ class PaymentChannelsClient {
 			this.setState({ withdrawalPending: true, withdrawalPendingValue: toWei(renderFromWei(balanceTokenUser)) });
 		} catch (e) {
 			this.logCurrentState('PC::withdraw');
-			Logger.error('PC::withdraw', e);
+			Logger.error(e, 'PC::withdraw');
 		}
 	};
 
@@ -483,7 +483,7 @@ const instance = {
 				await client.pollAndSwap();
 			} catch (e) {
 				client.logCurrentState('PC::init');
-				Logger.error('PC::init', e);
+				Logger.error(e, 'PC::init');
 			}
 		}
 	},

diff --git a/app/util/Logger.js b/app/util/Logger.js
@@ -1,6 +1,6 @@
 'use strict';
 
-import { addBreadcrumb, captureException } from '@sentry/react-native';
+import { addBreadcrumb, captureException, withScope } from '@sentry/react-native';
 import AsyncStorage from '@react-native-community/async-storage';
 
 /**
@@ -32,17 +32,27 @@ export default class Logger {
 	/**
 	 * console.error wrapper
 	 *
-	 * @param {object} args - data to be logged
+	 * @param {Error} error - error to be logged
+	 * @param {string|object} extra - Extra error info
 	 * @returns - void
 	 */
-	static async error(...args) {
+	static async error(error, extra) {
 		// Check if user passed accepted opt-in to metrics
 		const metricsOptIn = await AsyncStorage.getItem('@MetaMask:metricsOptIn');
 		if (__DEV__) {
-			args.unshift('[MetaMask DEBUG]:');
-			console.warn(args); // eslint-disable-line no-console
+			console.warn('[MetaMask DEBUG]:', error); // eslint-disable-line no-console
 		} else if (metricsOptIn === 'agreed') {
-			captureException(args);
+			if (extra) {
+				if (typeof extra === 'string') {
+					extra = { message: extra };
+				}
+				withScope(scope => {
+					scope.setExtras(extra);
+					captureException(error);
+				});
+			} else {
+				captureException(error);
+			}
 		}
 	}
 }
diff --git a/app/util/middlewares.js b/app/util/middlewares.js
@@ -28,7 +28,8 @@ export function createLoggerMiddleware(opts) {
 	return function loggerMiddleware(/** @type {any} */ req, /** @type {any} */ res, /** @type {Function} */ next) {
 		next((/** @type {Function} */ cb) => {
 			if (res.error) {
-				Logger.error('Error in RPC response:\n', res);
+				const { error, ...resWithoutError } = res;
+				Logger.error(error, { message: 'Error in RPC response', res: resWithoutError });
 			}
 			if (req.isMetamaskInternal) {
 				return;

diff --git a/app/util/syncWithExtension.js b/app/util/syncWithExtension.js
@@ -142,15 +142,18 @@ export default class PubNubWrapper {
 		this.pubnubListener = {
 			message: ({ channel, message }) => {
 				if (channel !== this.channelName || !message) {
-					Logger.log('Sync::message', channel !== this.channelName, !message);
-					Logger.error('Sync::message', channel !== this.channelName, !message);
+					Logger.error(new Error('Unrecognized message'), {
+						thisChannelName: this.channelName,
+						channel,
+						message
+					});
 					this.timeout = false;
 					return false;
 				}
 				if (message.event === 'error-sync') {
 					this.timeout = false;
 					this.disconnectWebsockets();
-					Logger.error('Sync::error-sync');
+					Logger.error(new Error('Sync::error-sync'), { channel, message });
 					onErrorSync();
 				}
 				if (message.event === 'syncing-data' && message.currentPkg > this.lastReceivedPkg) {
@@ -162,8 +165,7 @@ export default class PubNubWrapper {
 							const data = JSON.parse(this.incomingDataStr);
 							onSyncingData(data);
 						} catch (e) {
-							Logger.log('Sync::parsing', e.toString());
-							Logger.error('Sync::parsing', e);
+							Logger.error(e, 'Sync::parsing');
 						}
 					}
 				}