test(replay): Add tests for session expiration #7253

mydea · 2023-02-22T14:22:13Z

This PR makes the session expiration overwritable, so we can test this without waiting for 30/60 mins 😜

I added three tests:

Test idle duration results in a new session
Test max session life is considered and results in a new session --> Extracted out, into a new follow up PR
Test inactivity leads to pausing/restarting the same session

github-actions · 2023-02-22T14:29:19Z

Replay SDK metrics 🚀

		Plain	+Sentry			+Replay
	Revision	Value	Value	Diff	Ratio	Value	Diff	Ratio
LCP	This PR `9c2c018`	74.49 ms	101.27 ms	+26.78 ms	+35.95 %	127.19 ms	+52.70 ms	+70.75 %
LCP	Previous `e60cd02`	95.19 ms	116.88 ms	+21.69 ms	+22.79 %	151.44 ms	+56.25 ms	+59.09 %
CLS	This PR `9c2c018`	0.06 ms	0.06 ms	-0.00 ms	-0.67 %	0.06 ms	-0.00 ms	-0.25 %
CLS	Previous `e60cd02`	0.06 ms	0.06 ms	-0.00 ms	-0.45 %	0.06 ms	-0.00 ms	-0.37 %
CPU	This PR `9c2c018`	12.98 %	12.43 %	-0.55 pp	-4.27 %	18.06 %	+5.07 pp	+39.06 %
CPU	Previous `e60cd02`	18.74 %	17.90 %	-0.85 pp	-4.52 %	25.06 %	+6.32 pp	+33.70 %
JS heap avg	This PR `9c2c018`	1.94 MB	1.99 MB	+45.67 kB	+2.35 %	2.87 MB	+928.58 kB	+47.82 %
JS heap avg	Previous `e60cd02`	1.94 MB	2 MB	+62.53 kB	+3.22 %	2.87 MB	+927.44 kB	+47.82 %
JS heap max	This PR `9c2c018`	2.3 MB	2.56 MB	+256.44 kB	+11.14 %	3.35 MB	+1.05 MB	+45.60 %
JS heap max	Previous `e60cd02`	2.3 MB	2.57 MB	+265.21 kB	+11.52 %	3.37 MB	+1.06 MB	+46.20 %
netTx	This PR `9c2c018`	0 B	0 B	0 B	n/a	2.21 kB	+2.21 kB	n/a
netTx	Previous `e60cd02`	0 B	0 B	0 B	n/a	2.21 kB	+2.21 kB	n/a
netRx	This PR `9c2c018`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netRx	Previous `e60cd02`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netCount	This PR `9c2c018`	0	0	0	n/a	1	+1	n/a
netCount	Previous `e60cd02`	0	0	0	n/a	1	+1	n/a
netTime	This PR `9c2c018`	0.00 ms	0.00 ms	0.00 ms	n/a	62.25 ms	+62.25 ms	n/a
netTime	Previous `e60cd02`	0.00 ms	0.00 ms	0.00 ms	n/a	117.55 ms	+117.55 ms	n/a

Previous results on branch: `develop`

Revision	LCP	CLS	CPU	JS heap avg	JS heap max	netTx	netRx	netCount	netTime
`e60cd02`	+56.25 ms	-0.00 ms	+6.32 pp	+927.44 kB	+1.06 MB	+2.21 kB	+41 B	+1	+117.55 ms
`e25c067`	+48.34 ms	+0.00 ms	+5.59 pp	+926.37 kB	+1.05 MB	+2.22 kB	+41 B	+1	+65.23 ms
`b1b249b`	+43.88 ms	+0.00 ms	+4.80 pp	+937.99 kB	+1.05 MB	+2.22 kB	+41 B	+1	+111.56 ms
`12e34d4`	+28.57 ms	+0.00 ms	+5.77 pp	+930.12 kB	+1.04 MB	+2.26 kB	+41 B	+1	+109.67 ms
`c46c56c`	+65.45 ms	-0.00 ms	+5.38 pp	+930.26 kB	+1.07 MB	+2.21 kB	+41 B	+1	+91.29 ms
`7f4c4ec`	+56.64 ms	-0.00 ms	+5.57 pp	+927.42 kB	+1.06 MB	+2.21 kB	+41 B	+1	+110.83 ms
`00d2360`	+55.18 ms	+0.00 ms	+2.23 pp	+934.14 kB	+1.05 MB	+2.22 kB	+41 B	+1	+71.65 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Wed, 22 Feb 2023 15:46:50 GMT

