Set sentry autoTrackSessions default #20132

brad-decker · 2023-07-21T19:42:41Z

Explanation

AutoSessionTracking defaults to true and operates by sending a session packet to sentry. This session packet does not appear to be filtered out via our beforeSend or FilterEvents integration. To avoid sending a request before we have the state tree and can validate the users preferences, we initiate this to false. Later, in startSession and endSession we modify this option and start the session or end the session manually. We also provide a toggleSession method that uses current state to determine whether to start or stop the session.
In sentry-install we call toggleSession after the page loads and state is available, this handles initiating the session for a user who has opted into MetaMetrics. This script is ran in both the background and UI so it should be effective at starting the session in both places.
In the MetaMetricsController the session is manually started or stopped when the user opts in or out of MetaMetrics. This occurs in the setParticipateInMetaMetrics function which is exposed to the UI via the MetaMaskController.
In actions.ts, after successfully calling the setParticipateInMetaMetrics background method, we tell sentry to toggle the session details appropriately.
Types for the global Sentry object, and the new methods added as part of this effort were added to global.d.ts in the types folder.
Another minor change is changing the throwTestError method so that it takes an optional input for the message text so that the Dedupe filter of sentry doesn't catch it for testing purposes. Without debug enabled in the sentry options you are unaware that the message is being dropped by that handler.
Added an INVERSE test case, asserting that requests are NOT sent to sentry when the flag is false.

Screenshots/Screencaps

Before

before.mov

After

after.mov

Manual Testing Steps With Existing Wallet

yarn build dist -- dev mode does not have a sentry global object attached to it. If you want to use dev mode, you need to comment out the 'return undefined' line in setupSentry with the comment block that talks about the bug with sentry in dev mode.
Go to settings -> security and privacy -> toggle participate in metametrics to false
in full screen mode, open devtools and inspect the network tab.
refresh the page and look for requests to sentry. There should be ZERO on this branch and ONE on develop.

The next steps only work on this branch:

toggle metrics back on in settings.
In the console. run window.stateHooks.throwTestError()
See that the error is sent to sentry. (will only work once with no input, pass a string to throwTestError to get another error if you want to. Sentry dedupes the errors).
toggle metrics back off in settings.
in the console run window.stateHooks.throwTestError('foo')
See error is not sent to sentry.

Test requests sent during onboarding

yarn build:test (easier to get to a clean wallet)
reset extension in plugins
open extension for first time.
On the welcome screen, open console and refresh.
On develop: request sent to sentry, on this branch: no request.

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

app/scripts/lib/setupSentry.js

brad-decker · 2023-07-22T19:53:03Z

ui/store/actions.test.js

+  describe('#setParticipateInMetaMetrics', () => {
+    beforeAll(() => {
+      window.sentry = {
+        toggleSession: jest.fn(),
+        endSession: jest.fn(),
+      };
+    });
+    it('sets participateInMetaMetrics to true', async () => {
+      const store = mockStore();
+      const setParticipateInMetaMetricsStub = sinon
+        .stub()
+        .callsFake((_, cb) => cb());
+
+      background.getApi.returns({
+        setParticipateInMetaMetrics: setParticipateInMetaMetricsStub,
+      });
+
+      _setBackgroundConnection(background.getApi());
+
+      await store.dispatch(actions.setParticipateInMetaMetrics(true));
+      expect(setParticipateInMetaMetricsStub.calledOnceWith(true)).toBeTruthy();
+      expect(window.sentry.toggleSession).toHaveBeenCalled();
+    });
+  });


No tests existed for setParticipateInMetaMetrics before. So this minimal test case covers the newly added code plus the happy path of enabling the flag.

codecov · 2023-07-22T20:33:28Z

Codecov Report

Merging #20132 (78b278f) into develop (4927ee7) will decrease coverage by 0.03%.
Report is 1 commits behind head on develop.
The diff coverage is 12.90%.

@@             Coverage Diff             @@
##           develop   #20132      +/-   ##
===========================================
- Coverage    69.48%   69.45%   -0.03%     
===========================================
  Files          984      984              
  Lines        37328    37355      +27     
  Branches     10035    10049      +14     
===========================================
+ Hits         25935    25943       +8     
- Misses       11393    11412      +19

