Skip to content
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.

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

fix(replay): Adjust slow/multi click handling #8380

Merged
merged 3 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
import {
getCustomRecordingEvents,
shouldSkipReplayTest,
waitForReplayRequest,
waitForReplayRequests,
} from '../../../../utils/replayHelpers';

sentryTest('captures multi click when not detecting slow click', async ({ getLocalTestUrl, page }) => {
if (shouldSkipReplayTest()) {
Expand Down Expand Up @@ -58,3 +63,97 @@ sentryTest('captures multi click when not detecting slow click', async ({ getLoc
},
]);
});

sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page, forceFlushReplay }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

const reqPromise0 = waitForReplayRequest(page, 0);

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ id: 'test-id' }),
});
});

const url = await getLocalTestUrl({ testDir: __dirname });

await page.goto(url);
await reqPromise0;

let multiClickBreadcrumbCount = 0;

const reqsPromise = waitForReplayRequests(page, (_event, res) => {
const { breadcrumbs } = getCustomRecordingEvents(res);
const count = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick').length;

multiClickBreadcrumbCount += count;

if (multiClickBreadcrumbCount === 2) {
return true;
}

return false;
});

await page.click('#mutationButtonImmediately', { clickCount: 4 });
await forceFlushReplay();

// Ensure we waited at least 1s, which is the threshold to create a new ui.click breadcrumb
await new Promise(resolve => setTimeout(resolve, 1001));

await page.click('#mutationButtonImmediately', { clickCount: 2 });
await forceFlushReplay();

const responses = await reqsPromise;

const slowClickBreadcrumbs = responses
.flatMap(res => getCustomRecordingEvents(res).breadcrumbs)
.filter(breadcrumb => breadcrumb.category === 'ui.multiClick');

expect(slowClickBreadcrumbs).toEqual([
{
category: 'ui.multiClick',
type: 'default',
data: {
clickCount: 6,
metric: true,
node: {
attributes: {
id: 'mutationButtonImmediately',
},
id: expect.any(Number),
tagName: 'button',
textContent: '******* ******** ***********',
},
nodeId: expect.any(Number),
url: 'http://sentry-test.io/index.html',
},
message: 'body > button#mutationButtonImmediately',
timestamp: expect.any(Number),
},
{
category: 'ui.multiClick',
type: 'default',
data: {
clickCount: 2,
metric: true,
node: {
attributes: {
id: 'mutationButtonImmediately',
},
id: expect.any(Number),
tagName: 'button',
textContent: '******* ******** ***********',
},
nodeId: expect.any(Number),
url: 'http://sentry-test.io/index.html',
},
message: 'body > button#mutationButtonImmediately',
timestamp: expect.any(Number),
},
]);
});
40 changes: 40 additions & 0 deletions packages/browser-integration-tests/utils/replayHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,46 @@ export function waitForReplayRequest(
);
}

/**
* Wait until a callback returns true, collecting all replay responses along the way.
* This can be useful when you don't know if stuff will be in one or multiple replay requests.
*/
export function waitForReplayRequests(
page: Page,
callback: (event: ReplayEvent, res: Response) => boolean,
timeout?: number,
): Promise<Response[]> {
const responses: Response[] = [];

return new Promise<Response[]>(resolve => {
void page.waitForResponse(
res => {
const req = res.request();

const event = getReplayEventFromRequest(req);

if (!event) {
return false;
}

responses.push(res);

try {
if (callback(event, res)) {
resolve(responses);
return true;
}

return false;
} catch {
return false;
}
},
timeout ? { timeout } : undefined,
);
});
}

export function isReplayEvent(event: Event): event is ReplayEvent {
return event.type === 'replay_event';
}
Expand Down
2 changes: 0 additions & 2 deletions packages/replay/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ export const CONSOLE_ARG_MAX_SIZE = 5_000;
export const SLOW_CLICK_THRESHOLD = 3_000;
/* For scroll actions after a click, we only look for a very short time period to detect programmatic scrolling. */
export const SLOW_CLICK_SCROLL_TIMEOUT = 300;
/* Clicks in this time period are considered e.g. double/triple clicks. */
export const MULTI_CLICK_TIMEOUT = 1_000;

