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.

Sign up for GitHub

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

Jump to bottom

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

Closed
billyvg opened this issue May 10, 2024 · 3 comments · Fixed by #12349
Closed

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

billyvg opened this issue May 10, 2024 · 3 comments · Fixed by #12349
Assignees
@mydea

Comments

@billyvg
Copy link
Member

billyvg commented May 10, 2024 •
edited
Loading

When calling start(), we always include a breadcrumb for the initial page navigation. This should not always be included and will cause the replay to appear to start earlier than when the public start() function is actually caused.

To repro:

  • set sample rates to 0 (we want manual recording)
  • reload page (this will create the navigation.reload breadcrumb)
  • wait seconds
  • observe current time and then call replay.start()
  • console.log something so that it gets record to replay

You'll see that we get a navigation breadcrumb at time = 0 and the console.log happens at time = x
@mydea
Copy link
Member

mydea commented May 13, 2024

I am not sure I understand this 🤔 what exactly is happening/should not be happening? We do not add a breadcrumb for start, I believe...? Or at least I do not know which breadcrumb this is refering to 😅

@billyvg
Copy link
Member Author

billyvg commented May 13, 2024

I think it's the handleHistorySpanListener -- basically what happens is that we call start() seconds after pageload, we will have breadcrumbs from around pageload time. In the replay it'll look like we have nothing happening for seconds until the time we actually call start.

I haven't verified this closely yet, but this seems like what is happening.

@mydea mydea mentioned this issue Jun 4, 2024
Merged
mydea added a commit that referenced this issue Jun 6, 2024
@mydea @c298lee
fix(replay): Ignore old events when manually starting replay (#12349)
4009e3f 
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>
@AbhiPrasad
Copy link
Member

Released with https://github.com/getsentry/sentry-javascript/releases/tag/8.8.0

billyvg pushed a commit that referenced this issue Jun 10, 2024
@mydea @c298lee @billyvg
fix(replay): Ignore old events when manually starting replay (#12349)
642b2da 
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
Assignees

@mydea mydea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(replay): Ignore old events when manually starting replay getsentry/sentry-javascript
3 participants
@billyvg @mydea @AbhiPrasad