-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(capture-rs): implement replay capture in capture-rs #24461
Conversation
Hey @frankh! 👋 |
2c72574
to
e349a3c
Compare
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.
This is a much smaller change than I expected tbh, all looks good to me aside from 1 question about where a metric is reported
c12594a
to
f0998a9
Compare
f0998a9
to
de856e7
Compare
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.
Only one note, a bit of a security/stability issue. As a general rule, you should never unwrap()
in any "real" code path, only tests, without being absolutely certain the thing being unwrapped will be what you expect (and usually the thing that makes you certain of that also lets you avoid unwrap()
)
Problem
capture is fragmented
django capture for recordings is flaky
lets move everything to rust
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?
added unit tests