Skip to content

Commit d8beaa0

Browse files
mydeamydea
authored
ref(replay): Use rrweb for slow click detection (#9408)
This refactors the slow/rage click detection to use rrweb events instead of adding our own event listeners. * We look for incremental snapshot events, and update the `_lastMutation` and `_lastScroll` properties based on them. * If a full snapshot happens, we interpret it as a mutation. this is not necessarily correct, but we don't know any different. also this shouldn't matter much because a full snapshot only happens in error mode, in which case things that happened before the full checkout should be discarded anyhow, so 🤷 Integration tests for this are still passing, so it seems to me to be a good change. Hopefully this: 1. Decreases perf overhead by only using a single instrumentation instead of multiple overlapping ones 2. Increases consistency by ensuring that whatever we see changing in the replay will also be considered the same way for slow/rage click detection.
1 parent f796d50 commit d8beaa0

File tree

8 files changed

+295
-39
lines changed

8 files changed

+295
-39
lines changed

‎packages/browser-integration-tests/suites/replay/slowClick/error/init.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = new Sentry.Replay({
5+
flushMinDelay: 200,
6+
flushMaxDelay: 200,
7+
minReplayDuration: 0,
8+
slowClickTimeout: 3500,
9+
});
10+
11+
Sentry.init({
12+
dsn: 'https://[email protected]/1337',
13+
sampleRate: 1,
14+
replaysSessionSampleRate: 0.0,
15+
replaysOnErrorSampleRate: 1.0,
16+
17+
integrations: [window.Replay],
18+
});

‎packages/browser-integration-tests/suites/replay/slowClick/error/template.html‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button id="buttonError">Trigger error</button>
8+
<button id="buttonErrorMutation">Trigger error</button>
9+
10+
<script>
11+
document.getElementById('buttonError').addEventListener('click', () => {
12+
throw new Error('test error happened');
13+
});
14+
15+
document.getElementById('buttonErrorMutation').addEventListener('click', () => {
16+
document.getElementById('buttonErrorMutation').innerText = 'Test error happened!';
17+
18+
throw new Error('test error happened');
19+
});
20+
</script>
21+
</body>
22+
</html>

‎packages/browser-integration-tests/suites/replay/slowClick/error/test.ts‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import {
5+
getCustomRecordingEvents,
6+
getReplayEventFromRequest,
7+
shouldSkipReplayTest,
8+
waitForReplayRequest,
9+
} from '../../../../utils/replayHelpers';
10+
11+
sentryTest('slow click that triggers error is captured', async ({ getLocalTestUrl, page }) => {
12+
if (shouldSkipReplayTest()) {
13+
sentryTest.skip();
14+
}
15+
16+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
17+
return route.fulfill({
18+
status: 200,
19+
contentType: 'application/json',
20+
body: JSON.stringify({ id: 'test-id' }),
21+
});
22+
});
23+
24+
const url = await getLocalTestUrl({ testDir: __dirname });
25+
26+
await page.goto(url);
27+
28+
const [req0] = await Promise.all([
29+
waitForReplayRequest(page, (_event, res) => {
30+
const { breadcrumbs } = getCustomRecordingEvents(res);
31+
32+
return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected');
33+
}),
34+
page.click('#buttonError'),
35+
]);
36+
37+
const { breadcrumbs } = getCustomRecordingEvents(req0);
38+
39+
const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected');
40+
41+
expect(slowClickBreadcrumbs).toEqual([
42+
{
43+
category: 'ui.slowClickDetected',
44+
type: 'default',
45+
data: {
46+
endReason: 'timeout',
47+
clickCount: 1,
48+
node: {
49+
attributes: {
50+
id: 'buttonError',
51+
},
52+
id: expect.any(Number),
53+
tagName: 'button',
54+
textContent: '******* *****',
55+
},
56+
nodeId: expect.any(Number),
57+
timeAfterClickMs: 3500,
58+
url: 'http://sentry-test.io/index.html',
59+
},
60+
message: 'body > button#buttonError',
61+
timestamp: expect.any(Number),
62+
},
63+
]);
64+
});
65+
66+
sentryTest(
67+
'click that triggers error & mutation is not captured',
68+
async ({ getLocalTestUrl, page, forceFlushReplay }) => {
69+
if (shouldSkipReplayTest()) {
70+
sentryTest.skip();
71+
}
72+
73+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
74+
return route.fulfill({
75+
status: 200,
76+
contentType: 'application/json',
77+
body: JSON.stringify({ id: 'test-id' }),
78+
});
79+
});
80+
81+
const url = await getLocalTestUrl({ testDir: __dirname });
82+
83+
await page.goto(url);
84+
85+
let slowClickCount = 0;
86+
87+
page.on('response', res => {
88+
const req = res.request();
89+
90+
const event = getReplayEventFromRequest(req);
91+
92+
if (!event) {
93+
return;
94+
}
95+
96+
const { breadcrumbs } = getCustomRecordingEvents(res);
97+
98+
const slowClicks = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected');
99+
slowClickCount += slowClicks.length;
100+
});
101+
102+
const [req1] = await Promise.all([
103+
waitForReplayRequest(page, (_event, res) => {
104+
const { breadcrumbs } = getCustomRecordingEvents(res);
105+
106+
return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click');
107+
}),
108+
page.click('#buttonErrorMutation'),
109+
]);
110+
111+
const { breadcrumbs } = getCustomRecordingEvents(req1);
112+
113+
expect(breadcrumbs).toEqual([
114+
{
115+
category: 'ui.click',
116+
data: {
117+
node: {
118+
attributes: {
119+
id: 'buttonErrorMutation',
120+
},
121+
id: expect.any(Number),
122+
tagName: 'button',
123+
textContent: '******* *****',
124+
},
125+
nodeId: expect.any(Number),
126+
},
127+
message: 'body > button#buttonErrorMutation',
128+
timestamp: expect.any(Number),
129+
type: 'default',
130+
},
131+
]);
132+
133+
// Ensure we wait for timeout, to make sure no slow click is created
134+
// Waiting for 3500 + 1s rounding room
135+
await new Promise(resolve => setTimeout(resolve, 4500));
136+
await forceFlushReplay();
137+
138+
expect(slowClickCount).toBe(0);
139+
},
140+
);

