From 2921c5eff23d0be06141c5d80f59149befb1f334 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 22 Jun 2023 10:26:04 +0200 Subject: [PATCH 1/3] fix(replay): Adjust slow/multi click handling --- .../replay/slowClick/multiClick/test.ts | 102 ++++++++++++++++ packages/replay/src/constants.ts | 2 - .../replay/src/coreHandlers/handleClick.ts | 40 ++----- packages/replay/src/replay.ts | 2 - packages/replay/src/types/replay.ts | 1 - .../unit/coreHandlers/handleClick.test.ts | 112 ------------------ 6 files changed, 112 insertions(+), 147 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts index 2d84eeb29ac3..13aada6ef41c 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts @@ -58,3 +58,105 @@ sentryTest('captures multi click when not detecting slow click', async ({ getLoc }, ]); }); + +sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page }) => { + 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 lastMultiClickSegmentId: number | undefined; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + const check = !lastMultiClickSegmentId && breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick'); + if (check) { + lastMultiClickSegmentId = event.segment_id; + } + return check; + }); + + const reqPromise2 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + const check = + !!lastMultiClickSegmentId && + event.segment_id > lastMultiClickSegmentId && + breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick'); + if (check) { + lastMultiClickSegmentId = event.segment_id; + } + return check; + }); + + await page.click('#mutationButtonImmediately', { clickCount: 4 }); + + await new Promise(resolve => setTimeout(resolve, 1001)); + + await page.click('#mutationButtonImmediately', { clickCount: 2 }); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + const { breadcrumbs: breadcrumb2 } = getCustomRecordingEvents(await reqPromise2); + + const slowClickBreadcrumbs = breadcrumbs + .concat(breadcrumb2) + .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), + }, + ]); +}); diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index 699cefe8b784..86dc228c50bf 100644 --- a/packages/replay/src/constants.ts +++ b/packages/replay/src/constants.ts @@ -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 diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index c5a65ddfdca0..5f7e46dbcbce 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -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; @@ -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; @@ -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, @@ -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. */ @@ -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); } @@ -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); } } diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 8fab410a0c34..c72306239533 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -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, @@ -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; diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index e2cfddd0f525..0e351c27a984 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -476,5 +476,4 @@ export interface SlowClickConfig { timeout: number; scrollTimeout: number; ignoreSelector: string; - multiClickTimeout: number; } diff --git a/packages/replay/test/unit/coreHandlers/handleClick.test.ts b/packages/replay/test/unit/coreHandlers/handleClick.test.ts index 8b06e4683656..f2980e60df69 100644 --- a/packages/replay/test/unit/coreHandlers/handleClick.test.ts +++ b/packages/replay/test/unit/coreHandlers/handleClick.test.ts @@ -26,7 +26,6 @@ describe('Unit | coreHandlers | handleClick', () => { timeout: 3_000, scrollTimeout: 200, ignoreSelector: '', - multiClickTimeout: 1_000, }, mockAddBreadcrumbEvent, ); @@ -72,113 +71,6 @@ describe('Unit | coreHandlers | handleClick', () => { expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); }); - test('it groups multiple clicks together', async () => { - const replay = { - getCurrentRoute: () => 'test-route', - } as ReplayContainer; - - const mockAddBreadcrumbEvent = jest.fn(); - - const detector = new ClickDetector( - replay, - { - threshold: 1_000, - timeout: 3_000, - scrollTimeout: 200, - ignoreSelector: '', - multiClickTimeout: 1_000, - }, - mockAddBreadcrumbEvent, - ); - - const breadcrumb1: Breadcrumb = { - timestamp: BASE_TIMESTAMP / 1000, - data: { - nodeId: 1, - }, - }; - const breadcrumb2: Breadcrumb = { - timestamp: BASE_TIMESTAMP / 1000 + 0.2, - data: { - nodeId: 1, - }, - }; - const breadcrumb3: Breadcrumb = { - timestamp: BASE_TIMESTAMP / 1000 + 0.6, - data: { - nodeId: 1, - }, - }; - const breadcrumb4: Breadcrumb = { - timestamp: BASE_TIMESTAMP / 1000 + 2, - data: { - nodeId: 1, - }, - }; - const breadcrumb5: Breadcrumb = { - timestamp: BASE_TIMESTAMP / 1000 + 2.9, - data: { - nodeId: 1, - }, - }; - const node = document.createElement('button'); - detector.handleClick(breadcrumb1, node); - - detector.handleClick(breadcrumb2, node); - - detector.handleClick(breadcrumb3, node); - - expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); - - jest.advanceTimersByTime(2_000); - - expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); - - detector.handleClick(breadcrumb4, node); - detector.handleClick(breadcrumb5, node); - - jest.advanceTimersByTime(1_000); - - expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); - expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { - category: 'ui.slowClickDetected', - type: 'default', - data: { - // count is not actually correct, because this is identified by a different click handler - clickCount: 1, - endReason: 'timeout', - nodeId: 1, - route: 'test-route', - timeAfterClickMs: 3000, - url: 'http://localhost/', - }, - message: undefined, - timestamp: expect.any(Number), - }); - - jest.advanceTimersByTime(2_000); - - expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(2); - expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { - category: 'ui.slowClickDetected', - type: 'default', - data: { - // count is not actually correct, because this is identified by a different click handler - clickCount: 1, - endReason: 'timeout', - nodeId: 1, - route: 'test-route', - timeAfterClickMs: 3000, - url: 'http://localhost/', - }, - message: undefined, - timestamp: expect.any(Number), - }); - - jest.advanceTimersByTime(5_000); - expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(2); - }); - test('it captures clicks on different elements', async () => { const replay = { getCurrentRoute: () => 'test-route', @@ -193,7 +85,6 @@ describe('Unit | coreHandlers | handleClick', () => { timeout: 3_000, scrollTimeout: 200, ignoreSelector: '', - multiClickTimeout: 1_000, }, mockAddBreadcrumbEvent, ); @@ -247,7 +138,6 @@ describe('Unit | coreHandlers | handleClick', () => { timeout: 3_000, scrollTimeout: 200, ignoreSelector: '', - multiClickTimeout: 1_000, }, mockAddBreadcrumbEvent, ); @@ -304,7 +194,6 @@ describe('Unit | coreHandlers | handleClick', () => { timeout: 3_000, scrollTimeout: 200, ignoreSelector: '', - multiClickTimeout: 1_000, }, mockAddBreadcrumbEvent, ); @@ -437,7 +326,6 @@ describe('Unit | coreHandlers | handleClick', () => { timeout: 3_000, scrollTimeout: 200, ignoreSelector: '', - multiClickTimeout: 1_000, }, mockAddBreadcrumbEvent, ); From 4389c5e11caaffaaafa2ed3e44f747a7ec757559 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 22 Jun 2023 15:36:41 +0200 Subject: [PATCH 2/3] try fix flaky test?? --- .../suites/replay/slowClick/multiClick/test.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts index 13aada6ef41c..2aa02a49b8cd 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts @@ -59,7 +59,7 @@ sentryTest('captures multi click when not detecting slow click', async ({ getLoc ]); }); -sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page }) => { +sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page, forceFlushReplay }) => { if (shouldSkipReplayTest()) { sentryTest.skip(); } @@ -104,13 +104,18 @@ sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page }) = return check; }); + const time = Date.now(); + await page.click('#mutationButtonImmediately', { clickCount: 4 }); - await new Promise(resolve => setTimeout(resolve, 1001)); + // Ensure we waited at least 1s, which is the threshold to create a new ui.click breadcrumb + await waitForFunction(() => Date.now() - time > 1000); await page.click('#mutationButtonImmediately', { clickCount: 2 }); const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + await forceFlushReplay(); + const { breadcrumbs: breadcrumb2 } = getCustomRecordingEvents(await reqPromise2); const slowClickBreadcrumbs = breadcrumbs @@ -160,3 +165,10 @@ sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page }) = }, ]); }); + +async function waitForFunction(cb: () => boolean, timeout = 2000, increment = 100) { + while (timeout > 0 && !cb()) { + await new Promise(resolve => setTimeout(resolve, increment)); + await waitForFunction(cb, timeout - increment, increment); + } +} From 7bec65faa0f37d88c2e2e0b9baa0abbf8c183358 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 22 Jun 2023 16:46:59 +0200 Subject: [PATCH 3/3] fix flakyness ?? --- .../replay/slowClick/multiClick/test.ts | 53 +++++++------------ .../utils/replayHelpers.ts | 40 ++++++++++++++ 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts index 2aa02a49b8cd..82bc436f714e 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts @@ -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()) { @@ -79,47 +84,34 @@ sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page, for await page.goto(url); await reqPromise0; - let lastMultiClickSegmentId: number | undefined; + let multiClickBreadcrumbCount = 0; - const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const reqsPromise = waitForReplayRequests(page, (_event, res) => { const { breadcrumbs } = getCustomRecordingEvents(res); + const count = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick').length; - const check = !lastMultiClickSegmentId && breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick'); - if (check) { - lastMultiClickSegmentId = event.segment_id; - } - return check; - }); - - const reqPromise2 = waitForReplayRequest(page, (event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); + multiClickBreadcrumbCount += count; - const check = - !!lastMultiClickSegmentId && - event.segment_id > lastMultiClickSegmentId && - breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick'); - if (check) { - lastMultiClickSegmentId = event.segment_id; + if (multiClickBreadcrumbCount === 2) { + return true; } - return check; - }); - const time = Date.now(); + 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 waitForFunction(() => Date.now() - time > 1000); + await new Promise(resolve => setTimeout(resolve, 1001)); await page.click('#mutationButtonImmediately', { clickCount: 2 }); - - const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); await forceFlushReplay(); - const { breadcrumbs: breadcrumb2 } = getCustomRecordingEvents(await reqPromise2); + const responses = await reqsPromise; - const slowClickBreadcrumbs = breadcrumbs - .concat(breadcrumb2) + const slowClickBreadcrumbs = responses + .flatMap(res => getCustomRecordingEvents(res).breadcrumbs) .filter(breadcrumb => breadcrumb.category === 'ui.multiClick'); expect(slowClickBreadcrumbs).toEqual([ @@ -165,10 +157,3 @@ sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page, for }, ]); }); - -async function waitForFunction(cb: () => boolean, timeout = 2000, increment = 100) { - while (timeout > 0 && !cb()) { - await new Promise(resolve => setTimeout(resolve, increment)); - await waitForFunction(cb, timeout - increment, increment); - } -} diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index ec332edea74d..4860b894a836 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -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 { + const responses: Response[] = []; + + return new Promise(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'; }