-
Notifications
You must be signed in to change notification settings - Fork 128
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: allow sampling based on decide response #839
feat: allow sampling based on decide response #839
Conversation
Size Change: +7.98 kB (+1%) Total Size: 731 kB
ℹ️ View Unchanged
|
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.
(talked in person about doing all the decision stuff only in the session manager class
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.
Getting there! Small changes discussed in person
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.
Mostly comments but some things that I would at least like to have eyes on before we merge
if (this.status === 'buffering' || isBelowMinimumDuration) { | ||
this.flushBufferTimer = setTimeout(() => { | ||
this._flushBuffer() | ||
}, RECORDING_BUFFER_TIMEOUT) | ||
return this.buffer || this.clearBuffer() | ||
} |
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.
This addition comes with the new challenge of the buffer getting too big... If we buffer 15 seconds and then flush, it will essentially always buffer one super big event. Which is probably fine for now but would be interesting to see if that becomes an issue...
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, I backed away from the buffer until manual flush change in this PR. Partly cos it is getting massive but also because I think the flushing can be improved now we know we have checkout snapshots on an interval. We should be able to walk the buffer between those checkouts sending "complete" sections.
But the more we change here the harder it is to understand what might break 🙈
Co-authored-by: Ben White <[email protected]>
Co-authored-by: Ben White <[email protected]>
We want to give people the tools to control their replay spend.
This adds sampling based on decide response
tested locally
follow-up?