Skip to content

fix(utils): Avoid keeping a reference of last used event #9387

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

Merged
merged 6 commits into from
Oct 30, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<title></title>
</head>
<body>
<button id="button1" type="button">Button 1</button>
<button id="button2" type="button">Button 2</button>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';

sentryTest('captures Breadcrumb for clicks & debounces them for a second', async ({ getLocalTestUrl, page }) => {
const url = await getLocalTestUrl({ testDir: __dirname });

await page.route('**/foo', route => {
return route.fulfill({
status: 200,
body: JSON.stringify({
userNames: ['John', 'Jane'],
}),
headers: {
'Content-Type': 'application/json',
},
});
});

const promise = getFirstSentryEnvelopeRequest<Event>(page);

await page.goto(url);

await page.click('#button1');
// not debounced because other target
await page.click('#button2');
// This should be debounced
await page.click('#button2');

// Wait a second for the debounce to finish
await page.waitForTimeout(1000);
await page.click('#button2');

await page.evaluate('Sentry.captureException("test exception")');

const eventData = await promise;

expect(eventData.exception?.values).toHaveLength(1);

expect(eventData.breadcrumbs).toEqual([
{
timestamp: expect.any(Number),
category: 'ui.click',
message: 'body > button#button1[type="button"]',
},
{
timestamp: expect.any(Number),
category: 'ui.click',
message: 'body > button#button2[type="button"]',
},
{
timestamp: expect.any(Number),
category: 'ui.click',
message: 'body > button#button2[type="button"]',
},
]);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
defaultIntegrations: false,
integrations: [new Sentry.Integrations.Breadcrumbs()],
sampleRate: 1,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<title></title>
</head>
<body>
<input id="input1" type="text" />
<input id="input2" type="text" />
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';

sentryTest('captures Breadcrumb for events on inputs & debounced them', async ({ getLocalTestUrl, page }) => {
const url = await getLocalTestUrl({ testDir: __dirname });

await page.route('**/foo', route => {
return route.fulfill({
status: 200,
body: JSON.stringify({
userNames: ['John', 'Jane'],
}),
headers: {
'Content-Type': 'application/json',
},
});
});

const promise = getFirstSentryEnvelopeRequest<Event>(page);

await page.goto(url);

await page.click('#input1');
// Not debounced because other event type
await page.type('#input1', 'John', { delay: 1 });
// This should be debounced
await page.type('#input1', 'Abby', { delay: 1 });
// not debounced because other target
await page.type('#input2', 'Anne', { delay: 1 });

// Wait a second for the debounce to finish
await page.waitForTimeout(1000);
await page.type('#input2', 'John', { delay: 1 });

await page.evaluate('Sentry.captureException("test exception")');

const eventData = await promise;

expect(eventData.exception?.values).toHaveLength(1);

expect(eventData.breadcrumbs).toEqual([
{
timestamp: expect.any(Number),
category: 'ui.click',
message: 'body > input#input1[type="text"]',
},
{
timestamp: expect.any(Number),
category: 'ui.input',
message: 'body > input#input1[type="text"]',
},
{
timestamp: expect.any(Number),
category: 'ui.input',
message: 'body > input#input2[type="text"]',
},
{
timestamp: expect.any(Number),
category: 'ui.input',
message: 'body > input#input2[type="text"]',
},
]);
});
62 changes: 39 additions & 23 deletions packages/utils/src/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
import { isString } from './is';
import type { ConsoleLevel } from './logger';
import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger';
import { uuid4 } from './misc';
import { addNonEnumerableProperty, fill } from './object';
import { getFunctionName } from './stacktrace';
import { supportsHistory, supportsNativeFetch } from './supports';
Expand Down Expand Up @@ -404,21 +405,24 @@ function instrumentHistory(): void {

const DEBOUNCE_DURATION = 1000;
let debounceTimerID: number | undefined;
let lastCapturedEvent: Event | undefined;
let lastCapturedEventType: string | undefined;
let lastCapturedEventTargetId: string | undefined;

type SentryWrappedTarget = HTMLElement & { _sentryId?: string };

/**
* Check whether two DOM events are similar to eachother. For example, two click events on the same button.
* Check whether the event is similar to the last captured one. For example, two click events on the same button.
*/
function areSimilarDomEvents(a: Event, b: Event): boolean {
function isSimilarToLastCapturedEvent(event: Event): boolean {
// If both events have different type, then user definitely performed two separate actions. e.g. click + keypress.
if (a.type !== b.type) {
if (event.type !== lastCapturedEventType) {
return false;
}

try {
// If both events have the same type, it's still possible that actions were performed on different targets.
// e.g. 2 clicks on different buttons.
if (a.target !== b.target) {
if (!event.target || (event.target as SentryWrappedTarget)._sentryId !== lastCapturedEventTargetId) {
return false;
}
} catch (e) {
Expand All @@ -436,30 +440,33 @@ function areSimilarDomEvents(a: Event, b: Event): boolean {
* Decide whether an event should be captured.
* @param event event to be captured
*/
function shouldSkipDOMEvent(event: Event): boolean {
function shouldSkipDOMEvent(eventType: string, target: SentryWrappedTarget | null): boolean {
// We are only interested in filtering `keypress` events for now.
if (event.type !== 'keypress') {
if (eventType !== 'keypress') {
return false;
}

try {
const target = event.target as HTMLElement;
if (!target || !target.tagName) {
return true;
}

if (!target || !target.tagName) {
return true;
}
// Only consider keypress events on actual input elements. This will disregard keypresses targeting body
// e.g.tabbing through elements, hotkeys, etc.
if (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) {
return false;
}

// Only consider keypress events on actual input elements. This will disregard keypresses targeting body
// e.g.tabbing through elements, hotkeys, etc.
if (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) {
return false;
}
return true;
}

function getEventTarget(event: Event): SentryWrappedTarget | null {
try {
return event.target as SentryWrappedTarget | null;
} catch (e) {
// just accessing `target` property can throw an exception in some rare circumstances
// see: https://github.com/getsentry/sentry-javascript/issues/838
return null;
}

return true;
}

/**
Expand All @@ -478,32 +485,41 @@ function makeDOMEventHandler(handler: Function, globalListener: boolean = false)
return;
}

const target = getEventTarget(event);

// We always want to skip _some_ events.
if (shouldSkipDOMEvent(event)) {
if (shouldSkipDOMEvent(event.type, target)) {
return;
}

// Mark event as "seen"
addNonEnumerableProperty(event, '_sentryCaptured', true);

if (target && !target._sentryId) {
// Add UUID to event target so we can identify if
addNonEnumerableProperty(target, '_sentryId', uuid4());
}

const name = event.type === 'keypress' ? 'input' : event.type;

// If there is no last captured event, it means that we can safely capture the new event and store it for future comparisons.
// If there is a last captured event, see if the new event is different enough to treat it as a unique one.
// If that's the case, emit the previous event and store locally the newly-captured DOM event.
if (lastCapturedEvent === undefined || !areSimilarDomEvents(lastCapturedEvent, event)) {
if (!isSimilarToLastCapturedEvent(event)) {
handler({
event: event,
name,
global: globalListener,
});
lastCapturedEvent = event;
lastCapturedEventType = event.type;
lastCapturedEventTargetId = target ? target._sentryId : undefined;
}

// Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together.
clearTimeout(debounceTimerID);
debounceTimerID = WINDOW.setTimeout(() => {
lastCapturedEvent = undefined;
lastCapturedEventTargetId = undefined;
lastCapturedEventType = undefined;
}, DEBOUNCE_DURATION);
};
}
Expand Down
3 changes: 3 additions & 0 deletions rollup/plugins/bundlePlugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ export function makeTerserPlugin() {
'_meta',
// Object we inject debug IDs into with bundler plugins
'_sentryDebugIds',
// These are used by instrument.ts in utils for identifying HTML elements & events
'_sentryCaptured',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good thing we have good integration tests, that captured this! We also forgot this for _sentryCaptured (I guess we have no integration test covering this), so I also added this here @lforst

'_sentryId',
],
},
},
Expand Down