✨ [RUM-3837] Force replay recording on sampled out sessions #2684

N-Boutaib · 2024-04-03T15:32:08Z

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 } to DD_RUM.startSessionReplayRecording function.

When this method is called with force option, we need to keep recording until the session ends. To achieve that:

A new tracking type is added
The cookie is updated to reflect that change, with the new tracking type
Session cache and history context are synchronised across all tabs

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

codecov-commenter · 2024-04-03T15:34:16Z

Codecov Report

Attention: Patch coverage is 97.72727% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.28%. Comparing base (5249c39) to head (b7323fc).
Report is 8 commits behind head on main.

Files	Patch %	Lines
packages/rum-core/src/domain/rumSessionManager.ts	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2684      +/-   ##
==========================================
+ Coverage   93.26%   93.28%   +0.01%     
==========================================
  Files         241      241              
  Lines        7028     7057      +29     
  Branches     1553     1569      +16     
==========================================
+ Hits         6555     6583      +28     
- Misses        473      474       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cit-pr-commenter · 2024-04-03T15:37:47Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	157.47 KiB	158.10 KiB	643 B	+0.40%	✅
Logs	56.09 KiB	56.40 KiB	319 B	+0.56%	✅
Rum Slim	103.96 KiB	104.51 KiB	563 B	+0.53%	✅
Worker	25.21 KiB	25.21 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base Average Cpu Time (ms)	Local Average Cpu Time (ms)	𝚫
adderror	0.031	0.032	0.001
addaction	0.016	0.015	-0.001
logmessage	0.005	0.006	0.001
startview	0.920	0.848	-0.072
startstopsessionreplayrecording	1.017	0.762	-0.255
addtiming	0.001	0.001	-0.000
addglobalcontext	0.001	0.001	-0.000

packages/core/src/domain/session/sessionStore.ts

packages/rum-core/src/domain/rumSessionManager.ts

packages/rum/src/boot/recorderApi.ts

packages/core/src/domain/session/sessionManager.ts

packages/rum-core/src/domain/rumSessionManager.ts

packages/rum-core/src/boot/rumPublicApi.ts

bcaudan · 2024-04-24T09:25:47Z

packages/rum-core/src/boot/rumPublicApi.ts

+      recorderApi.start(options)
      addTelemetryUsage({ feature: 'start-session-replay-recording' })


💬 suggestion: ‏what about collecting the usage of this new option? 🙂

Definitely 👌

I'll add this in a followup PR, since I need to update rum-event-format

bcaudan · 2024-04-24T09:59:11Z

packages/core/src/domain/session/sessionStore.ts

@@ -179,6 +181,16 @@ export function startSessionStore<TrackingType extends string>(
    renewObservable.notify()
  }

