Skip to content
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(session replay): delay compresion #885

Merged
merged 10 commits into from
Sep 24, 2024
Merged

feat(session replay): delay compresion #885

merged 10 commits into from
Sep 24, 2024

Conversation

jxiwang
Copy link

@jxiwang jxiwang commented Sep 23, 2024

Summary

Delays the compression of events emitted by rrrweb record using requestIdleCallback.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@jxiwang jxiwang changed the base branch from main to v1.x September 23, 2024 06:41
@jxiwang jxiwang changed the title Amp 110166 feat(session replay): delay compresion Sep 23, 2024
@jxiwang jxiwang marked this pull request as ready for review September 23, 2024 14:25
Copy link

@lewgordon-amplitude lewgordon-amplitude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool. I had no idea about requestIdleCallback before the PR, it seems like a good use case for this. I'm wondering though if we want to change this a little bit. The things that come to mind for me:

  1. The hard coded timeout seems like it'll fire a lot of events just slightly delayed. Therefore not immediately impacting performance, but just having it delayed. I think it being hard coded is to going to be slightly problematic since that number will vary from app to app.
  2. It seems like we're creating a new callback for each event. I'm not entirely certain it's a problem, but for a high number of events this could be a lot of callbacks.

I'm thinking to help mitigate some of these things maybe we could instead of a queue that we work through during idle periods and get through as many as possible. We could probably use the IdleDeadline object to know when to quit. The idea being that we register a single callback that processes through the queue as much as possible during idle periods. Getting this exactly right and not going over time may be tough, but I think this would be more performant.

I think in either case before we merge this we should add an option so that we can test this ourselves for now and make sure we add some performance charts with a sample app or preferably our site so we're confident in the change.

I'm definitely open to feedback/other thoughts on this!

packages/session-replay-browser/src/session-replay.ts Outdated Show resolved Hide resolved
packages/session-replay-browser/src/session-replay.ts Outdated Show resolved Hide resolved
packages/session-replay-browser/src/session-replay.ts Outdated Show resolved Hide resolved
packages/session-replay-browser/src/session-replay.ts Outdated Show resolved Hide resolved
@jxiwang
Copy link
Author

jxiwang commented Sep 23, 2024

@lewgordon-amplitude

Thanks for the feedback!

  1. For the timeout, I considered not including one but am also concerned with the case that the compression doesn't get fired. Even if the timeout is reached, the compression should get pushed to the back of the event queue in cases like when the page takes > 2s to load. Although I'm not entirely sure that doesn't have performance implications still. I could remove the timeout, and we could monitor the effects.

  2. Good call on the callbacks being generated per event. I'll add an event queue based on the timeRemaining, and I actually did not know idleDeadline could provide that information. Pretty cool that it does do that!

Agreed on adding an option so we can have control over testing this, and we can let customers know this is something they could turn on.

Copy link

@lewgordon-amplitude lewgordon-amplitude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think there are minor changes we can make, but with the config in place and the current approach I don't see anything majorly blocking here.

@jxiwang jxiwang merged commit 278115c into v1.x Sep 24, 2024
3 checks passed
@jxiwang jxiwang deleted the AMP-110166 branch September 24, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants