-
Notifications
You must be signed in to change notification settings - Fork 125
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: safer custom event on return from idle #913
Conversation
Size Change: +824 B (0%) Total Size: 744 kB
ℹ️ View Unchanged
|
let tries = 0 | ||
const retryingFn = () => { | ||
tries++ | ||
if (tries > 3) { | ||
logger.warn('Could not take full snapshot after 150 milliseconds. Giving up.') | ||
return | ||
} | ||
|
||
try { | ||
if (withCustomEvent) { | ||
this.rrwebRecord?.addCustomEvent(withCustomEvent.tag, withCustomEvent.payload) | ||
} | ||
this.rrwebRecord?.takeFullSnapshot() | ||
} catch (e) { | ||
setTimeout(retryingFn, 50) | ||
} | ||
} | ||
retryingFn() |
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.
I don't get this change... If the recorder hasn't started - that's okay right? We wouldn't take a full snapshot, as the starting of the recorder will take the snapshot.
It feels unnecessary and the real fix is in having the customEvent come after the _captureStarted
check?
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.
Oh, that's where I was heading, but we already had take full snapshot wrapped in a check to swallow failing to take one. So, the code was written to drop the request for a full snapshot. Which feels wrong
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.
and this is on return from idle so everything should already be running
but, let's swallow the error the same way and get the error out of sentry. it's only a debug signal
https://posthog.sentry.io/issues/4662102372/?project=1899813&query=is%3Aunresolved+please+add+custom+event+after+start+recording+PostHog-Recording-URL%3A%22https%3A%2F%2Fapp.posthog.com%2Freplay%2F018c1241-c72e-7c77-855b-e0bd29823f63%3Ft%3D756%22&referrer=issue-stream&statsPeriod=24h&stream_index=0
We see an error where setting a custom event on the session on return from idle is failing because the rrweb recording isn't ready to accept events.
Immediately after that we try to take a full snapshot and swallow any errors
But the full snapshot is relatively important.
Let's make those activities one activity and add a simple retry.