Skip to content

Metrics adjustments #15313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 22, 2022
Merged

Metrics adjustments #15313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba1becb
Don't send errors to sentry if users have not opted-in to participate…
danjm Jul 21, 2022
31f0ffc
Don't capture opt-out metrics
danjm Jul 21, 2022
dfba519
Move the metrics-opt in screen to immediately after the welcome screen
danjm Jul 21, 2022
1eba37b
Ensure that global.getSentryState is set in the background
danjm Jul 22, 2022
648f49f
Fix e2e tests after rearranging onboardin flow
danjm Jul 22, 2022
791dfac
Fix unit tests
danjm Jul 22, 2022
7173829
Merge branch 'develop' into metrics-adjustments
danjm Jul 22, 2022
f6ae2f6
More e2e test fixes
danjm Jul 22, 2022
d7202b4
Remove unnecessary wrappers around capture exception
danjm Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions app/scripts/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,14 @@ import {
REJECT_NOTFICIATION_CLOSE_SIG,
} from '../../shared/constants/metametrics';
import { isManifestV3 } from '../../shared/modules/mv3.utils';
import { maskObject } from '../../shared/modules/object.utils';
import migrations from './migrations';
import Migrator from './lib/migrator';
import ExtensionPlatform from './platforms/extension';
import LocalStore from './lib/local-store';
import ReadOnlyNetworkStore from './lib/network-store';
import { SENTRY_STATE } from './lib/setupSentry';

import createStreamSink from './lib/createStreamSink';
import NotificationManager, {
NOTIFICATION_MANAGER_EVENTS,
Expand Down Expand Up @@ -353,6 +356,8 @@ function setupController(initState, initLangCode, remoteSourcePort) {
},
);

setupSentryGetStateGlobal(controller.store);

/**
* Assigns the given state to the versioned object (with metadata), and returns that.
*
Expand Down Expand Up @@ -755,3 +760,15 @@ browser.runtime.onInstalled.addListener(({ reason }) => {
platform.openExtensionInBrowser();
}
});

function setupSentryGetStateGlobal(store) {
global.getSentryState = function () {
const fullState = store.getState();
const debugState = maskObject(fullState, SENTRY_STATE);
return {
browser: window.navigator.userAgent,
store: debugState,
version: global.platform.getVersion(),
};
};
}
12 changes: 11 additions & 1 deletion app/scripts/lib/setupSentry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,17 @@ export default function setupSentry({ release, getState }) {
environment,
integrations: [new Dedupe(), new ExtraErrorData()],
release,
beforeSend: (report) => rewriteReport(report),
beforeSend: (report) => {
if (getState) {
Copy link
Member

@Gudahtt Gudahtt Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just tried testing this, and there is still a call made to Sentry upon .init being called. This effectively prevents error reports from being sent, but it doesn't delay all communication. We will need to come up with a different strategy.

Typically I would suggest delaying the call to init. This would require handling captureException calls prior to init somehow (e.g. bring back those wrapper conditions to skip captureException pre-opt-in, or implement our own captureException wrapper function). But it wouldn't be that hard to do.

But for us that might not work, because I suspect that init has side-effects that do not work correctly under lockdown. It's possible they happen on import, not on init, I have not checked yet.. If they happen on init, we're in a tough spot. It means that if we call it later, it'll fail.

Assuming that is true, we might be able to effectively stop all communication by passing in a custom transport. The transport option can be used to handle proxies and things like that, so I would expect it to affect absolutely all communication, even the initial call.

Apparently init doesn't support the transport option anymore despite it being documented, but it can be set by creating our own Sentry client manually. (details here: getsentry/sentry-javascript#5422)

Another option would be to restart the process after opt-in, and somehow communicate to this bundle that the user opted in 🤔 Challenging since it would have to run before the controllers. This would certainly be more disruptive than the other option.

Copy link
Member

@Gudahtt Gudahtt Jul 22, 2022

Choose a reason for hiding this comment

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

I'm going to consider this non-blocking for this PR because what you have implemented so far is still an improvement, it's just not a complete solution yet.

Copy link
Contributor Author

@danjm danjm Jul 22, 2022

Choose a reason for hiding this comment

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

Okay, I will merge and then create a ticket to address the issues you have raised here.

const appState = getState();
if (!appState?.store?.metamask?.participateInMetaMetrics) {
return null;
}
} else {
return null;
}
return rewriteReport(report);
},
});

function rewriteReport(report) {
Expand Down
22 changes: 22 additions & 0 deletions shared/modules/object.utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Return a "masked" copy of the given object.
*
* The returned object includes only the properties present in the mask. The
* mask is an object that mirrors the structure of the given object, except
* the only values are `true` or a sub-mask. `true` implies the property
* should be included, and a sub-mask implies the property should be further
* masked according to that sub-mask.
*
* @param {Object} object - The object to mask
* @param {Object<Object|boolean>} mask - The mask to apply to the object
*/
export function maskObject(object, mask) {
return Object.keys(object).reduce((state, key) => {
if (mask[key] === true) {
state[key] = object[key];
} else if (mask[key]) {
state[key] = maskObject(object[key], mask[key]);
}
return state;
}, {});
}
12 changes: 6 additions & 6 deletions test/e2e/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,12 @@ const completeImportSRPOnboardingFlow = async (
tag: 'button',
});