Files Changed	Coverage Δ
app/scripts/lib/setupSentry.js	`44.22% <0.00%> (-8.63%)`	⬇️
ui/index.js	`0.00% <0.00%> (ø)`
app/scripts/controllers/metametrics.js	`79.80% <100.00%> (+0.13%)`	⬆️
ui/store/actions.ts	`42.23% <100.00%> (+0.34%)`	⬆️

metamaskbot · 2023-07-22T20:46:31Z

Builds ready [949f93a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1563 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	110	203	134	20	10
		domContentLoaded	1425	1931	1563	108	52
		load	1426	1931	1563	108	52
		domInteractive	1425	1931	1563	108	52

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.12 KiB (0.03%)
ui: 65 Bytes (0.00%)
common: 888 Bytes (0.02%)

pedronfigueiredo · 2023-07-24T18:03:59Z

app/scripts/lib/setupSentry.js

+     * change may result in the FilterEvents integration no longer being
+     * necessary, but that should be tested and executed in a future PR.
+     *
+     * TODO: Potentially remove the FilterEvents integration.


[nit] If this TODO is outside of this task, can you please create a task for it and link it in this comment?

Sounds good, Pedro!

#20174 -- ill update the comment in a bit

See here: #15677

The FilterEvents integration is used over beforeSend to workaround the incompatibility between beforeSend and Dedupe

Ah, I'll close the issue and remove the change to beforeSend then

The changes to beforeSend were reverted and the comment removed @pedronfigueiredo and @Gudahtt

metamaskbot · 2023-07-25T12:55:44Z

Builds ready [a16a65d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1558 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	106	159	129	15	7
		domContentLoaded	1434	1707	1558	81	39
		load	1434	1707	1558	81	39
		domInteractive	1434	1707	1558	81	39

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.12 KiB (0.03%)
ui: 65 Bytes (0.00%)
common: 888 Bytes (0.02%)

metamaskbot · 2023-07-25T14:07:03Z

Builds ready [9dff5a3]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1664 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	120	325	151	43	21
		domContentLoaded	1501	1886	1663	106	51
		load	1501	1886	1664	107	51
		domInteractive	1501	1886	1663	106	51

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.1 KiB (0.03%)
ui: 65 Bytes (0.00%)
common: 874 Bytes (0.02%)

DDDDDanica · 2023-07-25T15:42:53Z

app/scripts/controllers/metametrics.test.js

@@ -146,6 +146,14 @@ describe('MetaMetricsController', function () {
  const now = new Date();
  let clock;
  beforeEach(function () {
+    globalThis.sentry = {
+      startSession: sinon.fake(() => {


Can we use jest.fn() here to mock?

Not without rewriting the tests to jest. Currently in mocha

app/scripts/lib/setupSentry.js

DDDDDanica · 2023-07-25T16:00:44Z

test/e2e/tests/errors.spec.js

+        await driver.fill('#password', 'correct horse battery staple');
+        await driver.press('#password', driver.Key.ENTER);


Suggested change

await driver.fill('#password', 'correct horse battery staple');

await driver.press('#password', driver.Key.ENTER);

await unlockWallet(driver)

DDDDDanica · 2023-07-25T16:01:27Z

test/e2e/tests/errors.spec.js

+        await driver.press('#password', driver.Key.ENTER);
+        // Trigger error
+        driver.executeScript('window.stateHooks.throwTestError()');
+        driver.delay(3000);


Question: is there a way to bypass the delay here?

No we need to wait sometime to give time for a network request to occur

In the inverse test, we wait for a specific network request. I want to give a similar pause but we can't wait for anything in particular.

DDDDDanica · 2023-07-25T16:02:09Z

ui/store/actions.test.js

+      const setParticipateInMetaMetricsStub = sinon
+        .stub()
+        .callsFake((_, cb) => cb());


can we use jest here as well?

~~Id rather do that as a larger part of fixing the actions test file to remove sinon. I copied patterns used for other background methods here. Do we have an issue to track dropping sinon?~~

Changed my mind after thinking about it. We do need to try and commit to making incremental changes when we do not have the time to do the more extreme work required. This was the right call here and it also helps to set an example for future extensions of a file.

Mmm... i might retract my statement. let me give it a stab and see how it looks

added the jest version and removed the additional sinon usage.

weizman

lgtm

metamaskbot · 2023-07-26T00:32:51Z

Builds ready [78b278f]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1596 ± 53 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	119	308	160	48	23
		domContentLoaded	1436	1917	1596	111	53
		load	1436	1917	1596	111	53
		domInteractive	1436	1915	1596	111	53

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.1 KiB (0.03%)
ui: 65 Bytes (0.00%)
common: 874 Bytes (0.02%)

pedronfigueiredo · 2023-07-26T09:54:20Z

LGTM

brad-decker marked this pull request as ready for review July 22, 2023 19:50

brad-decker requested a review from a team as a code owner July 22, 2023 19:50

brad-decker commented Jul 22, 2023

View reviewed changes

app/scripts/lib/setupSentry.js Outdated Show resolved Hide resolved

brad-decker commented Jul 22, 2023

View reviewed changes

legobeat mentioned this pull request Jul 22, 2023

[Bug]: As a user, I do not have enough control over, nor do I always consent, to what is sent via MetaMetrics, Sentry, APIs, and RPCs #15576

Open

pedronfigueiredo reviewed Jul 24, 2023

View reviewed changes

brad-decker mentioned this pull request Jul 25, 2023

Explore necessity of custom Filter Integration for Sentry #20174

Closed

brad-decker added 6 commits July 25, 2023 08:29

Set sentry autoTrackSessions default

fb77780

endSession....

ff82a23

fixup

400512a

updated comment

87b4dd4

prevent breaking devmode

3c75a8e

remove changes to beforeSend

9dff5a3

brad-decker force-pushed the controlled-sessions branch from a16a65d to 9dff5a3 Compare July 25, 2023 13:30

DDDDDanica reviewed Jul 25, 2023

View reviewed changes

remove additional usage of sinon

d334403

DDDDDanica approved these changes Jul 25, 2023

View reviewed changes

weizman approved these changes Jul 25, 2023

View reviewed changes

brad-decker added 2 commits July 25, 2023 16:23

Merge branch 'develop' into controlled-sessions

07dda82

Merge branch 'develop' into controlled-sessions

78b278f

pedronfigueiredo approved these changes Jul 26, 2023

View reviewed changes

brad-decker merged commit e28db07 into develop Jul 26, 2023

brad-decker deleted the controlled-sessions branch July 26, 2023 12:13

github-actions bot locked and limited conversation to collaborators Jul 26, 2023

metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Jul 26, 2023

Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set sentry autoTrackSessions default #20132

Set sentry autoTrackSessions default #20132

brad-decker commented Jul 21, 2023 •

edited

Loading

brad-decker Jul 22, 2023

codecov bot commented Jul 22, 2023 •

edited

Loading

metamaskbot commented Jul 22, 2023

pedronfigueiredo Jul 24, 2023

brad-decker Jul 24, 2023

brad-decker Jul 25, 2023

Gudahtt Jul 25, 2023

brad-decker Jul 25, 2023

brad-decker Jul 25, 2023

metamaskbot commented Jul 25, 2023

metamaskbot commented Jul 25, 2023

DDDDDanica Jul 25, 2023

brad-decker Jul 25, 2023

DDDDDanica Jul 25, 2023

DDDDDanica Jul 25, 2023

brad-decker Jul 25, 2023

brad-decker Jul 25, 2023

DDDDDanica Jul 25, 2023

brad-decker Jul 25, 2023 •

edited

Loading

brad-decker Jul 25, 2023

brad-decker Jul 25, 2023

weizman left a comment

metamaskbot commented Jul 26, 2023

pedronfigueiredo commented Jul 26, 2023

		await driver.fill('#password', 'correct horse battery staple');
		await driver.press('#password', driver.Key.ENTER);

	await driver.fill('#password', 'correct horse battery staple');
	await driver.press('#password', driver.Key.ENTER);
	await unlockWallet(driver)

Set sentry autoTrackSessions default #20132

Set sentry autoTrackSessions default #20132

Conversation

brad-decker commented Jul 21, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps With Existing Wallet

Test requests sent during onboarding

Pre-merge author checklist

Pre-merge reviewer checklist

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2023 • edited Loading

Codecov Report

metamaskbot commented Jul 22, 2023

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

metamaskbot commented Jul 25, 2023

metamaskbot commented Jul 25, 2023

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

brad-decker Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizman left a comment

Choose a reason for hiding this comment

metamaskbot commented Jul 26, 2023

pedronfigueiredo commented Jul 26, 2023

brad-decker commented Jul 21, 2023 •

edited

Loading

codecov bot commented Jul 22, 2023 •

edited

Loading

brad-decker Jul 25, 2023 •

edited

Loading