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

feat(replay): Throttle breadcrumbs to max 300/5s #8086

Merged
merged 9 commits into from May 26, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
@@ -0,0 +1,17 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;
window.Replay = new Sentry.Replay({
flushMinDelay: 5000,
flushMaxDelay: 5000,
useCompression: false,
});

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
sampleRate: 0,
replaysSessionSampleRate: 1.0,
replaysOnErrorSampleRate: 0.0,

integrations: [window.Replay],
});
@@ -0,0 +1,35 @@
window.loaded = [];
const head = document.querySelector('head');

const COUNT = 250;

window.__isLoaded = (run = 1) => {
return window.loaded.length === COUNT * 2 * run;
};

document.querySelector('[data-network]').addEventListener('click', () => {
const offset = window.loaded.length;

// Create many scripts
for (let i = offset; i < offset + COUNT; i++) {
const script = document.createElement('script');
script.src = `/virtual-assets/script-${i}.js`;
script.setAttribute('crossorigin', 'anonymous');
head.appendChild(script);

script.addEventListener('load', () => {
window.loaded.push(`script-${i}`);
});
}
});

document.querySelector('[data-fetch]').addEventListener('click', () => {
const offset = window.loaded.length;

// Make many fetch requests
for (let i = offset; i < offset + COUNT; i++) {
fetch(`/virtual-assets/fetch-${i}.json`).then(() => {
window.loaded.push(`fetch-${i}`);
});
}
});
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body>
<button data-fetch>Trigger fetch requests</button>
<button data-network>Trigger network requests</button>
</body>
</html>
@@ -0,0 +1,106 @@
import { expect } from '@playwright/test';
import type { Breadcrumb } from '@sentry/types';

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

const COUNT = 250;
const THROTTLE_LIMIT = 300;

sentryTest(
Copy link
Member

Choose a reason for hiding this comment

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

RE the flakiness we discussed in the call yesterday: iiuc, we're throttling all kinds of breadcrumbs, right? So to decrease/get rid of the flakiness, should we perhaps only create and check for click breadcrumbs? Maybe these are more reliably created than the network breadcrumbs.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, would clicks end up getting throttled by the core sdk first?

Copy link
Member

Choose a reason for hiding this comment

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

Huh I thought we didn't but it turns out we do, good catch!

/**
* Wraps addEventListener to capture UI breadcrumbs
* @param handler function that will be triggered
* @param globalListener indicates whether event was captured by the global event listener
* @returns wrapped breadcrumb events handler
* @hidden
*/
function makeDOMEventHandler(handler: Function, globalListener: boolean = false): (event: Event) => void {
return (event: Event): void => {
// It's possible this handler might trigger multiple times for the same
// event (e.g. event propagation through node ancestors).
// Ignore if we've already captured that event.
if (!event || lastCapturedEvent === event) {
return;
}
// We always want to skip _some_ events.
if (shouldSkipDOMEvent(event)) {
return;
}
const name = event.type === 'keypress' ? 'input' : event.type;
// If there is no debounce timer, it means that we can safely capture the new event and store it for future comparisons.
if (debounceTimerID === undefined) {
handler({
event: event,
name,
global: globalListener,
});
lastCapturedEvent = event;
}
// If there is a debounce awaiting, 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.
else if (shouldShortcircuitPreviousDebounce(lastCapturedEvent, event)) {
handler({
event: event,
name,
global: globalListener,
});
lastCapturedEvent = event;
}
// Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together.
clearTimeout(debounceTimerID);
debounceTimerID = WINDOW.setTimeout(() => {
debounceTimerID = undefined;
}, debounceDuration);
};
}

Well, then I guess we could do console breadcrumbs? 😅

'throttles breadcrumbs when many requests are made at the same time',
async ({ getLocalTestUrl, page, forceFlushReplay, browserName }) => {
if (shouldSkipReplayTest() || browserName !== 'chromium') {
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' }),
});
});

let scriptsLoaded = 0;
let fetchLoaded = 0;

await page.route('**/virtual-assets/script-**', route => {
scriptsLoaded++;
return route.fulfill({
status: 200,
contentType: 'text/javascript',
body: `const aha = ${'xx'.repeat(20_000)};`,
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be valid js? i think it needs some extra quotes around the string

Suggested change
body: `const aha = ${'xx'.repeat(20_000)};`,
body: `const aha = '${'xx'.repeat(20_000)}';`,

Copy link
Member

Choose a reason for hiding this comment

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

@ryan953 Francesco's out for the next two weeks. Do you want to take over this (only if you have availability)?

});
});