‎packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,17 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush
141141
await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]);
142142
await forceFlushReplay();
143143

144+
let slowClickCount = 0;
145+
146+
page.on('response', res => {
147+
const { breadcrumbs } = getCustomRecordingEvents(res);
148+
149+
const slowClicks = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected');
150+
slowClickCount += slowClicks.length;
151+
});
152+
144153
const [req1] = await Promise.all([
145-
waitForReplayRequest(page, (event, res) => {
154+
waitForReplayRequest(page, (_event, res) => {
146155
const { breadcrumbs } = getCustomRecordingEvents(res);
147156

148157
return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click');
@@ -171,6 +180,13 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush
171180
type: 'default',
172181
},
173182
]);
183+
184+
// Ensure we wait for timeout, to make sure no slow click is created
185+
// Waiting for 3500 + 1s rounding room
186+
await new Promise(resolve => setTimeout(resolve, 4500));
187+
await forceFlushReplay();
188+
189+
expect(slowClickCount).toBe(0);
174190
});
175191

176192
sentryTest('inline click handler does not trigger slow click', async ({ forceFlushReplay, getLocalTestUrl, page }) => {

‎packages/replay/src/coreHandlers/handleClick.ts‎

Lines changed: 78 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
1+
import { IncrementalSource, MouseInteractions, record } from '@sentry-internal/rrweb';
12
import type { Breadcrumb } from '@sentry/types';
23

34
import { WINDOW } from '../constants';
45
import type {
6+
RecordingEvent,
57
ReplayClickDetector,
68
ReplayContainer,
79
ReplayMultiClickFrame,
810
ReplaySlowClickFrame,
911
SlowClickConfig,
1012
} from '../types';
13+
import { ReplayEventTypeIncrementalSnapshot } from '../types';
1114
import { timestampToS } from '../util/timestamp';
1215
import { addBreadcrumbEvent } from './util/addBreadcrumbEvent';
13-
import { getClickTargetNode } from './util/domUtils';
16+
import { getClosestInteractive } from './util/domUtils';
1417
import { onWindowOpen } from './util/onWindowOpen';
1518

1619
type ClickBreadcrumb = Breadcrumb & {
@@ -26,6 +29,16 @@ interface Click {
2629
node: HTMLElement;
2730
}
2831

32+
type IncrementalRecordingEvent = RecordingEvent & {
33+
type: typeof ReplayEventTypeIncrementalSnapshot;
34+
data: { source: IncrementalSource };
35+
};
36+
37+
type IncrementalMouseInteractionRecordingEvent = IncrementalRecordingEvent & {
38+
type: typeof ReplayEventTypeIncrementalSnapshot;
39+
data: { type: MouseInteractions; id: number };
40+
};
41+
2942
/** Handle a click. */
3043
export function handleClick(clickDetector: ReplayClickDetector, clickBreadcrumb: Breadcrumb, node: HTMLElement): void {
3144
clickDetector.handleClick(clickBreadcrumb, node);
@@ -70,48 +83,14 @@ export class ClickDetector implements ReplayClickDetector {
7083

7184
/** Register click detection handlers on mutation or scroll. */
7285
public addListeners(): void {
73-
const mutationHandler = (): void => {
74-
this._lastMutation = nowInSeconds();
75-
};
76-
77-
const scrollHandler = (): void => {
78-
this._lastScroll = nowInSeconds();
79-
};
80-
8186
const cleanupWindowOpen = onWindowOpen(() => {
8287
// Treat window.open as mutation
8388
this._lastMutation = nowInSeconds();
8489
});
8590

86-
const clickHandler = (event: MouseEvent): void => {
87-
if (!event.target) {
88-
return;
89-
}
90-
91-
const node = getClickTargetNode(event);
92-
if (node) {
93-
this._handleMultiClick(node as HTMLElement);
94-
}
95-
};
96-
97-
const obs = new MutationObserver(mutationHandler);
98-
99-
obs.observe(WINDOW.document.documentElement, {
100-
attributes: true,
101-
characterData: true,
102-
childList: true,
103-
subtree: true,
104-
});
105-
106-
WINDOW.addEventListener('scroll', scrollHandler, { passive: true });
107-
WINDOW.addEventListener('click', clickHandler, { passive: true });
108-
10991
this._teardown = () => {
110-
WINDOW.removeEventListener('scroll', scrollHandler);
111-
WINDOW.removeEventListener('click', clickHandler);
11292
cleanupWindowOpen();
11393

114-
obs.disconnect();
11594
this._clicks = [];
11695
this._lastMutation = 0;
11796
this._lastScroll = 0;
@@ -129,7 +108,7 @@ export class ClickDetector implements ReplayClickDetector {
129108
}
130109
}
131110

132-
/** Handle a click */
111+
/** @inheritDoc */
133112
public handleClick(breadcrumb: Breadcrumb, node: HTMLElement): void {
134113
if (ignoreElement(node, this._ignoreSelector) || !isClickBreadcrumb(breadcrumb)) {
135114
return;
@@ -158,6 +137,22 @@ export class ClickDetector implements ReplayClickDetector {
158137
}
159138
}
160139

140+
/** @inheritDoc */
141+
public registerMutation(timestamp = Date.now()): void {
142+
this._lastMutation = timestampToS(timestamp);
143+
}
144+
145+
/** @inheritDoc */
146+
public registerScroll(timestamp = Date.now()): void {
147+
this._lastScroll = timestampToS(timestamp);
148+
}
149+
150+
/** @inheritDoc */
151+
public registerClick(element: HTMLElement): void {
152+
const node = getClosestInteractive(element);
153+
this._handleMultiClick(node as HTMLElement);
154+
}
155+
161156
/** Count multiple clicks on elements. */
162157
private _handleMultiClick(node: HTMLElement): void {
163158
this._getClicks(node).forEach(click => {
@@ -311,3 +306,50 @@ function isClickBreadcrumb(breadcrumb: Breadcrumb): breadcrumb is ClickBreadcrum
311306
function nowInSeconds(): number {
312307
return Date.now() / 1000;
313308
}
309+
310+
/** Update the click detector based on a recording event of rrweb. */
311+
export function updateClickDetectorForRecordingEvent(clickDetector: ReplayClickDetector, event: RecordingEvent): void {
312+
try {
313+
// note: We only consider incremental snapshots here
314+
// This means that any full snapshot is ignored for mutation detection - the reason is that we simply cannot know if a mutation happened here.
315+
// E.g. think that we are buffering, an error happens and we take a full snapshot because we switched to session mode -
316+
// in this scenario, we would not know if a dead click happened because of the error, which is a key dead click scenario.
317+
// Instead, by ignoring full snapshots, we have the risk that we generate a false positive
318+
// (if a mutation _did_ happen but was "swallowed" by the full snapshot)
319+
// But this should be more unlikely as we'd generally capture the incremental snapshot right away
320+
321+
if (!isIncrementalEvent(event)) {
322+
return;
323+
}
324+
325+
const { source } = event.data;
326+
if (source === IncrementalSource.Mutation) {
327+
clickDetector.registerMutation(event.timestamp);
328+
}
329+
330+
if (source === IncrementalSource.Scroll) {
331+
clickDetector.registerScroll(event.timestamp);
332+
}
333+
334+
if (isIncrementalMouseInteraction(event)) {
335+
const { type, id } = event.data;
336+
const node = record.mirror.getNode(id);
337+
338+
if (node instanceof HTMLElement && type === MouseInteractions.Click) {
339+
clickDetector.registerClick(node);
340+
}
341+
}
342+
} catch {
343+
// ignore errors here, e.g. if accessing something that does not exist
344+
}
345+
}
346+
347+
function isIncrementalEvent(event: RecordingEvent): event is IncrementalRecordingEvent {
348+
return event.type === ReplayEventTypeIncrementalSnapshot;
349+
}
350+
351+
function isIncrementalMouseInteraction(
352+
event: IncrementalRecordingEvent,
353+
): event is IncrementalMouseInteractionRecordingEvent {
354+
return event.data.source === IncrementalSource.MouseInteraction;
355+
}

0 commit comments

Comments
 (0)