-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(replay): Throttle breadcrumbs to max 300/5s #8086
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
Changes from all commits
e98a356
7231d2a
433da18
f9eb6f5
13bc401
7b3a957
5c894e3
6e5fdc9
ee4440e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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://[email protected]/1337', | ||
sampleRate: 0, | ||
replaysSessionSampleRate: 1.0, | ||
replaysOnErrorSampleRate: 0.0, | ||
|
||
integrations: [window.Replay], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
const COUNT = 400; | ||
|
||
document.querySelector('[data-console]').addEventListener('click', () => { | ||
// Call console.log() many times | ||
for (let i = 0; i < COUNT; i++) { | ||
console.log(`testing ${i}`); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta charset="utf-8" /> | ||
</head> | ||
<body> | ||
<button data-console>Trigger console.log</button> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { expect } from '@playwright/test'; | ||
|
||
import { sentryTest } from '../../../utils/fixtures'; | ||
import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; | ||
|
||
const THROTTLE_LIMIT = 300; | ||
|
||
sentryTest( | ||
'throttles breadcrumbs when many `console.log` are made at the same time', | ||
async ({ getLocalTestUrl, page, forceFlushReplay, browserName }) => { | ||
if (shouldSkipReplayTest() || browserName !== 'chromium') { | ||
sentryTest.skip(); | ||
} | ||
|
||
const reqPromise0 = waitForReplayRequest(page, 0); | ||
const reqPromise1 = waitForReplayRequest(page, 1); | ||
|
||
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; | ||
|
||
await page.click('[data-console]'); | ||
await forceFlushReplay(); | ||
|
||
const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); | ||
|
||
// 1 click breadcrumb + 1 throttled breadcrumb is why console logs are less | ||
// than throttle limit | ||
expect(breadcrumbs.length).toBe(THROTTLE_LIMIT); | ||
expect(breadcrumbs.filter(breadcrumb => breadcrumb.category === 'replay.throttled').length).toBe(1); | ||
}, | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
export const THROTTLED = '__THROTTLED'; | ||
export const SKIPPED = '__SKIPPED'; | ||
|
||
/** | ||
* Create a throttled function off a given function. | ||
* When calling the throttled function, it will call the original function only | ||
* if it hasn't been called more than `maxCount` times in the last `durationSeconds`. | ||
* | ||
* Returns `THROTTLED` if throttled for the first time, after that `SKIPPED`, | ||
* or else the return value of the original function. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
export function throttle<T extends (...rest: any[]) => any>( | ||
fn: T, | ||
maxCount: number, | ||
durationSeconds: number, | ||
): (...rest: Parameters<T>) => ReturnType<T> | typeof THROTTLED | typeof SKIPPED { | ||
const counter = new Map<number, number>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: At first I was thinking, we should change this to a ring buffer because this would allow us to have a constant size memory object instead of the map that could grow. But then I noticed that we always clean up and never add or count separately. So unless I'm missing something, this map would not grow infinitely and hence the ring buffer isn't strictly necessary. It's probably still a little cleaner because the map is a dynamically allocating data structure vs a fixed-size one but I'll leave this up to you/Billy to decide (also we can revisit ofc). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine for now -- we can always revisit this in the future |
||
|
||
const _cleanup = (now: number): void => { | ||
const threshold = now - durationSeconds; | ||
counter.forEach((_value, key) => { | ||
if (key < threshold) { | ||
counter.delete(key); | ||
} | ||
}); | ||
}; | ||
|
||
const _getTotalCount = (): number => { | ||
return [...counter.values()].reduce((a, b) => a + b, 0); | ||
}; | ||
|
||
let isThrottled = false; | ||
|
||
return (...rest: Parameters<T>): ReturnType<T> | typeof THROTTLED | typeof SKIPPED => { | ||
// Date in second-precision, which we use as basis for the throttling | ||
const now = Math.floor(Date.now() / 1000); | ||
|
||
// First, make sure to delete any old entries | ||
_cleanup(now); | ||
|
||
// If already over limit, do nothing | ||
if (_getTotalCount() >= maxCount) { | ||
const wasThrottled = isThrottled; | ||
isThrottled = true; | ||
return wasThrottled ? SKIPPED : THROTTLED; | ||
} | ||
|
||
isThrottled = false; | ||
const count = counter.get(now) || 0; | ||
counter.set(now, count + 1); | ||
|
||
return fn(...rest); | ||
}; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
sentry-javascript/packages/utils/src/instrument.ts
Lines 458 to 507 in de3e675
Well, then I guess we could do console breadcrumbs? 😅