⚡️ [RUM-6813] Lazy load session replay #3152

amortemousque · 2024-11-20T16:18:43Z

Motivation

Use dynamic imports to lazy load Session Replay, reducing the bundle size by 3KB (compressed) for users without Session Replay. Transitioning from Webpack, which adds overhead via __webpack_require__, to a new bundler could save additional bytes.

Changes

Configure ts module to ES2020
Update test app to target node and ES2018 (fix ssr)
Lazy load startRecording using dynamic import

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

codecov-commenter · 2024-11-21T09:23:26Z

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.48%. Comparing base (839bae6) to head (04ff36b).
Report is 80 commits behind head on v6.

Files with missing lines	Patch %	Lines
packages/core/test/collectAsyncCalls.ts	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               v6    #3152      +/-   ##
==========================================
- Coverage   93.49%   93.48%   -0.01%     
==========================================
  Files         275      275              
  Lines        7393     7417      +24     
  Branches     1675     1678       +3     
==========================================
+ Hits         6912     6934      +22     
- Misses        481      483       +2

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

amortemousque · 2024-11-21T09:23:33Z

test/app/webpack.config.js

@@ -2,7 +2,7 @@ const path = require('path')

 module.exports = (_env, argv) => ({
  entry: './app.ts',
-  target: ['web', 'es5'],
+  target: ['node', 'es2018'],


The test app used to test if the SDK works for SSR was targeting the web. I fixed it to target node and es2018.

The test app used to test if the SDK works for SSR was targeting the web.

Is it what is tested? As I understand it is used to test the npm setup and it's injected in a html:

browser-sdk/test/e2e/lib/framework/pageSetups.ts

Line 153 in 7ea12b0

header += html` <script type="text/javascript" src="./app.js"></script> `

Good point, I forgot that the test app is also used for npm e2e 😭

amortemousque · 2024-11-21T09:26:03Z

packages/core/src/browser/runOnReadyState.ts

@@ -14,3 +14,17 @@ export function runOnReadyState(
  const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED
  return addEventListener(configuration, window, eventName, callback, { once: true })
 }
+
+export function asyncRunOnReadyState(


For now, I introduced an async version of runOnReadyState but if we agree on this lazy loading approach I'll refactor the original runOnReadyState to use promises.

cit-pr-commenter · 2024-11-21T09:29:28Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	151.13 KiB	141.86 KiB	-9495 B	-6.14%	✅
Logs	50.67 KiB	50.67 KiB	0 B	0.00%	✅
Rum Slim	101.10 KiB	101.10 KiB	0 B	0.00%	✅
Worker	25.16 KiB	25.16 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base Average Cpu Time (ms)	Local Average Cpu Time (ms)	𝚫
addglobalcontext	0.002	0.002	0.000
addaction	0.034	0.038	0.004
addtiming	0.001	0.001	0.000
adderror	0.031	0.044	0.013
startstopsessionreplayrecording	0.764	0.008	-0.756
startview	0.920	0.341	-0.579
logmessage	0.022	0.018	-0.004

🧠 Memory Performance

Action Name	Base Consumption Memory (bytes)	Local Consumption Memory (bytes)	𝚫 (bytes)
addglobalcontext	8.38 KiB	9.73 KiB	1.35 KiB
addaction	41.34 KiB	41.98 KiB	651 B
addtiming	7.69 KiB	8.13 KiB	454 B
adderror	44.28 KiB	45.73 KiB	1.45 KiB
startstopsessionreplayrecording	5.85 KiB	6.09 KiB	249 B
startview	407.29 KiB	406.37 KiB	-943 B
logmessage	42.05 KiB	42.32 KiB	278 B

🔗 RealWorld

thomas-lebeau · 2024-11-21T14:28:30Z

packages/rum/src/boot/lazyLoadRecorder.ts

@@ -0,0 +1,3 @@
+export function lazyLoadRecorder() {
+  return import(/* webpackChunkName: "recorder" */ './startRecording').then((module) => module.startRecording)


❓ question: ‏ I have a doubt, does this means that customer need to use webpack as well? or is this a stupid question?

Not exactly, I used the webpackChunkName special comment to have a fixed chunk name to deploy our CDN files. For customers using NPM:

if they use webpack: the chunk will have a fix name

If they use another bundler this comment is ignored

I don't think we should use a fixed name for chunks though. It should include a hash or something to avoid cache issues, because I don't trust cloudfront/the browser to invalidate both files exactly at the same time.

We can talk about it.

thomas-lebeau · 2024-11-21T14:30:14Z

packages/rum/src/boot/recorderApi.spec.ts

-    }))
+    startRecordingSpy = jasmine.createSpy('startRecording')
+
+    // Workaround because using resolveTo(startRecordingSpy) is not was not working


Suggested change

// Workaround because using resolveTo(startRecordingSpy) is not was not working

// Workaround because using resolveTo(startRecordingSpy) was not working

thomas-lebeau · 2024-11-21T14:40:21Z

packages/rum/src/boot/recorderApi.ts

+      const doStart = async () => {
+        const [startRecordingImpl] = await Promise.all([
+          loadRecorder(),
+          asyncRunOnReadyState(configuration, 'interactive'),
+        ])
+
+        if (state.status !== RecorderStatus.Starting) {
+          return
+        }
+
+        const deflateEncoder = getOrCreateDeflateEncoder()
+        if (!deflateEncoder) {
+          state = {
+            status: RecorderStatus.Stopped,
+          }
+          return
+        }
+
+        const { stop: stopRecording } = startRecordingImpl(
+          lifeCycle,
+          configuration,
+          sessionManager,
+          viewHistory,
+          deflateEncoder
+        )
+
+        state = {
+          status: RecorderStatus.Started,
+          stopRecording,
+        }
+      }


💬 suggestion: ‏ As we're using promise, I'll avoid having state all over the place. maybe something like this would be a bit more clean:

const doStart = async () => { const [startRecordingImpl] = await Promise.all([ loadRecorder(), asyncRunOnReadyState(configuration, 'interactive'), ]) if (state.status !== RecorderStatus.Starting) { return } const deflateEncoder = getOrCreateDeflateEncoder() if (!deflateEncoder) { throw new Error('deflate encoder not initialized!'); } const { stop: stopRecording } = startRecordingImpl( lifeCycle, configuration, sessionManager, viewHistory, deflateEncoder ) return stopRecording }

And manage the state when you use it:

doStart() .then(stop => { state = { status: RecorderStatus.Started, stopRecording: stop } }) .catch(() => { state = { status: RecorderStatus.Stopped, } })

Interesting! I'll look at it

I'm not a big fan of throwing errors for control flow. Typescript does not have a syntax to tell whether a function can throw.

Having a Result type could be nicer (ex), as it can be typechecked properly.

Or, you can make doStart work as a state machine and return the new state:

const doStart = async () => { const [startRecordingImpl] = await Promise.all([ loadRecorder(), asyncRunOnReadyState(configuration, 'interactive'), ]) if (state.status !== RecorderStatus.Starting) { return state } const deflateEncoder = getOrCreateDeflateEncoder() if (!deflateEncoder) { return { status: RecorderStatus.Stopped, } } const { stop: stopRecording } = startRecordingImpl( lifeCycle, configuration, sessionManager, viewHistory, deflateEncoder ) return { status: RecorderStatus.Started, stopRecording, } }

Not sure if it simplifies things though.

I've worked on a refacto for the recorderApi to mimic what we have in RumPublicAPI:
Having preStart and postStart strategies. I think it will also simplify the lazyLoading. I'll create a dedicated PR for it.

BenoitZugmeyer · 2024-11-26T10:08:14Z

packages/core/src/browser/runOnReadyState.ts

+  return new Promise((resolve) => {
+    if (document.readyState === expectedReadyState || document.readyState === 'complete') {
+      resolve()
+    } else {
+      const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED
+      addEventListener(configuration, window, eventName, () => resolve(), { once: true })
+    }
+  })


💬 suggestion: ‏

Suggested change

return new Promise((resolve) => {

if (document.readyState === expectedReadyState || document.readyState === 'complete') {

resolve()

} else {

const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED

addEventListener(configuration, window, eventName, () => resolve(), { once: true })

}

})

return new Promise((resolve) => {

runOnReadyState(configuration, expectedReadyState, resolve)

})

BenoitZugmeyer · 2024-11-26T10:13:32Z

packages/rum/src/boot/recorderApi.spec.ts

@@ -459,51 +535,54 @@ describe('makeRecorderApi', () => {

  describe('isRecording', () => {
    it('is false when recording has not been started', () => {
-      setupRecorderApi()
+      setupRecorderApi({ startSessionReplayRecordingManually: true })


❓ question: ‏Why do we need this change?

BenoitZugmeyer · 2024-11-26T10:13:53Z

packages/rum/src/boot/recorderApi.spec.ts

@@ -514,37 +593,37 @@ describe('makeRecorderApi', () => {
    const VIEW_ID = 'xxx'

    it('is undefined when recording has not been started', () => {
-      setupRecorderApi()
+      setupRecorderApi({ startSessionReplayRecordingManually: true })


❓ question: ‏Here too

BenoitZugmeyer · 2024-11-26T10:16:04Z

packages/rum/src/boot/recorderApi.ts

-            viewHistory,
-            deflateEncoder
-          )
+        doStart().catch(() => {


💭 thought: ‏It doesn't matter too much here, since the catch callback probably won't fail, but I think we'll need to monitor() Promise callbacks at the end of promise chains

BenoitZugmeyer · 2024-11-26T10:23:32Z

packages/rum/src/boot/recorderApi.ts

+      const doStart = async () => {
+        const [startRecordingImpl] = await Promise.all([
+          loadRecorder(),
+          asyncRunOnReadyState(configuration, 'interactive'),
+        ])
+
+        if (state.status !== RecorderStatus.Starting) {
+          return
+        }
+
+        const deflateEncoder = getOrCreateDeflateEncoder()
+        if (!deflateEncoder) {
+          state = {
+            status: RecorderStatus.Stopped,
+          }
+          return
+        }
+
+        const { stop: stopRecording } = startRecordingImpl(
+          lifeCycle,
+          configuration,
+          sessionManager,
+          viewHistory,
+          deflateEncoder
+        )
+
+        state = {
+          status: RecorderStatus.Started,
+          stopRecording,
+        }
+      }


I'm not a big fan of throwing errors for control flow. Typescript does not have a syntax to tell whether a function can throw.

Having a Result type could be nicer (ex), as it can be typechecked properly.

Or, you can make doStart work as a state machine and return the new state:

const doStart = async () => { const [startRecordingImpl] = await Promise.all([ loadRecorder(), asyncRunOnReadyState(configuration, 'interactive'), ]) if (state.status !== RecorderStatus.Starting) { return state } const deflateEncoder = getOrCreateDeflateEncoder() if (!deflateEncoder) { return { status: RecorderStatus.Stopped, } } const { stop: stopRecording } = startRecordingImpl( lifeCycle, configuration, sessionManager, viewHistory, deflateEncoder ) return { status: RecorderStatus.Started, stopRecording, } }

Not sure if it simplifies things though.

BenoitZugmeyer · 2024-11-26T10:31:03Z

scripts/deploy/deploy.js

+  const bundleFolder = buildBundleFolder(packageName)
+
+  for (const uploadPathType of uploadPathTypes) {
+    for (const chunkName of chunks) {


💬 suggestion: ‏Instead of hardcoding the chunks here, we could use fs.readdirSync and upload all files in the bundles/ folder.

BenoitZugmeyer · 2024-11-26T10:34:14Z

packages/rum/src/boot/lazyLoadRecorder.ts

@@ -0,0 +1,3 @@
+export function lazyLoadRecorder() {
+  return import(/* webpackChunkName: "recorder" */ './startRecording').then((module) => module.startRecording)


I don't think we should use a fixed name for chunks though. It should include a hash or something to avoid cache issues, because I don't trust cloudfront/the browser to invalidate both files exactly at the same time.

We can talk about it.

BenoitZugmeyer · 2024-11-26T10:39:55Z

packages/core/test/collectAsyncCalls.ts

@@ -28,3 +28,34 @@ export function collectAsyncCalls<F extends jasmine.Func>(
    fail(`Unexpected extra call for spec '${currentSpec!.fullName}'`)
  }
 }
+export function asyncCollectAsyncCalls<F extends jasmine.Func>(


💭 thought: ‏Not a big fan of that function name. Maybe we could use the same function as above, but also return a Promise

export function collectAsyncCalls<F extends jasmine.Func>( spy: jasmine.Spy<F>, expectedCallsCount: number, callback?: (calls: jasmine.Calls<F>) => void ): Promise<jasmine.Calls<F>> { ... }

and deprecate callback usage so we can remove it one day.

I agree! Sry my intention was not clear. I introduced asyncCollectAsyncCalls for the early review, to validate the approach. My goal is to turn collectAsyncCalls in promise in a dedicated PR before merging this one.

amortemousque added 2 commits November 20, 2024 16:56

Configure ts module to ES2020

a79ab4a

Lazy load session replay

a2e770a

amortemousque changed the base branch from main to v6 November 20, 2024 16:19

amortemousque commented Nov 21, 2024

View reviewed changes

thomas-lebeau reviewed Nov 21, 2024

View reviewed changes

amortemousque added 3 commits November 22, 2024 16:29

Update test app target to fix ssr

a2ca8f0

Update the deploy script to deploy chunks

c6c30b2

Fix e2e by expose the recorder chunk in the e2e server app

2205ee9

amortemousque force-pushed the aymeric/lazy-load-session-replay branch from 04a8db5 to 2205ee9 Compare November 22, 2024 15:30

BenoitZugmeyer reviewed Nov 26, 2024

View reviewed changes

amortemousque mentioned this pull request Nov 26, 2024

✅ [RUM-6813] Use promise in collectAsyncCalls instead of a callback #3168

Merged

4 tasks

amortemousque added 3 commits November 26, 2024 17:29

Simplify asyncRunOnReadyState

f101a0f

Update chunks format

3e1bc2a

Upload chunks source map

04ff36b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚡️ [RUM-6813] Lazy load session replay #3152

⚡️ [RUM-6813] Lazy load session replay #3152

amortemousque commented Nov 20, 2024 •

edited

Loading

codecov-commenter commented Nov 21, 2024 •

edited

Loading

amortemousque Nov 21, 2024

thomas-lebeau Nov 21, 2024

amortemousque Nov 22, 2024

amortemousque Nov 21, 2024

cit-pr-commenter bot commented Nov 21, 2024

thomas-lebeau Nov 21, 2024

amortemousque Nov 22, 2024

BenoitZugmeyer Nov 26, 2024

thomas-lebeau Nov 21, 2024

thomas-lebeau Nov 21, 2024

amortemousque Nov 22, 2024

BenoitZugmeyer Nov 26, 2024

amortemousque Nov 26, 2024 •

edited

Loading

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

BenoitZugmeyer Nov 26, 2024

amortemousque Nov 26, 2024 •

edited

Loading

BenoitZugmeyer Nov 26, 2024

		@@ -0,0 +1,3 @@
		export function lazyLoadRecorder() {
		return import(/* webpackChunkName: "recorder" */ './startRecording').then((module) => module.startRecording)

	// Workaround because using resolveTo(startRecordingSpy) is not was not working
	// Workaround because using resolveTo(startRecordingSpy) was not working

⚡️ [RUM-6813] Lazy load session replay #3152

Are you sure you want to change the base?

⚡️ [RUM-6813] Lazy load session replay #3152

Conversation

amortemousque commented Nov 20, 2024 • edited Loading

Motivation

Changes

Testing

codecov-commenter commented Nov 21, 2024 • edited Loading

Codecov Report

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

cit-pr-commenter bot commented Nov 21, 2024

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

amortemousque Nov 26, 2024 • edited Loading

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 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amortemousque commented Nov 20, 2024 •

edited

Loading

codecov-commenter commented Nov 21, 2024 •

edited

Loading

amortemousque Nov 26, 2024 •

edited

Loading

amortemousque Nov 26, 2024 •

edited

Loading