github-actions · 2023-02-22T14:34:24Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.12 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	62.52 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.75 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	55.51 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.49 KB (0%)
@sentry/browser - Webpack (minified)	66.98 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.52 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	48.2 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	27.21 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.46 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	42.92 KB (+0.12% 🔺)
@sentry/replay - Webpack (gzipped + minified)	36.97 KB (+0.12% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	60.66 KB (+0.08% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	54.05 KB (+0.1% 🔺)

Lms24

I had some questions but overall LGTM. As I said I don't wanna block you too long with this so feel free to go with what we have right now.

rollup/plugins/bundlePlugins.js

packages/integration-tests/suites/replay/sessionExpiry/test.ts

Lms24 · 2023-02-22T15:12:49Z

packages/integration-tests/suites/replay/sessionExpiry/test.ts

+  expect(incrementalSnapshots1).toEqual(
+    expect.arrayContaining([
+      {
+        source: 1,
+        positions: [
+          {
+            id: 9,
+            timeOffset: expect.any(Number),
+            x: expect.any(Number),
+            y: expect.any(Number),
+          },
+        ],
+      },
+    ]),
+  );


I'm not entirely sure what this snapshot element represents - should we add a comment what we're checking here?
Also related but no action required: Any specific reason why we're checking this explicitly over using toMatchSnapshot? (not saying this needs to change)

the reason was that I didn't want to test the exact x/y values as I figured they may be flaky - and the toMatchSnapshot stuff does string comparison, basically 😬

I'll add a comment - not sure if this actually helps us at all, my thought was to test that something reasonable is sent, but since it is sadly not stable between browsers, it's pretty diluted now 😅

Yeah, I also noticed that incremental snapshots are quite browser-specific... In the errors tests, I opted to check for "length of the incr. snapshot > SomeLowerBoundIObservedDuringTestFlakesMinusOne" 😅

RE flakes: My "replay across multiple pages" test snapshot-tests incr. snapshots and so far I haven't noticed flakes (🤞). Perhaps it's still worth a shot? Although, I'm only testing on chromium in this test...

Jup, I suspect the reason it works well there is that it's chromium only 😅

Lms24 · 2023-02-22T15:21:00Z

packages/integration-tests/suites/replay/sessionInactive/test.ts

+  const replay = await getReplaySnapshot(page);
+  // @ts-ignore private api
+  expect(replay._isEnabled).toEqual(true);
+  // @ts-ignore private api
+  expect(replay._isPaused).toEqual(false);
+
+  // Now we trigger a blur event, which should move the session to paused mode
+  await page.evaluate(() => {
+    window.dispatchEvent(new Event('blur'));
+  });
+
+  const replay2 = await getReplaySnapshot(page);
+  // @ts-ignore private api
+  expect(replay2._isEnabled).toEqual(true);
+  // @ts-ignore private api
+  expect(replay2._isPaused).toEqual(true);
+
+  // Trigger an action, should re-start the recording
+  await page.click('#button2');
+
+  const replay3 = await getReplaySnapshot(page);
+  // @ts-ignore private api
+  expect(replay3._isEnabled).toEqual(true);
+  // @ts-ignore private api
+  expect(replay3._isPaused).toEqual(false);
+


I know we already have such tests but I'm not sure if we want to add more of them: IMO we're inspecting intrinsics too much for an integration test by checking the internal state of the ReplayContainer.
That being said, I get that it's hard to check that nothing happens in this situation otherwise. So my ramble is probably pointless 😅
IIRC we already have some unit integration tests for paused replays. Any chance we could use/adjust them instead?

But I don't want to block this too long so if there's no feasible option, let's just roll with this test instead!

Yeah, I totally get you. I mean we do have some unitgration (xD) tests for this, so we may also skip this here - I figured this is a bit more "real" as it actually waits the correct time and we have less/no mocking/overwriting of stuff. IMHO it's "OK" to have this for now, but we should keep an eye on this so this doesn't go overboard 😅

unitgration

I love it 😅

Sure, sounds fair - let's just try to minimize using this in the future whenever there's another possiblilty.

packages/replay/src/constants.ts

billyvg · 2023-02-22T16:20:41Z

packages/replay/src/replay.ts

+  public timeouts = {
+    sessionIdle: SESSION_IDLE_DURATION,
+    maxSessionLife: MAX_SESSION_LIFE,
+  };


Is there a way we only allow writes to these timeouts during tests? I worry some crafty people may change these values to something unsupported.

you'd have to reach into a private object with no docs whatsover 😅 (e.g. replay._replay.timeouts). I'll add @hidden, actually. Other than this, not sure 😅 but at this point people could be overwriting anything in there basically xD

Could we make this into getter/setter and have the setters check for an env var that's set during test? Maybe you're right though it's probably overkill

IMHO if we do that users can still overwrite it as well if they really want 😅 and we'd have to look this up on window etc. which IMHO would also be a bit weird, so I'd leave it like this for now. If we find this is abused we can still change it later, I'd say.

I've made this readonly & const, hopefully making it clear that this cannot/should not be overwritten!

mydea · 2023-03-06T16:24:14Z

So, finally I extracted the one new test that keeps flaking out, will make a new PR for this specific test so we can finally merge this at least 🙈

mydea added Type: Tests Package: replay Issues related to the Sentry Replay SDK labels Feb 22, 2023

mydea requested review from billyvg and Lms24 February 22, 2023 14:22

mydea self-assigned this Feb 22, 2023

mydea mentioned this pull request Feb 22, 2023

Improve Replay Playwright Integration Tests #7044

Closed

Lms24 approved these changes Feb 22, 2023

View reviewed changes

mydea force-pushed the fn/session-timeout-config branch from c87d7e9 to 1431a32 Compare February 22, 2023 15:33

billyvg reviewed Feb 22, 2023

View reviewed changes

mydea force-pushed the fn/session-timeout-config branch from bb75b72 to cbd0c97 Compare February 27, 2023 07:50

mydea mentioned this pull request Feb 28, 2023

ref(replay): Use SESSION_IDLE_DURATION instead of VISIBILITY_CHANGE_TIMEOUT #7297

Merged

mydea force-pushed the fn/session-timeout-config branch 9 times, most recently from 004549a to 86746b5 Compare March 6, 2023 16:23

test(replay): Add tests for session expiration

9168095

mydea force-pushed the fn/session-timeout-config branch from 86746b5 to 9168095 Compare March 7, 2023 07:44

mydea merged commit af48921 into develop Mar 7, 2023

mydea deleted the fn/session-timeout-config branch March 7, 2023 09:02

Uh oh!

test(replay): Add tests for session expiration #7253

test(replay): Add tests for session expiration #7253

Uh oh!

Conversation

mydea commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Replay SDK metrics 🚀

Previous results on branch: develop

Uh oh!

github-actions bot commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Lms24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Mar 6, 2023

Uh oh!

Uh oh!

mydea commented Feb 22, 2023 •

edited

Loading

github-actions bot commented Feb 22, 2023 •

edited

Loading

Previous results on branch: `develop`

github-actions bot commented Feb 22, 2023 •

edited

Loading

Lms24 left a comment •

edited

Loading