/** When encountering a total segment size exceeding this size, stop the replay (as we cannot properly ingest it). */
export const REPLAY_MAX_EVENT_BUFFER_SIZE = 20_000_000; // ~20MB
40 changes: 10 additions & 30 deletions packages/replay/src/coreHandlers/handleClick.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export class ClickDetector implements ReplayClickDetector {
private _clicks: Click[] = [];
private _teardown: undefined | (() => void);

private _multiClickTimeout: number;
private _threshold: number;
private _scollTimeout: number;
private _timeout: number;
Expand All @@ -51,7 +50,6 @@ export class ClickDetector implements ReplayClickDetector {
) {
// We want everything in s, but options are in ms
this._timeout = slowClickConfig.timeout / 1000;
this._multiClickTimeout = slowClickConfig.multiClickTimeout / 1000;
this._threshold = slowClickConfig.threshold / 1000;
this._scollTimeout = slowClickConfig.scrollTimeout / 1000;
this._replay = replay;
Expand Down Expand Up @@ -126,13 +124,6 @@ export class ClickDetector implements ReplayClickDetector {
return;
}

const click = this._getClick(node);

if (click) {
// this means a click on the same element was captured in the last 1s, so we consider this a multi click
return;
}

const newClick: Click = {
timestamp: breadcrumb.timestamp,
clickBreadcrumb: breadcrumb,
Expand All @@ -150,22 +141,14 @@ export class ClickDetector implements ReplayClickDetector {

/** Count multiple clicks on elements. */
private _handleMultiClick(node: HTMLElement): void {
const click = this._getClick(node);

if (!click) {
return;
}

click.clickCount++;
this._getClicks(node).forEach(click => {
click.clickCount++;
});
}

/** Try to get an existing click on the given element. */
private _getClick(node: HTMLElement): Click | undefined {
const now = nowInSeconds();

// Find any click on the same element in the last second
// If one exists, we consider this click as a double/triple/etc click
return this._clicks.find(click => click.node === node && now - click.timestamp < this._multiClickTimeout);
/** Get all pending clicks for a given node. */
private _getClicks(node: HTMLElement): Click[] {
return this._clicks.filter(click => click.node === node);
}

/** Check the clicks that happened. */
Expand All @@ -182,13 +165,6 @@ export class ClickDetector implements ReplayClickDetector {
click.scrollAfter = click.timestamp <= this._lastScroll ? this._lastScroll - click.timestamp : undefined;
}

// If an action happens after the multi click threshold, we can skip waiting and handle the click right away
const actionTime = click.scrollAfter || click.mutationAfter || 0;
if (actionTime && actionTime >= this._multiClickTimeout) {
timedOutClicks.push(click);
return;
}

if (click.timestamp + this._timeout <= now) {
timedOutClicks.push(click);
}
Expand Down Expand Up @@ -269,6 +245,10 @@ export class ClickDetector implements ReplayClickDetector {

/** Schedule to check current clicks. */
private _scheduleCheckClicks(): void {
if (this._checkClickTimeout) {
clearTimeout(this._checkClickTimeout);
}

this._checkClickTimeout = setTimeout(() => this._checkClicks(), 1000);
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/replay/src/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { logger } from '@sentry/utils';
import {
BUFFER_CHECKOUT_TIME,
MAX_SESSION_LIFE,
MULTI_CLICK_TIMEOUT,
SESSION_IDLE_EXPIRE_DURATION,
SESSION_IDLE_PAUSE_DURATION,
SLOW_CLICK_SCROLL_TIMEOUT,
Expand Down Expand Up @@ -175,7 +174,6 @@ export class ReplayContainer implements ReplayContainerInterface {
timeout: slowClickTimeout,
scrollTimeout: SLOW_CLICK_SCROLL_TIMEOUT,
ignoreSelector: slowClickIgnoreSelectors ? slowClickIgnoreSelectors.join(',') : '',
multiClickTimeout: MULTI_CLICK_TIMEOUT,
}
: undefined;

Expand Down
1 change: 0 additions & 1 deletion packages/replay/src/types/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -476,5 +476,4 @@ export interface SlowClickConfig {
timeout: number;
scrollTimeout: number;
ignoreSelector: string;
multiClickTimeout: number;
}