+  function updateSessionInStore(updatedState: Partial<SessionState>) {


💬 suggestion: could we had a test to ensure that this store update is well propagated to other tabs/store?‏

bcaudan · 2024-04-24T10:00:25Z

packages/rum-core/src/domain/rumSessionManager.ts

@@ -28,6 +29,7 @@ export const enum RumTrackingType {
  NOT_TRACKED = '0',
  TRACKED_WITH_SESSION_REPLAY = '1',
  TRACKED_WITHOUT_SESSION_REPLAY = '2',
+  TRACKED_WITH_FORCED_REPLAY = '3',


💬 suggestion: could you support this new state in the extension?
‏

Yeah, sure. I forgot about it 😅

Done ✔️

bcaudan · 2024-04-24T10:04:06Z

packages/rum-core/src/domain/rumSessionManager.ts

@@ -102,13 +106,15 @@ function hasValidRumSession(trackingType?: string): trackingType is RumTrackingT
  return (
    trackingType === RumTrackingType.NOT_TRACKED ||
    trackingType === RumTrackingType.TRACKED_WITH_SESSION_REPLAY ||
-    trackingType === RumTrackingType.TRACKED_WITHOUT_SESSION_REPLAY
+    trackingType === RumTrackingType.TRACKED_WITHOUT_SESSION_REPLAY ||


💬 suggestion: ‏could we add this new type in the related tests?

It would be nice to ensure somewhere that sessionReplayAllowed is false for the forced state as it is used to compute sampled_for_replay

This is just a typeguard IIRC, sessionReplayAllowed compares the tracking type, and is set to true only if the state is TRACKED_WITH_SESSION_REPLAY

I added a new attribute in the session object that reflect either the session was originally sampled for replay or not, to avoid any confusing (and to fix a small bug I found)

Done ✔️

packages/core/src/domain/session/sessionManager.ts

amortemousque · 2024-04-24T12:09:56Z

packages/core/src/domain/session/sessionManager.ts

@@ -84,6 +86,7 @@ export function startSessionManager<TrackingType extends string>(
    renewObservable,
    expireObservable,
    expire: sessionStore.expire,
+    updateSessionInStore: sessionStore.updateSessionInStore,


🔨 warning: ‏ FMU, updateSessionInStore() does not update the session entry in sessionContextHistory. It means that findTrackedSession() will still return return TRACKED_WITHOUT_SESSION_REPLAY.

I'll check this aspect, I didn't run into any issue while testing though 🤔

From my test when using startSessionReplayRecording({force: true}) on sampled out session, it triggers the expireObservable. Then I have to interact with the page to trigger the renewObservable.

This addressed through the synchronisation mechanism discussed

packages/rum/src/boot/recorderApi.ts

…c API

update

Update

thomas-lebeau · 2024-05-09T12:09:24Z

packages/core/src/domain/session/sessionManager.spec.ts

+      })
+      sessionManager.updateSession({ [FIRST_PRODUCT_KEY]: FakeTrackingType.OTHER_TRACKING })
+
+      expect(sessionManager.findActiveSession()!.id).toBeDefined()


💬 suggestion: ‏

Suggested change

expect(sessionManager.findActiveSession()!.id).toBeDefined()

expectSessionIdToBeDefined()

thomas-lebeau · 2024-05-09T12:10:34Z

packages/core/src/domain/session/sessionManager.spec.ts

+      const sessionManager = startSessionManagerWithDefaults({
+        allowedTrackingTransition: FakeTrackingType.OTHER_TRACKING,
+      })
+      sessionManager.updateSession({ [FIRST_PRODUCT_KEY]: FakeTrackingType.DISALLOWED_TRACKING })


💬 suggestion: ‏

Suggested change

sessionManager.updateSession({ [FIRST_PRODUCT_KEY]: FakeTrackingType.DISALLOWED_TRACKING })

expectSessionToBeExpired()

thomas-lebeau · 2024-05-09T12:15:02Z

packages/core/src/domain/session/sessionStore.ts

+    const a = didSessionIdChange || untoleratedTrackingChange
+    return a


💬 suggestion: ‏

Suggested change

const a = didSessionIdChange || untoleratedTrackingChange

return a

return didSessionIdChange || untoleratedTrackingChange

thomas-lebeau · 2024-05-09T12:16:31Z

packages/core/src/domain/telemetry/telemetryEvent.types.ts

seems like this is not an expected change for this PR, is it?

N-Boutaib force-pushed the najib.boutaib/RUM-3837-force-replay-recording-on-sampled-out-sessions branch from 3e48bbc to 59d7883 Compare April 3, 2024 15:34

N-Boutaib force-pushed the najib.boutaib/RUM-3837-force-replay-recording-on-sampled-out-sessions branch 2 times, most recently from 562e9c6 to 7b93d67 Compare April 23, 2024 15:10

N-Boutaib marked this pull request as ready for review April 23, 2024 15:31

N-Boutaib requested a review from a team as a code owner April 23, 2024 15:31

thomas-lebeau reviewed Apr 24, 2024

View reviewed changes

bcaudan reviewed Apr 24, 2024

View reviewed changes

amortemousque reviewed Apr 24, 2024

View reviewed changes

N-Boutaib force-pushed the najib.boutaib/RUM-3837-force-replay-recording-on-sampled-out-sessions branch from ed48e1b to 985f6e8 Compare April 25, 2024 13:43

N-Boutaib added 7 commits May 2, 2024 15:28

[RUM-3837] Force replay recording on sampled out sessions using publi…

6d5f14e

…c API

[RUM-3837] Add unit and E2E tests

0f05596

[RUM-3837] Format code

2cc0e5f

Expose a method to update session in store - cookie & cache

d5c66cd

update

Apply reviews

2bea578

Update

Separate session sampling state from replay state

b923430

Format

f6e6a40

N-Boutaib force-pushed the najib.boutaib/RUM-3837-force-replay-recording-on-sampled-out-sessions branch from 192cc6a to f6e6a40 Compare May 2, 2024 13:28

N-Boutaib marked this pull request as draft May 2, 2024 13:28

N-Boutaib added 6 commits May 2, 2024 15:59

Fix old files after rebase

ba40013

Propagate session synchronization across store managers

e229c9f

Update

8151108

Fix tests

09e9d21

Report forced start to usage telemetry

80acc4d

Sync schemas

d09444b

N-Boutaib marked this pull request as ready for review May 7, 2024 09:16

N-Boutaib requested review from bcaudan, amortemousque and thomas-lebeau May 7, 2024 09:16

N-Boutaib added 2 commits May 7, 2024 15:44

Add more tests

823b435

Format

b7323fc

thomas-lebeau reviewed May 9, 2024

View reviewed changes

N-Boutaib marked this pull request as draft May 24, 2024 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [RUM-3837] Force replay recording on sampled out sessions #2684

✨ [RUM-3837] Force replay recording on sampled out sessions #2684

N-Boutaib commented Apr 3, 2024 •

edited

codecov-commenter commented Apr 3, 2024 •

edited

cit-pr-commenter bot commented Apr 3, 2024 •

edited

bcaudan Apr 24, 2024

N-Boutaib Apr 24, 2024

N-Boutaib Apr 25, 2024

bcaudan Apr 24, 2024

bcaudan Apr 24, 2024

N-Boutaib Apr 24, 2024

N-Boutaib Apr 25, 2024

bcaudan Apr 24, 2024

N-Boutaib Apr 24, 2024

N-Boutaib Apr 25, 2024

amortemousque Apr 24, 2024 •

edited

N-Boutaib Apr 24, 2024

amortemousque Apr 25, 2024

N-Boutaib May 7, 2024

thomas-lebeau May 9, 2024

thomas-lebeau May 9, 2024

thomas-lebeau May 9, 2024

thomas-lebeau May 9, 2024

		recorderApi.start(options)
		addTelemetryUsage({ feature: 'start-session-replay-recording' })

	expect(sessionManager.findActiveSession()!.id).toBeDefined()
	expectSessionIdToBeDefined()

	sessionManager.updateSession({ [FIRST_PRODUCT_KEY]: FakeTrackingType.DISALLOWED_TRACKING })
	expectSessionToBeExpired()

		const a = didSessionIdChange \|\| untoleratedTrackingChange
		return a

	const a = didSessionIdChange \|\| untoleratedTrackingChange
	return a
	return didSessionIdChange \|\| untoleratedTrackingChange

✨ [RUM-3837] Force replay recording on sampled out sessions #2684

Are you sure you want to change the base?

✨ [RUM-3837] Force replay recording on sampled out sessions #2684

Conversation

N-Boutaib commented Apr 3, 2024 • edited

Motivation

Changes

Testing

codecov-commenter commented Apr 3, 2024 • edited

Codecov Report

cit-pr-commenter bot commented Apr 3, 2024 • edited

Bundles Sizes Evolution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amortemousque Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Boutaib commented Apr 3, 2024 •

edited

codecov-commenter commented Apr 3, 2024 •

edited

cit-pr-commenter bot commented Apr 3, 2024 •

edited

amortemousque Apr 24, 2024 •

edited