-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(replay): Do not capture replays < 5 seconds #7949
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
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 |
---|---|---|
|
@@ -80,6 +80,30 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa | |
} | ||
} | ||
|
||
const options = replay.getOptions(); | ||
|
||
// TODO: We want this as an experiment so that we can test | ||
// internally and create metrics before making this the default | ||
if (options._experiments.delayFlushOnCheckout) { | ||
// If the full snapshot is due to an initial load, we will not have | ||
// a previous session ID. In this case, we want to buffer events | ||
// for a set amount of time before flushing. This can help avoid | ||
// capturing replays of users that immediately close the window. | ||
setTimeout(() => replay.conditionalFlush(), options._experiments.delayFlushOnCheckout); | ||
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 wonder if this is worth the overhead, to have a separate initial flush timeout 🤔 If we "enforce" this to be === general flush debounce timeout, we could simply call debounced flush and call it a day? Then it would also ensure we do not flush twice etc. out of the box? 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 it's fine for now since this is an experiment. We can think of bundling it up with the debounced flush when we GA this. The conditionalFlush here operates on the debounced flush anyway. 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. If we allowed the general debounce timeout to be user-controlled, chances are that they set it to something too high that would cause payloads to become too big? 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 don't think this is necessarily an option we expose at all - we just want to verify data integrity on Sentry before moving forward with a change. |
||
|
||
// Cancel any previously debounced flushes to ensure there are no [near] | ||
// simultaneous flushes happening. The latter request should be | ||
// insignificant in this case, so wait for additional user interaction to | ||
// trigger a new flush. | ||
// | ||
// This can happen because there's no guarantee that a recording event | ||
// happens first. e.g. a mouse click can happen and trigger a debounced | ||
// flush before the checkout. | ||
replay.cancelFlush(); | ||
|
||
return true; | ||
} | ||
|
||
// Flush immediately so that we do not miss the first segment, otherwise | ||
// it can prevent loading on the UI. This will cause an increase in short | ||
// replays (e.g. opening and closing a tab quickly), but these can be | ||
|
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.
m: As long as we keep this an experiment, I'm fine with this name but if we make part of the public stable API, should we rename it to something less technical like
initialFlushDelay
?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.
Yeah, as stated in a diff comment, this may not be a public option, but if it is, we will rename it!