From 685d5d81aa4f94181ec14d343ef9263b42aa5aad Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 27 Oct 2023 15:04:51 +0200 Subject: [PATCH 1/2] ref(replay): Use rrweb for slow click detection --- .../replay/src/coreHandlers/handleClick.ts | 111 ++++++++++++------ .../replay/src/coreHandlers/util/domUtils.ts | 9 +- packages/replay/src/types/replay.ts | 8 ++ .../replay/src/util/handleRecordingEmit.ts | 5 + 4 files changed, 95 insertions(+), 38 deletions(-) diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index 91928ea6ac3f..9b1df4ee13c1 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -1,16 +1,19 @@ +import { IncrementalSource, MouseInteractions, record } from '@sentry-internal/rrweb'; import type { Breadcrumb } from '@sentry/types'; import { WINDOW } from '../constants'; import type { + RecordingEvent, ReplayClickDetector, ReplayContainer, ReplayMultiClickFrame, ReplaySlowClickFrame, SlowClickConfig, } from '../types'; +import { ReplayEventTypeFullSnapshot, ReplayEventTypeIncrementalSnapshot } from '../types'; import { timestampToS } from '../util/timestamp'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; -import { getClickTargetNode } from './util/domUtils'; +import { getClosestInteractive } from './util/domUtils'; import { onWindowOpen } from './util/onWindowOpen'; type ClickBreadcrumb = Breadcrumb & { @@ -26,6 +29,16 @@ interface Click { node: HTMLElement; } +type IncrementalRecordingEvent = RecordingEvent & { + type: typeof ReplayEventTypeIncrementalSnapshot; + data: { source: IncrementalSource }; +}; + +type IncrementalMouseInteractionRecordingEvent = IncrementalRecordingEvent & { + type: typeof ReplayEventTypeIncrementalSnapshot; + data: { type: MouseInteractions; id: number }; +}; + /** Handle a click. */ export function handleClick(clickDetector: ReplayClickDetector, clickBreadcrumb: Breadcrumb, node: HTMLElement): void { clickDetector.handleClick(clickBreadcrumb, node); @@ -70,48 +83,14 @@ export class ClickDetector implements ReplayClickDetector { /** Register click detection handlers on mutation or scroll. */ public addListeners(): void { - const mutationHandler = (): void => { - this._lastMutation = nowInSeconds(); - }; - - const scrollHandler = (): void => { - this._lastScroll = nowInSeconds(); - }; - const cleanupWindowOpen = onWindowOpen(() => { // Treat window.open as mutation this._lastMutation = nowInSeconds(); }); - const clickHandler = (event: MouseEvent): void => { - if (!event.target) { - return; - } - - const node = getClickTargetNode(event); - if (node) { - this._handleMultiClick(node as HTMLElement); - } - }; - - const obs = new MutationObserver(mutationHandler); - - obs.observe(WINDOW.document.documentElement, { - attributes: true, - characterData: true, - childList: true, - subtree: true, - }); - - WINDOW.addEventListener('scroll', scrollHandler, { passive: true }); - WINDOW.addEventListener('click', clickHandler, { passive: true }); - this._teardown = () => { - WINDOW.removeEventListener('scroll', scrollHandler); - WINDOW.removeEventListener('click', clickHandler); cleanupWindowOpen(); - obs.disconnect(); this._clicks = []; this._lastMutation = 0; this._lastScroll = 0; @@ -129,7 +108,7 @@ export class ClickDetector implements ReplayClickDetector { } } - /** Handle a click */ + /** @inheritDoc */ public handleClick(breadcrumb: Breadcrumb, node: HTMLElement): void { if (ignoreElement(node, this._ignoreSelector) || !isClickBreadcrumb(breadcrumb)) { return; @@ -158,6 +137,22 @@ export class ClickDetector implements ReplayClickDetector { } } + /** @inheritDoc */ + public registerMutation(timestamp = Date.now()): void { + this._lastMutation = timestampToS(timestamp); + } + + /** @inheritDoc */ + public registerScroll(timestamp = Date.now()): void { + this._lastScroll = timestampToS(timestamp); + } + + /** @inheritDoc */ + public registerClick(element: HTMLElement): void { + const node = getClosestInteractive(element); + this._handleMultiClick(node as HTMLElement); + } + /** Count multiple clicks on elements. */ private _handleMultiClick(node: HTMLElement): void { this._getClicks(node).forEach(click => { @@ -311,3 +306,47 @@ function isClickBreadcrumb(breadcrumb: Breadcrumb): breadcrumb is ClickBreadcrum function nowInSeconds(): number { return Date.now() / 1000; } + +/** Update the click detector based on a recording event of rrweb. */ +export function updateClickDetectorForRecordingEvent(clickDetector: ReplayClickDetector, event: RecordingEvent): void { + try { + // We interpret a full snapshot as a mutation (this may not be true, but there is no way for us to know) + if (event.type === ReplayEventTypeFullSnapshot) { + clickDetector.registerMutation(event.timestamp); + } + + if (!isIncrementalEvent(event)) { + return; + } + + const { source } = event.data; + if (source === IncrementalSource.Mutation) { + clickDetector.registerMutation(event.timestamp); + } + + if (source === IncrementalSource.Scroll) { + clickDetector.registerScroll(event.timestamp); + } + + if (isIncrementalMouseInteraction(event)) { + const { type, id } = event.data; + const node = record.mirror.getNode(id); + + if (node instanceof HTMLElement && type === MouseInteractions.Click) { + clickDetector.registerClick(node); + } + } + } catch { + // ignore errors here, e.g. if accessing something that does not exist + } +} + +function isIncrementalEvent(event: RecordingEvent): event is IncrementalRecordingEvent { + return event.type === ReplayEventTypeIncrementalSnapshot; +} + +function isIncrementalMouseInteraction( + event: IncrementalRecordingEvent, +): event is IncrementalMouseInteractionRecordingEvent { + return event.data.source === IncrementalSource.MouseInteraction; +} diff --git a/packages/replay/src/coreHandlers/util/domUtils.ts b/packages/replay/src/coreHandlers/util/domUtils.ts index 6091dc7fe837..5f45bfbd367d 100644 --- a/packages/replay/src/coreHandlers/util/domUtils.ts +++ b/packages/replay/src/coreHandlers/util/domUtils.ts @@ -7,6 +7,12 @@ export interface DomHandlerData { const INTERACTIVE_SELECTOR = 'button,a'; +/** Get the closest interactive parent element, or else return the given element. */ +export function getClosestInteractive(element: Element): Element { + const closestInteractive = element.closest(INTERACTIVE_SELECTOR); + return closestInteractive || element; +} + /** * For clicks, we check if the target is inside of a button or link * If so, we use this as the target instead @@ -20,8 +26,7 @@ export function getClickTargetNode(event: DomHandlerData['event'] | MouseEvent): return target; } - const closestInteractive = target.closest(INTERACTIVE_SELECTOR); - return closestInteractive || target; + return getClosestInteractive(target); } /** Get the event target node. */ diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index 96e4a4fbf36f..f8ec28e2f799 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -441,7 +441,15 @@ export interface SendBufferedReplayOptions { export interface ReplayClickDetector { addListeners(): void; removeListeners(): void; + + /** Handle a click breadcrumb. */ handleClick(breadcrumb: Breadcrumb, node: HTMLElement): void; + /** Register a mutation that happened at a given time. */ + registerMutation(timestamp?: number): void; + /** Register a scroll that happened at a given time. */ + registerScroll(timestamp?: number): void; + /** Register that a click on an element happened. */ + registerClick(element: HTMLElement): void; } export interface ReplayContainer { diff --git a/packages/replay/src/util/handleRecordingEmit.ts b/packages/replay/src/util/handleRecordingEmit.ts index ada70adf980d..ac7d098825b4 100644 --- a/packages/replay/src/util/handleRecordingEmit.ts +++ b/packages/replay/src/util/handleRecordingEmit.ts @@ -1,6 +1,7 @@ import { EventType } from '@sentry-internal/rrweb'; import { logger } from '@sentry/utils'; +import { updateClickDetectorForRecordingEvent } from '../coreHandlers/handleClick'; import { saveSession } from '../session/saveSession'; import type { RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types'; import { addEventSync } from './addEvent'; @@ -29,6 +30,10 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa const isCheckout = _isCheckout || !hadFirstEvent; hadFirstEvent = true; + if (replay.clickDetector) { + updateClickDetectorForRecordingEvent(replay.clickDetector, event); + } + // The handler returns `true` if we do not want to trigger debounced flush, `false` if we want to debounce flush. replay.addUpdate(() => { // The session is always started immediately on pageload/init, but for From 1e2b0b293aca43df31955e61da1985418a7fd6c8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 31 Oct 2023 09:24:45 +0100 Subject: [PATCH 2/2] add tests --- .../suites/replay/slowClick/error/init.js | 18 +++ .../replay/slowClick/error/template.html | 22 +++ .../suites/replay/slowClick/error/test.ts | 140 ++++++++++++++++++ .../suites/replay/slowClick/mutation/test.ts | 18 ++- .../replay/src/coreHandlers/handleClick.ts | 13 +- 5 files changed, 205 insertions(+), 6 deletions(-) create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/error/init.js create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/error/template.html create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/error/test.ts diff --git a/packages/browser-integration-tests/suites/replay/slowClick/error/init.js b/packages/browser-integration-tests/suites/replay/slowClick/error/init.js new file mode 100644 index 000000000000..b253a8bca6e9 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/error/init.js @@ -0,0 +1,18 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; +window.Replay = new Sentry.Replay({ + flushMinDelay: 200, + flushMaxDelay: 200, + minReplayDuration: 0, + slowClickTimeout: 3500, +}); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + sampleRate: 1, + replaysSessionSampleRate: 0.0, + replaysOnErrorSampleRate: 1.0, + + integrations: [window.Replay], +}); diff --git a/packages/browser-integration-tests/suites/replay/slowClick/error/template.html b/packages/browser-integration-tests/suites/replay/slowClick/error/template.html new file mode 100644 index 000000000000..1a394556896f --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/error/template.html @@ -0,0 +1,22 @@ + + + + + + + + + + + + diff --git a/packages/browser-integration-tests/suites/replay/slowClick/error/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/error/test.ts new file mode 100644 index 000000000000..09c1fc29d3c0 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/error/test.ts @@ -0,0 +1,140 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { + getCustomRecordingEvents, + getReplayEventFromRequest, + shouldSkipReplayTest, + waitForReplayRequest, +} from '../../../../utils/replayHelpers'; + +sentryTest('slow click that triggers error is captured', async ({ getLocalTestUrl, page }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + 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); + + const [req0] = await Promise.all([ + waitForReplayRequest(page, (_event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + }), + page.click('#buttonError'), + ]); + + const { breadcrumbs } = getCustomRecordingEvents(req0); + + const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + + expect(slowClickBreadcrumbs).toEqual([ + { + category: 'ui.slowClickDetected', + type: 'default', + data: { + endReason: 'timeout', + clickCount: 1, + node: { + attributes: { + id: 'buttonError', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '******* *****', + }, + nodeId: expect.any(Number), + timeAfterClickMs: 3500, + url: 'http://sentry-test.io/index.html', + }, + message: 'body > button#buttonError', + timestamp: expect.any(Number), + }, + ]); +}); + +sentryTest( + 'click that triggers error & mutation is not captured', + async ({ getLocalTestUrl, page, forceFlushReplay }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + 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); + + let slowClickCount = 0; + + page.on('response', res => { + const req = res.request(); + + const event = getReplayEventFromRequest(req); + + if (!event) { + return; + } + + const { breadcrumbs } = getCustomRecordingEvents(res); + + const slowClicks = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + slowClickCount += slowClicks.length; + }); + + const [req1] = await Promise.all([ + waitForReplayRequest(page, (_event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }), + page.click('#buttonErrorMutation'), + ]); + + const { breadcrumbs } = getCustomRecordingEvents(req1); + + expect(breadcrumbs).toEqual([ + { + category: 'ui.click', + data: { + node: { + attributes: { + id: 'buttonErrorMutation', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '******* *****', + }, + nodeId: expect.any(Number), + }, + message: 'body > button#buttonErrorMutation', + timestamp: expect.any(Number), + type: 'default', + }, + ]); + + // Ensure we wait for timeout, to make sure no slow click is created + // Waiting for 3500 + 1s rounding room + await new Promise(resolve => setTimeout(resolve, 4500)); + await forceFlushReplay(); + + expect(slowClickCount).toBe(0); + }, +); diff --git a/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts index 196719783229..39599c84cd32 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts @@ -141,8 +141,17 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); await forceFlushReplay(); + let slowClickCount = 0; + + page.on('response', res => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + const slowClicks = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + slowClickCount += slowClicks.length; + }); + const [req1] = await Promise.all([ - waitForReplayRequest(page, (event, res) => { + waitForReplayRequest(page, (_event, res) => { const { breadcrumbs } = getCustomRecordingEvents(res); return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); @@ -171,6 +180,13 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush type: 'default', }, ]); + + // Ensure we wait for timeout, to make sure no slow click is created + // Waiting for 3500 + 1s rounding room + await new Promise(resolve => setTimeout(resolve, 4500)); + await forceFlushReplay(); + + expect(slowClickCount).toBe(0); }); sentryTest('inline click handler does not trigger slow click', async ({ forceFlushReplay, getLocalTestUrl, page }) => { diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index 9b1df4ee13c1..dab5d04657fc 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -10,7 +10,7 @@ import type { ReplaySlowClickFrame, SlowClickConfig, } from '../types'; -import { ReplayEventTypeFullSnapshot, ReplayEventTypeIncrementalSnapshot } from '../types'; +import { ReplayEventTypeIncrementalSnapshot } from '../types'; import { timestampToS } from '../util/timestamp'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; import { getClosestInteractive } from './util/domUtils'; @@ -310,10 +310,13 @@ function nowInSeconds(): number { /** Update the click detector based on a recording event of rrweb. */ export function updateClickDetectorForRecordingEvent(clickDetector: ReplayClickDetector, event: RecordingEvent): void { try { - // We interpret a full snapshot as a mutation (this may not be true, but there is no way for us to know) - if (event.type === ReplayEventTypeFullSnapshot) { - clickDetector.registerMutation(event.timestamp); - } + // note: We only consider incremental snapshots here + // This means that any full snapshot is ignored for mutation detection - the reason is that we simply cannot know if a mutation happened here. + // E.g. think that we are buffering, an error happens and we take a full snapshot because we switched to session mode - + // in this scenario, we would not know if a dead click happened because of the error, which is a key dead click scenario. + // Instead, by ignoring full snapshots, we have the risk that we generate a false positive + // (if a mutation _did_ happen but was "swallowed" by the full snapshot) + // But this should be more unlikely as we'd generally capture the incremental snapshot right away if (!isIncrementalEvent(event)) { return;