-
Notifications
You must be signed in to change notification settings - Fork 216
Capture leading replay #1334
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
Capture leading replay #1334
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ export default class Recorder { | |
| } | ||
|
|
||
| /** | ||
| * Exports the recording span with all recorded events. | ||
| * Exports the recording span with all recorded events or events after a specific point. | ||
| * | ||
| * This method takes the recorder's stored events, creates a new span with the | ||
| * provided tracing context, attaches all events with their timestamps as span | ||
|
|
@@ -86,14 +86,14 @@ export default class Recorder { | |
| * | ||
| * @param {Object} tracing - The tracing system instance to create spans | ||
| * @param {Object} attributes - Attributes to add to the span | ||
| * (e.g., rollbar.replay.id, rollbar.occurrence.uuid) | ||
| * (e.g., rollbar.replay.id, rollbar.occurrence.uuid, rollbar.replay.type) | ||
| * @param {number} [afterCount=0] - If provided, only export events after this count (for leading replay) | ||
| */ | ||
| exportRecordingSpan(tracing, attributes = {}) { | ||
| const events = this._collectEvents(); | ||
| exportRecordingSpan(tracing, attributes = {}, afterCount = 0) { | ||
| const events = this._collectEvents(afterCount); | ||
|
|
||
|
matux marked this conversation as resolved.
|
||
| if (events.length < 3) { | ||
| // TODO(matux): improve how we consider a recording valid | ||
| throw new Error('Replay recording cannot have less than 3 events'); | ||
| if (events.length === 0) { | ||
| throw new Error('Replay recording has no events'); | ||
| } | ||
|
|
||
| const recordingSpan = tracing.startSpan('rrweb-replay-recording', {}); | ||
|
|
@@ -179,20 +179,34 @@ export default class Recorder { | |
| this._isReady = false; | ||
| } | ||
|
|
||
| _collectEvents() { | ||
| const events = this._events.previous.concat(this._events.current); | ||
| _collectEvents(afterCount = 0) { | ||
| const allEvents = this._events.previous.concat(this._events.current); | ||
|
|
||
| // Helps the application correctly align playback by adding a noop event | ||
| // to the end of the recording. | ||
| events.push({ | ||
| timestamp: Date.now(), | ||
| type: EventType.Custom, | ||
| data: { tag: 'replay.end', payload: {} }, | ||
| }); | ||
| const events = afterCount > 0 ? allEvents.slice(afterCount) : allEvents; | ||
|
|
||
| if (events.length > 0) { | ||
| // Helps the application correctly align playback by adding a noop event | ||
| // to the end of the recording. | ||
| events.push({ | ||
| timestamp: Date.now(), | ||
| type: EventType.Custom, | ||
| data: { tag: 'replay.end', payload: {} }, | ||
| }); | ||
| } | ||
|
matux marked this conversation as resolved.
|
||
|
|
||
| return events; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the current count of events in the buffers. | ||
| * This represents a marker for where trailing events end. | ||
| * | ||
| * @returns {number} The total number of events currently stored | ||
| */ | ||
| getCurrentEventCount() { | ||
| return this._events.previous.length + this._events.current.length; | ||
| } | ||
|
Comment on lines
+200
to
+208
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use the event count as a boundary marker rather than tracking the specific event. This has a fatal flaw, though: If during those N post-seconds we get a checkout from rrweb, we rotate the buffer and this number becomes invalid (out of bounds). I have a fix for this, but I'll send it in a subsequent PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Looking forward to the fix. We had talked about tracking both the buffer and the index in the buffer. |
||
|
|
||
| _logEvent(event, isCheckout) { | ||
| logger.log( | ||
| `Recorder: ${isCheckout ? 'checkout' : ''} event\n`, | ||
|
|
||
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.
Maybe
startIndexorstartPosition?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.
Since this is changing completely, can we leave it like this? So I can merge this and move on to the next PRs!
Then we circle back.