// clicks the "Import Wallet" option
await driver.clickElement({ text: 'Import wallet', tag: 'button' });

// clicks the "No thanks" option on the metametrics opt-in screen
await driver.clickElement('.btn-secondary');

// clicks the "Import Wallet" option
await driver.clickElement({ text: 'Import wallet', tag: 'button' });

// Import Secret Recovery Phrase
await driver.pasteIntoField(
'[data-testid="import-srp__srp-word-0"]',
Expand Down Expand Up @@ -290,12 +290,12 @@ const completeImportSRPOnboardingFlowWordByWord = async (
tag: 'button',
});

// clicks the "Import Wallet" option
await driver.clickElement({ text: 'Import wallet', tag: 'button' });

// clicks the "No thanks" option on the metametrics opt-in screen
await driver.clickElement('.btn-secondary');

// clicks the "Import Wallet" option
await driver.clickElement({ text: 'Import wallet', tag: 'button' });

const words = seedPhrase.split(' ');
for (const word of words) {
await driver.pasteIntoField(
Expand Down
8 changes: 4 additions & 4 deletions test/e2e/metamask-ui.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ describe('MetaMask', function () {
await driver.delay(largeDelayMs);
});

it('clicks the "Create New Wallet" option', async function () {
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });
it('clicks the "No thanks" option on the metametrics opt-in screen', async function () {
await driver.clickElement('.btn-secondary');
await driver.delay(largeDelayMs);
});

it('clicks the "No thanks" option on the metametrics opt-in screen', async function () {
await driver.clickElement('.btn-secondary');
it('clicks the "Create New Wallet" option', async function () {
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });
await driver.delay(largeDelayMs);
});

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/tests/incremental-security.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ describe('Incremental Security', function () {
tag: 'button',
});

// clicks the "Create New Wallet" option
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });

// clicks the "No thanks" option on the metametrics opt-in screen
await driver.clickElement('.btn-secondary');

// clicks the "Create New Wallet" option
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });

// accepts a secure password
await driver.fill(
'.first-time-flow__form #create-password',
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/tests/metamask-responsive-ui.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ describe('MetaMask Responsive UI', function () {
});
await driver.delay(tinyDelayMs);

// clicks the "Create New Wallet" option
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });

// clicks the "I Agree" option on the metametrics opt-in screen
await driver.clickElement('.btn-primary');

// clicks the "Create New Wallet" option
await driver.clickElement({ text: 'Create a Wallet', tag: 'button' });

