Skip to content

fix(replay): Ignore old events when manually starting replay #12349

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

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Jun 4, 2024

This PR ensures that if a replay is manually started (=no sample rates are defined at all, and a user later calls start() or startBuffering(), we do not back-port the initialTimestamp of the replay based on the event buffer.

By default (for the first segment) we'll backport the initialTimestamp to the time of the first event in the event buffer, to ensure that e.g. the pageload browser metrics that may be emitted with an earlier timestamp all show up correctly.

However, this may be unexpected if manually calling startBuffering() and seeing things for stuff that happened before.
Now, we keep track of this and adjust the behavior accordingly.

Fixes #11984

@mydea mydea requested a review from billyvg June 4, 2024 12:23
@mydea mydea self-assigned this Jun 4, 2024
@mydea mydea requested a review from a team as a code owner June 4, 2024 12:23
Copy link
Contributor

github-actions bot commented Jun 4, 2024

size-limit report 📦

Path Size
@sentry/browser 21.92 KB (+0.57% 🔺)
@sentry/browser (incl. Tracing) 32.97 KB (+0.39% 🔺)
@sentry/browser (incl. Tracing, Replay) 68.57 KB (+0.22% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.86 KB (+0.24% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.62 KB (+0.21% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.74 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 86.58 KB (+0.18% 🔺)
@sentry/browser (incl. metrics) 26.11 KB (+0.47% 🔺)
@sentry/browser (incl. Feedback) 38.09 KB (+0.34% 🔺)
@sentry/browser (incl. sendFeedback) 26.51 KB (+0.48% 🔺)
@sentry/browser (incl. FeedbackAsync) 31.06 KB (+0.41% 🔺)
@sentry/react 24.69 KB (+0.5% 🔺)
@sentry/react (incl. Tracing) 36 KB (+0.33% 🔺)
@sentry/vue 25.93 KB (+0.48% 🔺)
@sentry/vue (incl. Tracing) 34.8 KB (+0.36% 🔺)
@sentry/svelte 22.06 KB (+0.57% 🔺)
CDN Bundle 23.3 KB (+0.58% 🔺)
CDN Bundle (incl. Tracing) 34.68 KB (+0.36% 🔺)
CDN Bundle (incl. Tracing, Replay) 68.63 KB (+0.21% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 73.78 KB (+0.19% 🔺)
CDN Bundle - uncompressed 68.42 KB (+0.38% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 102.6 KB (+0.25% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 212.52 KB (+0.14% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 224.99 KB (+0.13% 🔺)
@sentry/nextjs (client) 35.37 KB (+0.4% 🔺)
@sentry/sveltekit (client) 33.6 KB (+0.41% 🔺)
@sentry/node 115.59 KB (+0.1% 🔺)
@sentry/node - without tracing 94.68 KB (+0.12% 🔺)
@sentry/aws-serverless 103.87 KB (+0.12% 🔺)

@c298lee
Copy link
Contributor

c298lee commented Jun 4, 2024

What happens if there is a sample rate and the user calls start() or startBuffering()? Is that a case we need to consider?

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.

A lot simpler fix than I figured, amaze!

Co-authored-by: Catherine Lee <55311782+c298lee@users.noreply.github.com>
@mydea
Copy link
Member Author

mydea commented Jun 6, 2024

What happens if there is a sample rate and the user calls start() or startBuffering()? Is that a case we need to consider?

If you call start() (or startBuffering()) when replay is already recording, we throw an error!

@mydea mydea enabled auto-merge (squash) June 6, 2024 12:27
@mydea mydea merged commit 4009e3f into develop Jun 6, 2024
79 checks passed
@mydea mydea deleted the fn/replay-before-start branch June 6, 2024 12:38
billyvg pushed a commit that referenced this pull request Jun 10, 2024
This PR ensures that if a replay is manually started (=no sample rates
are defined at all, and a user later calls `start()` or
`startBuffering()`, we do not back-port the `initialTimestamp` of the
replay based on the event buffer.

By default (for the first segment) we'll backport the `initialTimestamp`
to the time of the first event in the event buffer, to ensure that e.g.
the pageload browser metrics that may be emitted with an earlier
timestamp all show up correctly.

However, this may be unexpected if manually calling `startBuffering()`
and seeing things for stuff that happened before.
Now, we keep track of this and adjust the behavior accordingly.

Fixes #11984

---------

Co-authored-by: Catherine Lee <55311782+c298lee@users.noreply.github.com>
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.

Replay start() can include old "history" events that are not relevant to the replay
3 participants