-
Notifications
You must be signed in to change notification settings - Fork 80
Feature/Session Replay: 'rrweb' integrated into as SessionReplayManager #674
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
base: main
Are you sure you want to change the base?
Conversation
Hey, are you planning on releasing this change? If not, you should target this PR to a dev branch |
// Define the eventWithTime interface based on how it's used in the code | ||
export interface eventWithTime { | ||
type: number; | ||
data: { |
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.
Is it possible to be more specific with the schema? What schema does rrweb use?
this.events.push(event); | ||
|
||
if (this.events.length >= this.BATCH_SIZE) { | ||
this.flushEvents(); |
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.
Reaching the batch limit seems to be the primary way we are flushing the session replay events.
Is it possible to improve how frequently we are reporting this, such as on an interval?
What happens to the remaining replay events when the window is closed?
} | ||
} catch (error) { | ||
// If recording fails, add the events back to be retried later | ||
this.events = [...eventsToSend, ...this.events]; |
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.
On the backend, are you enforcing a limit on number of events within a single SessionReplayEvent? If so, you'll go over the limit with this approach
import * as rrweb from 'rrweb'; | ||
// Define the eventWithTime interface based on how it's used in the code | ||
export interface eventWithTime { | ||
type: number; |
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.
What is type? Is it an index?
// Default behavior - send to RUM service | ||
this.context.record(SESSION_REPLAY_EVENT_TYPE, { | ||
events: eventsToSend, | ||
sessionId: this.session?.sessionId |
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.
sessionId is already in the metadata
Approach looks good overall, you'll need smoke and integ tests though Also try running webpack bundle analyzer to see how many bytes this adds |
[key: string]: any; | ||
} | ||
import { InternalPlugin } from '../InternalPlugin'; | ||
import { Session } from '../../sessions/SessionManager'; |
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.
SessionReplayPlugin created with corresponding unit tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.