-
Notifications
You must be signed in to change notification settings - Fork 140
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
✨ [RUM-3837] Force Replay recording on sampled-out sessions #2777
✨ [RUM-3837] Force Replay recording on sampled-out sessions #2777
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
e633213
to
488177d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
+ Coverage 93.59% 93.61% +0.02%
==========================================
Files 243 243
Lines 7068 7094 +26
Branches 1567 1580 +13
==========================================
+ Hits 6615 6641 +26
Misses 453 453 ☔ View full report in Codecov by Sentry. |
b24dbfe
to
637814d
Compare
/to-staging |
🚂 Branch Integration: this build is next, merge in < 32m Commit 88c335a0e2 will soon be integrated into staging-22. This build is next! (estimated merge in less than 32m) Use |
🚨 Branch Integration: The build pipeline contains failing jobs for this merge request We couldn't automatically merge the commit 88c335a0e2 into staging-22. Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail. You should have a look at the pipeline, wait for the build to finish and investigate the failures.
|
/to-staging |
🚂 Branch Integration: starting soon, merge in < 32m Commit bea862d58c will soon be integrated into staging-22. This build is going to start soon! (estimated merge in less than 32m) Use |
…2777) into staging-22 Integrated commit sha: bea862d Co-authored-by: Najib Boutaib <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit bea862d58c has been merged into staging-22 in merge commit e54e3eb0ee. Check out the triggered pipeline on Gitlab 🦊 |
@@ -53,6 +55,12 @@ export function startSessionManager<TrackingType extends string>( | |||
expireObservable.notify() | |||
sessionContextHistory.closeActive(relativeNow()) | |||
}) | |||
sessionStore.forceReplayObservable.subscribe(() => { |
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.
💭 thought: RUM logic is leaking into core. Maybe we can think of a way to abstract it, via computeSessionState
?
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.
Indeed, but that wouldn't be enough because the sessionContextHistory
entry won't be updated
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.
Done ✔️
/to-staging |
🚂 Branch Integration: starting soon, merge in < 33m Commit 18876276b2 will soon be integrated into staging-22. This build is going to start soon! (estimated merge in less than 33m) Use |
Commit 18876276b2 had already been merged into staging-22 If you need support, contact us on Slack #devflow! |
} | ||
|
||
export interface SessionContext<TrackingType extends string> extends Context { | ||
id: string | ||
trackingType: TrackingType | ||
isReplayForced: boolean |
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.
💭 thought: Ideally we should remove any notion of "replay" from @datadog/browser-core
. But I understand this last bit is not trivial to fix, so let's keep it like this for now, and we'll re-explore this later (I opened https://datadoghq.atlassian.net/browse/RUM-4740 for that)
@@ -57,6 +58,8 @@ type RecorderState = | |||
stopRecording: () => void | |||
} | |||
|
|||
type StartStrategyFn = (options?: StartRecordingOptions) => void |
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.
💬 suggestion: Since there is 1 ref to that type we could inline it.
@@ -139,9 +142,9 @@ export function makeRecorderApi( | |||
return cachedDeflateEncoder | |||
} | |||
|
|||
startStrategy = () => { | |||
startStrategy = (options?: StartRecordingOptions) => { |
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.
💬 suggestion: The type can be infered, so
startStrategy = (options?: StartRecordingOptions) => { | |
startStrategy = (options) => { |
also here
startStrategy = (options?: StartRecordingOptions) => { |
Motivation
Allow session replay recording on sampled out sessions
Changes
On the RUM's public API, session replay recording can be forced even if the session is sampled out for replay. This is achieved by passing
{ force: true }
toDD_RUM.startSessionReplayRecording
function.When this method is called with
force
option, we need to keep recording until the session ends. To achieve that:_dd_s
cookieTesting
I have gone over the contributing documentation.