await page.route('**/virtual-assets/fetch-**', route => {
fetchLoaded++;
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ fetchResponse: 'aa'.repeat(20_000) }),
});
});

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

await page.goto(url);
await reqPromise0;

const collectedSpans: PerformanceSpan[] = [];
const collectedBreadcrumbs: Breadcrumb[] = [];

page.on('response', response => {
// We only capture sentry stuff
if (!response.url().includes('https://dsn.ingest.sentry')) {
Fixed Show fixed Hide fixed
return;
}

// If this is undefined, this is not a replay request
if (!getReplayEventFromRequest(response.request())) {
return;
}

const { performanceSpans, breadcrumbs } = getCustomRecordingEvents(response);

collectedSpans.push(
...performanceSpans.filter(span => span.op === 'resource.script' || span.op === 'resource.fetch'),
);
collectedBreadcrumbs.push(...breadcrumbs.filter(breadcrumb => breadcrumb.category === 'replay.throttled'));
});

await page.click('[data-network]');
await page.click('[data-fetch]');

await page.waitForFunction('window.__isLoaded()');
await forceFlushReplay();

await waitForFunction(() => collectedBreadcrumbs.length === 1, 6_000, 100);

// All assets have been _loaded_
expect(scriptsLoaded).toBe(COUNT);
expect(fetchLoaded).toBe(COUNT);

