-
Notifications
You must be signed in to change notification settings - Fork 141
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
Changes from 12 commits
c5e1614
488177d
ece231f
637814d
bcbebc4
d48ac9f
88c335a
edd6cb4
ed446dd
6c4705e
2f8b40e
bea862d
9e203bd
2ccf155
e8d6cb1
b595827
1be6164
1887627
74ff0ec
58927d7
3e8657b
2e6a761
0dac36d
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 |
---|---|---|
|
@@ -18,11 +18,13 @@ export interface SessionManager<TrackingType extends string> { | |
renewObservable: Observable<void> | ||
expireObservable: Observable<void> | ||
expire: () => void | ||
setForcedReplay: () => void | ||
} | ||
|
||
export interface SessionContext<TrackingType extends string> extends Context { | ||
id: string | ||
trackingType: TrackingType | ||
isReplayForced: boolean | ||
} | ||
|
||
export const VISIBILITY_CHECK_DELAY = ONE_MINUTE | ||
|
@@ -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 commentThe 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 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. Indeed, but that wouldn't be enough because the 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. Done ✔️ |
||
const sessionEntity = sessionContextHistory.find() | ||
if (sessionEntity) { | ||
sessionEntity.isReplayForced = true | ||
} | ||
}) | ||
|
||
// We expand/renew session unconditionally as tracking consent is always granted when the session | ||
// manager is started. | ||
|
@@ -79,6 +87,7 @@ export function startSessionManager<TrackingType extends string>( | |
return { | ||
id: sessionStore.getSession().id!, | ||
trackingType: sessionStore.getSession()[productKey] as TrackingType, | ||
isReplayForced: !!sessionStore.getSession().forcedReplay, | ||
} | ||
} | ||
|
||
|
@@ -87,6 +96,7 @@ export function startSessionManager<TrackingType extends string>( | |
renewObservable, | ||
expireObservable, | ||
expire: sessionStore.expire, | ||
setForcedReplay: sessionStore.setForcedReplay, | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -14,6 +14,7 @@ import type { | |||||||
RumSessionManager, | ||||||||
RecorderApi, | ||||||||
RumConfiguration, | ||||||||
StartRecordingOptions, | ||||||||
} from '@datadog/browser-rum-core' | ||||||||
import { LifeCycleEventType } from '@datadog/browser-rum-core' | ||||||||
import { getReplayStats as getReplayStatsImpl } from '../domain/replayStats' | ||||||||
|
@@ -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 commentThe 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. |
||||||||
|
||||||||
export function makeRecorderApi( | ||||||||
startRecordingImpl: StartRecording, | ||||||||
createDeflateWorkerImpl?: CreateDeflateWorker | ||||||||
|
@@ -76,7 +79,7 @@ export function makeRecorderApi( | |||||||
status: RecorderStatus.IntentToStart, | ||||||||
} | ||||||||
|
||||||||
let startStrategy = () => { | ||||||||
let startStrategy: StartStrategyFn = () => { | ||||||||
state = { status: RecorderStatus.IntentToStart } | ||||||||
} | ||||||||
let stopStrategy = () => { | ||||||||
|
@@ -85,7 +88,7 @@ export function makeRecorderApi( | |||||||
let getSessionReplayLinkStrategy = noop as () => string | undefined | ||||||||
|
||||||||
return { | ||||||||
start: () => startStrategy(), | ||||||||
start: (options?: StartRecordingOptions) => startStrategy(options), | ||||||||
stop: () => stopStrategy(), | ||||||||
getSessionReplayLink: () => getSessionReplayLinkStrategy(), | ||||||||
onRumStart: ( | ||||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: The type can be infered, so
Suggested change
also here
|
||||||||
const session = sessionManager.findTrackedSession() | ||||||||
if (!session || !session.sessionReplayAllowed) { | ||||||||
if (!session || (!session.sessionReplayAllowed && !options?.force)) { | ||||||||
state = { status: RecorderStatus.IntentToStart } | ||||||||
return | ||||||||
} | ||||||||
|
@@ -177,6 +180,10 @@ export function makeRecorderApi( | |||||||
stopRecording, | ||||||||
} | ||||||||
}) | ||||||||
|
||||||||
if (options?.force && !session.sessionReplayAllowed) { | ||||||||
sessionManager.setForcedReplay() | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
stopStrategy = () => { | ||||||||
|
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)