fix(session-replay-browser): Optimize function restart logic #1310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpollock-ampl
changed the title
jxiwang
approved these changes
Sep 29, 2025
lewgordon-amplitude
approved these changes
Sep 29, 2025
|
|
||
| // NOTE: If there is already an existing active recording, exit early unless forceRestart is true | ||
| if (this.recordCancelCallback && !forceRestart) { | ||
| this.loggerProvider.debug(`A capture is already in progress and forceRestart is false. Exiting.`); |
There was a problem hiding this comment.
Nit: Maybe instead of "Exiting" we could say "not restarting". I wouldn't want this to leak to users who made get the wrong impression.
There was a problem hiding this comment.
Thank you for the feedback - c81dab9
Comment on lines
558
to
559
| async recordEvents(recordEventsConfig?: { shouldLogMetadata?: boolean; forceRestart?: boolean }) { | ||
| const { shouldLogMetadata = true, forceRestart = true } = recordEventsConfig || {}; |
There was a problem hiding this comment.
I was thinking these could be non null since their defaults may not be obvious and maybe it's better to be more explicit here. It also saves a null check.
Suggested change
| async recordEvents(recordEventsConfig?: { shouldLogMetadata?: boolean; forceRestart?: boolean }) { | |
| const { shouldLogMetadata = true, forceRestart = true } = recordEventsConfig || {}; | |
| async recordEvents({shouldLogMetadata, forceRestart}: { shouldLogMetadata: boolean; forceRestart: boolean }) { |
There was a problem hiding this comment.
Great call - f75a512
| this.loggerProvider.debug(`A capture is already in progress and forceRestart is false. Exiting.`); | ||
| return; | ||
| } | ||
| this.loggerProvider.debug(`Starting new capture for session.`); |
There was a problem hiding this comment.
Minor nit: Maybe we should include the session replay ID or session ID? I think it wouldn't hurt and would potentially help with debugging.
There was a problem hiding this comment.
Done! c68312f
- @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR optimizes some of the logic around starting/restarting replay capture.
Previously, the following three actions would cause the replay to start or restart:
setSessionId())This PR stops the replay capture from being restarted when an event is emitted from the SDK.
How does this fix work?
The primary change is in
recordEvents():We will check for an existing callback and if the user does not pass forceRestart (default is
forceRestart=false), we will exit early and not re-create the SDK.In the case of "focus" or "session ID changes", we pass
forceRestart=true, to ensure that a new capture is created.Verification
Beyond unit tests, I created a test app and installed this version of the SDK. I was able to observe the behavior of the SDK on a focus event and then when sending 5 events via a button click:
The first 3 logs represent the "focus" event. The fourth log represents the button click.
Notes:
Given a call to
track()in your application, it goes through the Amplitude Analytics SDK's pipeline:For us, that means the plugin execution pipeline. So it goes through timeline.ts, which executes the various types of plugins:
Our plugin (the SR plugin) is an enrichment plugin (source), so its execute() method is called for every single event (source)
The execute() method calls:
The recordEvents() method:
Checklist