// accepts a secure password
await driver.fill(
'.first-time-flow__form #create-password',
Expand Down
24 changes: 1 addition & 23 deletions ui/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import browser from 'webextension-polyfill';

import { getEnvironmentType } from '../app/scripts/lib/util';
import { ALERT_TYPES } from '../shared/constants/alerts';
import { maskObject } from '../shared/modules/object.utils';
import { SENTRY_STATE } from '../app/scripts/lib/setupSentry';
import { ENVIRONMENT_TYPE_POPUP } from '../shared/constants/app';
import * as actions from './store/actions';
Expand Down Expand Up @@ -171,29 +172,6 @@ async function startApp(metamaskState, backgroundConnection, opts) {
return store;
}

/**
* Return a "masked" copy of the given object.
*
* The returned object includes only the properties present in the mask. The
* mask is an object that mirrors the structure of the given object, except
* the only values are `true` or a sub-mask. `true` implies the property
* should be included, and a sub-mask implies the property should be further
* masked according to that sub-mask.
*
* @param {Object} object - The object to mask
* @param {Object<Object|boolean>} mask - The mask to apply to the object
*/
function maskObject(object, mask) {
return Object.keys(object).reduce((state, key) => {
if (mask[key] === true) {
state[key] = object[key];
} else if (mask[key]) {
state[key] = maskObject(object[key], mask[key]);
}
return state;
}, {});
}

function setupDebuggingHelpers(store) {
window.getCleanAppState = async function () {
const state = clone(store.getState());
Expand Down
29 changes: 3 additions & 26 deletions ui/pages/first-time-flow/metametrics-opt-in/metametrics-opt-in.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import PropTypes from 'prop-types';
import MetaFoxLogo from '../../../components/ui/metafox-logo';
import PageContainerFooter from '../../../components/ui/page-container/page-container-footer';
import { EVENT } from '../../../../shared/constants/metametrics';
import { INITIALIZE_SELECT_ACTION_ROUTE } from '../../../helpers/constants/routes';

export default class MetaMetricsOptIn extends Component {
static propTypes = {
history: PropTypes.object,
setParticipateInMetaMetrics: PropTypes.func,
nextRoute: PropTypes.string,
firstTimeSelectionMetaMetricsName: PropTypes.string,
participateInMetaMetrics: PropTypes.bool,
};
Expand All @@ -21,7 +21,6 @@ export default class MetaMetricsOptIn extends Component {
render() {
const { trackEvent, t } = this.context;
const {
nextRoute,
history,
setParticipateInMetaMetrics,
firstTimeSelectionMetaMetricsName,
Expand Down Expand Up @@ -105,29 +104,7 @@ export default class MetaMetricsOptIn extends Component {
onCancel={async () => {
await setParticipateInMetaMetrics(false);

try {
if (
participateInMetaMetrics === null ||
participateInMetaMetrics === true
) {
await trackEvent(
{
category: EVENT.CATEGORIES.ONBOARDING,
event: 'Metrics Opt Out',
properties: {
action: 'Metrics Option',
legacy_event: true,
},
},
{
isOptIn: true,
flushImmediately: true,
},
);
}
} finally {
history.push(nextRoute);
}
history.push(INITIALIZE_SELECT_ACTION_ROUTE);
}}
cancelText={t('noThanks')}
hideCancel={false}
Expand Down Expand Up @@ -177,7 +154,7 @@ export default class MetaMetricsOptIn extends Component {
);
await Promise.all(metrics);
} finally {
history.push(nextRoute);
history.push(INITIALIZE_SELECT_ACTION_ROUTE);
}
}}
submitText={t('affirmAgree')}
Expand Down
2 changes: 0 additions & 2 deletions ui/pages/first-time-flow/metametrics-opt-in/metametrics-opt-in.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { connect } from 'react-redux';
import { setParticipateInMetaMetrics } from '../../../store/actions';
import { getFirstTimeFlowTypeRoute } from '../../../selectors';
import MetaMetricsOptIn from './metametrics-opt-in.component';

const firstTimeFlowTypeNameMap = {
Expand All @@ -12,7 +11,6 @@ const mapStateToProps = (state) => {
const { firstTimeFlowType, participateInMetaMetrics } = state.metamask;

return {
nextRoute: getFirstTimeFlowTypeRoute(state),
firstTimeSelectionMetaMetricsName:
firstTimeFlowTypeNameMap[firstTimeFlowType],
participateInMetaMetrics,
Expand Down
9 changes: 6 additions & 3 deletions ui/pages/first-time-flow/select-action/select-action.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import React, { PureComponent } from 'react';
import PropTypes from 'prop-types';
import Button from '../../../components/ui/button';
import MetaFoxLogo from '../../../components/ui/metafox-logo';
import { INITIALIZE_METAMETRICS_OPT_IN_ROUTE } from '../../../helpers/constants/routes';
import {
INITIALIZE_CREATE_PASSWORD_ROUTE,
INITIALIZE_IMPORT_WITH_SEED_PHRASE_ROUTE,
} from '../../../helpers/constants/routes';

export default class SelectAction extends PureComponent {
static propTypes = {
Expand All @@ -26,12 +29,12 @@ export default class SelectAction extends PureComponent {

handleCreate = () => {
this.props.setFirstTimeFlowType('create');
this.props.history.push(INITIALIZE_METAMETRICS_OPT_IN_ROUTE);
this.props.history.push(INITIALIZE_CREATE_PASSWORD_ROUTE);
};

handleImport = () => {
this.props.setFirstTimeFlowType('import');
this.props.history.push(INITIALIZE_METAMETRICS_OPT_IN_ROUTE);
this.props.history.push(INITIALIZE_IMPORT_WITH_SEED_PHRASE_ROUTE);
};

render() {
Expand Down
21 changes: 17 additions & 4 deletions ui/pages/first-time-flow/welcome/welcome.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Button from '../../../components/ui/button';
import {
INITIALIZE_CREATE_PASSWORD_ROUTE,
INITIALIZE_SELECT_ACTION_ROUTE,
INITIALIZE_METAMETRICS_OPT_IN_ROUTE,
} from '../../../helpers/constants/routes';
import { isBeta } from '../../../helpers/utils/build-types';
import WelcomeFooter from './welcome-footer.component';
Expand All @@ -16,6 +17,7 @@ export default class Welcome extends PureComponent {
history: PropTypes.object,
participateInMetaMetrics: PropTypes.bool,
welcomeScreenSeen: PropTypes.bool,
isInitialized: PropTypes.bool,
};

static contextTypes = {
Expand All @@ -29,17 +31,28 @@ export default class Welcome extends PureComponent {
}

componentDidMount() {
const { history, participateInMetaMetrics, welcomeScreenSeen } = this.props;
const {
history,
participateInMetaMetrics,
welcomeScreenSeen,
isInitialized,
} = this.props;

if (welcomeScreenSeen && participateInMetaMetrics !== null) {
if (
welcomeScreenSeen &&
isInitialized &&
participateInMetaMetrics !== null
) {
history.push(INITIALIZE_CREATE_PASSWORD_ROUTE);
} else if (welcomeScreenSeen) {
} else if (welcomeScreenSeen && participateInMetaMetrics !== null) {
history.push(INITIALIZE_SELECT_ACTION_ROUTE);
} else if (welcomeScreenSeen) {
history.push(INITIALIZE_METAMETRICS_OPT_IN_ROUTE);
}
}

handleContinue = () => {
this.props.history.push(INITIALIZE_SELECT_ACTION_ROUTE);
this.props.history.push(INITIALIZE_METAMETRICS_OPT_IN_ROUTE);
};

render() {
Expand Down
7 changes: 6 additions & 1 deletion ui/pages/first-time-flow/welcome/welcome.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@ import { closeWelcomeScreen } from '../../../store/actions';
import Welcome from './welcome.component';

const mapStateToProps = ({ metamask }) => {
const { welcomeScreenSeen, participateInMetaMetrics } = metamask;
const {
welcomeScreenSeen,
participateInMetaMetrics,
isInitialized,
} = metamask;

return {
welcomeScreenSeen,
participateInMetaMetrics,
isInitialized,
};
};

Expand Down
Loading