// But only some have been captured by replay
// We give it some wiggle room to account for flakyness
expect(collectedSpans.length).toBeLessThanOrEqual(THROTTLE_LIMIT);
expect(collectedSpans.length).toBeGreaterThanOrEqual(THROTTLE_LIMIT - 50);
expect(collectedBreadcrumbs.length).toBe(1);
},
);

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);
}
}
30 changes: 22 additions & 8 deletions packages/browser-integration-tests/utils/replayHelpers.ts
Expand Up @@ -34,6 +34,25 @@ export type IncrementalRecordingSnapshot = eventWithTime & {

export type RecordingSnapshot = FullRecordingSnapshot | IncrementalRecordingSnapshot;

/** Returns the replay event from the given request, or undefined if this is not a replay request. */
export function getReplayEventFromRequest(req: Request): ReplayEvent | undefined {
const postData = req.postData();
if (!postData) {
return undefined;
}

try {
const event = envelopeRequestParser(req);

if (!isReplayEvent(event)) {
return undefined;
}

return event;
} catch {
return undefined;
}
}
/**
* Waits for a replay request to be sent by the page and returns it.
*
Expand All @@ -58,18 +77,13 @@ export function waitForReplayRequest(
res => {
const req = res.request();

const postData = req.postData();
if (!postData) {
const event = getReplayEventFromRequest(req);

if (!event) {
return false;
}

try {
const event = envelopeRequestParser(req);

if (!isReplayEvent(event)) {
return false;
}

if (callback) {
return callback(event, res);
}
Expand Down
4 changes: 3 additions & 1 deletion packages/replay/.eslintrc.js
Expand Up @@ -8,7 +8,9 @@ module.exports = {
overrides: [
{
files: ['src/**/*.ts'],
rules: {},
rules: {
'@sentry-internal/sdk/no-unsupported-es6-methods': 'off',
},
},
{
files: ['jest.setup.ts', 'jest.config.ts'],
Expand Down
3 changes: 1 addition & 2 deletions packages/replay/src/coreHandlers/util/addBreadcrumbEvent.ts
Expand Up @@ -3,7 +3,6 @@ import type { Breadcrumb } from '@sentry/types';
import { normalize } from '@sentry/utils';

import type { ReplayContainer } from '../../types';
import { addEvent } from '../../util/addEvent';

/**
* Add a breadcrumb event to replay.
Expand All @@ -20,7 +19,7 @@ export function addBreadcrumbEvent(replay: ReplayContainer, breadcrumb: Breadcru
}

replay.addUpdate(() => {
void addEvent(replay, {
void replay.throttledAddEvent({
type: EventType.Custom,
// TODO: We were converting from ms to seconds for breadcrumbs, spans,
// but maybe we should just keep them as milliseconds
Expand Down
50 changes: 49 additions & 1 deletion packages/replay/src/replay.ts
Expand Up @@ -24,6 +24,7 @@ import type {
EventBuffer,
InternalEventContext,
PopEventContext,
RecordingEvent,
RecordingOptions,
ReplayContainer as ReplayContainerInterface,
ReplayPluginOptions,
Expand All @@ -42,6 +43,8 @@ import { getHandleRecordingEmit } from './util/handleRecordingEmit';
import { isExpired } from './util/isExpired';
import { isSessionExpired } from './util/isSessionExpired';
import { sendReplay } from './util/sendReplay';
import type { SKIPPED } from './util/throttle';
import { throttle, THROTTLED } from './util/throttle';

/**
* The main replay container class, which holds all the state and methods for recording and sending replays.
Expand Down Expand Up @@ -75,6 +78,11 @@ export class ReplayContainer implements ReplayContainerInterface {
maxSessionLife: MAX_SESSION_LIFE,
} as const;

private _throttledAddEvent: (
event: RecordingEvent,
isCheckout?: boolean,
) => typeof THROTTLED | typeof SKIPPED | Promise<AddEventResult | null>;

/**
* Options to pass to `rrweb.record()`
*/
Expand Down Expand Up @@ -136,6 +144,14 @@ export class ReplayContainer implements ReplayContainerInterface {
this._debouncedFlush = debounce(() => this._flush(), this._options.flushMinDelay, {
maxWait: this._options.flushMaxDelay,
});

this._throttledAddEvent = throttle(
(event: RecordingEvent, isCheckout?: boolean) => addEvent(this, event, isCheckout),
// Max 300 events...
300,
// ... per 5s
5,
);
}

/** Get the event context. */
Expand Down Expand Up @@ -546,6 +562,38 @@ export class ReplayContainer implements ReplayContainerInterface {
this._context.urls.push(url);
}

/**
* Add a breadcrumb event, that may be throttled.
* If it was throttled, we add a custom breadcrumb to indicate that.
*/
public throttledAddEvent(
event: RecordingEvent,
isCheckout?: boolean,
): typeof THROTTLED | typeof SKIPPED | Promise<AddEventResult | null> {
const res = this._throttledAddEvent(event, isCheckout);

// If this is THROTTLED, it means we have throttled the event for the first time
// In this case, we want to add a breadcrumb indicating that something was skipped
if (res === THROTTLED) {
const breadcrumb = createBreadcrumb({
category: 'replay.throttled',
});

this.addUpdate(() => {
void addEvent(this, {
type: EventType.Custom,
timestamp: breadcrumb.timestamp || 0,
data: {
tag: 'breadcrumb',
payload: breadcrumb,
},
});
});
}

return res;
}

/**
* Initialize and start all listeners to varying events (DOM,
* Performance Observer, Recording, Sentry SDK, etc)
Expand Down Expand Up @@ -784,7 +832,7 @@ export class ReplayContainer implements ReplayContainerInterface {
*/
private _createCustomBreadcrumb(breadcrumb: Breadcrumb): void {
this.addUpdate(() => {
void addEvent(this, {
void this.throttledAddEvent({
type: EventType.Custom,
timestamp: breadcrumb.timestamp || 0,
data: {
Expand Down
5 changes: 5 additions & 0 deletions packages/replay/src/types.ts
Expand Up @@ -8,6 +8,7 @@ import type {
} from '@sentry/types';

import type { eventWithTime, recordOptions } from './types/rrweb';
import type { SKIPPED, THROTTLED } from './util/throttle';

export type RecordingEvent = eventWithTime;
export type RecordingOptions = recordOptions;
Expand Down Expand Up @@ -498,6 +499,10 @@ export interface ReplayContainer {
session: Session | undefined;
recordingMode: ReplayRecordingMode;
timeouts: Timeouts;
throttledAddEvent: (
event: RecordingEvent,
isCheckout?: boolean,
) => typeof THROTTLED | typeof SKIPPED | Promise<AddEventResult | null>;
isEnabled(): boolean;
isPaused(): boolean;
getContext(): InternalEventContext;
Expand Down
14 changes: 8 additions & 6 deletions packages/replay/src/util/createPerformanceSpans.ts
@@ -1,17 +1,16 @@
import { EventType } from '@sentry-internal/rrweb';

import type { AddEventResult, AllEntryData, ReplayContainer, ReplayPerformanceEntry } from '../types';
import { addEvent } from './addEvent';

/**
* Create a "span" for each performance entry. The parent transaction is `this.replayEvent`.
* Create a "span" for each performance entry.
*/
export function createPerformanceSpans(
replay: ReplayContainer,
entries: ReplayPerformanceEntry<AllEntryData>[],
): Promise<AddEventResult | null>[] {
return entries.map(({ type, start, end, name, data }) =>
addEvent(replay, {
return entries.map(({ type, start, end, name, data }) => {
const response = replay.throttledAddEvent({
type: EventType.Custom,
timestamp: start,
data: {
Expand All @@ -24,6 +23,9 @@ export function createPerformanceSpans(
data,
},
},
}),
);
});

// If response is a string, it means its either THROTTLED or SKIPPED
return typeof response === 'string' ? Promise.resolve(null) : response;
});
}