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

fix: account for negative timestamps from rrweb #17849

Merged
merged 10 commits into from
Oct 11, 2023

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 6, 2023

We increasingly use the contents of rrweb events during ingestion so we need stricter validation...

We recently saw an issue caused by having received rrweb events from a client that had negative timestamps 🙃

This change ensures that timestamps we lift from rrweb events to create replay events are positive and result in a valid luxon datetime

@daibhin daibhin marked this pull request as ready for review October 6, 2023 16:37
@daibhin daibhin requested a review from a team October 6, 2023 16:38
@posthog-bot
Copy link
Contributor

Hey @daibhin! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

@daibhin daibhin requested a review from pauldambra October 9, 2023 12:42
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

Thoughts on how we could level up the validation even more but not blocking since this is better already

plugin-server/src/worker/ingestion/process-event.ts Outdated Show resolved Hide resolved
@daibhin daibhin merged commit 5e0eb33 into master Oct 11, 2023
71 checks passed
@daibhin daibhin deleted the dn-fix/negative-rrweb-timestamp branch October 11, 2023 12:44
@daibhin
Copy link
Contributor Author

daibhin commented Oct 11, 2023

@pauldambra I didn't exactly know how to do it but another quick win would be to catch any invalid parameter and send it to Sentry. Debugging via logs / Kafka messages is always needle in a haystack kind of work

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.

3 participants