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

rfc(feature): Video replay envelope #129

Merged
merged 26 commits into from
Feb 9, 2024

Conversation

vaind
Copy link
Contributor

@vaind vaind commented Feb 6, 2024

How should Video-based replay behave:

  • sending to Sentry (SDK side of things)
  • processing for UI, if necessary
  • high-level overview of the UI integration (rrweb?)

Rendered RFC

closes #128

Relates to: getsentry/develop#1144

text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
vaind and others added 2 commits February 7, 2024 16:30
Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@romtsn
Copy link
Member

romtsn commented Feb 8, 2024

not sure if this should go into this PR, but what structure the video_url field would eventually be part of (for frontend)?

text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved

## Using a video, with EnvelopeItem:ReplayVideo

- From the SDK, we would send a new envelope with the following items: `Replay`, `ReplayVideo` and `ReplayRecording`.
Copy link
Member

Choose a reason for hiding this comment

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

@billyvg How would the player like to be notified that it should download video data? A type value on the replay? The video events in the RRWeb? Infer it from the replay's platform? Something else?

Copy link
Member

Choose a reason for hiding this comment

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

@cmanallen Thinking of using the rrweb video event


```json
{
"segment-id": 4,
Copy link
Member

@romtsn romtsn Feb 8, 2024

Choose a reason for hiding this comment

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

@cmanallen just to confirm: this segment_id and the ones in the Replay item and ReplayRecording's payload should be the same, correct? Shall we also document it here for transparency?

Copy link
Member

Choose a reason for hiding this comment

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

They should be the same but if you sent a segment 2 video with a segment 3 rrweb event nothing would break. So long as the rrweb video event on segment 2 accurately reflects the segment 2 video. The events are not dependent on one another for processing. They are just associated to the same "segment_id" metadata.

Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

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

This looks good. So long as the player is satisfied I'm happy with this design. I would give @billyvg final sign off.

text/0129-video-replay-envelope.md Outdated Show resolved Hide resolved
@vaind vaind marked this pull request as ready for review February 9, 2024 13:36
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

lgtm

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@vaind
Copy link
Contributor Author

vaind commented Feb 9, 2024

@romtsn I think you're the last one to approve this, let me know if there're more changes you'd like to see, otherwise it's good to go

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 maybe @brustolin could take a look on Monday, but we can merge now

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.

RFC for Session Replay based on video snippets
6 participants