Skip to content

feat(replay): Add "start recording" breadcrumb to replays #7004

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

Closed
wants to merge 3 commits into from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 31, 2023

This PR adds a breadcrumb to replay, whenever a full checkout occurs and we're in session mode. The purpose of this breadcrumb (as explained in #6928) is that we need to be aware of page reloads and multi-page app navigations (i.e. page loads). Therefore, this new breadcrumb contains a url string in its data object, which contains the current full URL.

Initially, I wanted to add it in startRecording, which however, caused weird behaviour in error mode, causing too early flushes of the initial segment (at least according to the weird test failures I got). So for now, this breadcrumb is always added for session-mode and for error-mode replays only when the actual error is caught and we switch to session mode. I'm not too happy about this, as we do not get this breadcrumb for the initial 30-60 seconds of the replay but not sure if we can actually do this, without rewriting how adding breadcrumbs actually works in the SDK. Open for suggestions if anyone has ideas.

I'm not sure if category: 'replay.recording.start' is set correctly but it seems like we already do some processing for it to transform the name in the UI (cc @ryan953). Happy to change if if necessary.

closes #6928

@Lms24 Lms24 requested review from billyvg, ryan953 and mydea January 31, 2023 16:33
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.87 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.54 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.29 KB (0%)
@sentry/browser - Webpack (minified) 66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.08 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 44.2 KB (+0.07% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.96 KB (+0.1% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.56 KB (+0.07% 🔺)

@billyvg
Copy link
Member

billyvg commented Jan 31, 2023

For error sessions (assuming we can get what you're intending to do to work), this would mean we would start recording at time 0, and say the error happens 4 minutes later, we would have a gap of 4 minutes. Is this what you want to happen? -- I also just thought of an unrelated, potential bug and/org unexpected behavior here for error sessions... if we have an error occur outside of either session idle time and/or max session time, we may end up dropping the replay completely due to session life checks.

The breadcrumb shouldn't be causing it to flush as addUpdate does not flush in error recording mode.

Other questions: should we also add breadcrumb for stop recording? Also might be good (especially for debug purposes) if we had a breadcrumb when a session is expired and a new session is created (not when it's a completely new session).

@Lms24
Copy link
Member Author

Lms24 commented Feb 1, 2023

For error sessions (assuming we can get what you're intending to do to work), this would mean we would start recording at time 0, and say the error happens 4 minutes later, we would have a gap of 4 minutes. Is this what you want to happen?

I was also thinking about that. I don't want this to happen as it wouldn't be helpful at all :D Making this breadcrumb appear at the beginning of the last checkout we keep before the error, but not more than once, is probably quite tricky. Which is why I'd opt to just leave it out for error sessions for the time being... I guess the biggest problem here is that users are confused if they get a breadcrumb amidst the replay that says something like "start recording".

The idea behind #6928 is to have some way of telling when e.g. a page (re)load happened. The breadcrumb is just the way we figured we could send this information. We also thought about adding this breadcrumb right at the start when the ReplayContainer contructor is called. However this would mean that we're recording a breadcrumb before we're even starting the recording. Not sure if this would even work.

The breadcrumb shouldn't be causing it to flush as addUpdate does not flush in error recording mode.

Right, I also saw this. Maybe the problem is in the error tests then but whenever I added this breadcrumb after an error mode checkout, (i.e. basically just leave out the if condition guarding the _addStartRecordingBreadcrumb) the tests would fail because the checkout that previously happened in segment 0 would for some reason be moved from to segment 1. Really weird behaviour...

should we also add breadcrumb for stop recording?

I think that's tricky because it can't be sent reliably. For instance in rate-limiting/http error responses, when we know we need to stop recording, it's already too late to send that breadcrumb. I guess the same applies to expired sessions in a way, right?

@mydea
Copy link
Member

mydea commented Feb 1, 2023

Another idea (just brainstorming here, really):

What about adding this breadcrumb when we do the initial flush (segmentId === 0)? Something along this line could work, not sure if worth it:

  • On each checkout event (or the initial event, which confusingly is not a checkout), store the current URL somewhere
  • When we flush the initial segment (id === 0), we retrospectively add a breadcrumb with the timestamp of the first event of the segment, and the cached URL.

I guess this would be the most correct solution, and work for both regular and error sessions...? But it's probably not super straightforward 😬

@Lms24
Copy link
Member Author

Lms24 commented Feb 1, 2023

I like the idea. Can definitely try it!

and work for both regular and error sessions

Just to confirm, the first segment we send after an error was thrown (i.e. the last ~60s that lead up to the error) would then get segmentId: 0, right?

@billyvg
Copy link
Member

billyvg commented Feb 1, 2023

Can we hold off on this for now? #7029 probably fixes the root cause -- we may still want something in the case where a new session gets started due to expiry/idle.

@Lms24
Copy link
Member Author

Lms24 commented Feb 1, 2023

Oh damn, it really does: https://sentry.io/organizations/sentry-sdks/replays/sentry-javascript-browser:3c4865bbaa3246f883366272c73b11cd/?project=5429213&query=&referrer=%2Forganizations%2F%3AorgId%2Freplays%2F&statsPeriod=14d&yAxis=count%28%29

Didn't know that the performance observer tracks pageloads in that way. So yes, then let's close this mess in favour of the good solution :D Thanks @billyvg!

@Lms24 Lms24 closed this Feb 1, 2023
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: No navigation or page load breadcrumb on post backs
3 participants