Uh oh!
There was an error while loading. Please reload this page.
feat(replay): Update rrweb to 1.105.0 & add breadcrumb when encountering large mutation - #7314
Conversation
size-limit report 📦
|
| }); | ||
| this._createCustomBreadcrumb(breadcrumb); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Maybe add a comment on why we return true here?
There was a problem hiding this comment.
Just to be sure, will this be shown in the UI automatically or does that need changes in the UI to be reflected?
Do we think the category & message is fine? I basically just freestyled this xD
There was a problem hiding this comment.
Hmm let me double check the breadcrumbs, iirc we just display them all
There was a problem hiding this comment.
OK so we have 2 options here: 1) we let UI handle the messaging, or 2) we let SDK handle messaging.
The logic goes like this... if we have data.label, it will use that as a title, otherwise it will split the category on . and display that as title. Likewise for the body, it will use message if data does not exist, otherwise it will stringify data.
Let's make this a bit more dumb and have the UI determine how it's displayed (since it's easier for us to update the UI). This also means we will need to make the UI update first.
I'm also thinking if we should only display these crumbs (in the UI) for Sentry internal users only at first - I'm worried this breadcrumb may confuse users?
There was a problem hiding this comment.
I'm also thinking if we should only display these crumbs (in the UI) for Sentry internal users only at first - I'm worried this breadcrumb may confuse users?
If we limit it to sentry-internal, let's at least give users a way to opt-in (_experiments ?). Maybe someone in #6946 could give it a try and tell us if these crumbs show up in performance-impacted replays. This way we'd have a good indication that we're on the right track to solving this problem.
There was a problem hiding this comment.
Good point - I'll add an _experiments option for this, and will remove message/label for now!
| mutationsCount: count, | ||
| }, | ||
| }); | ||
| this._createCustomBreadcrumb(breadcrumb); |
There was a problem hiding this comment.
I think this is a good start to see how often this happens
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| category: 'replay.mutations', | ||
| message: `A mutation with ${count} changes was recorded, which may indicate slow performance.`, | ||
| data: { | ||
| mutationsCount: count, |
There was a problem hiding this comment.
Let's make this a bit more generic and use value?
There was a problem hiding this comment.
not sure but may be too generic? As what does value mean when looking at replay.mutations 😅 I could make it just count or length maybe?
mydea
commented
Mar 3, 2023
Updated this:
|
billyvg
left a comment
There was a problem hiding this comment.
This seems good to me to start with. I think what we'll want in the future are metrics (i.e. what we'll be doing to support logging compression mode)
| // instead, we'll always keep the last 60 seconds of replay before an error happened | ||
| ...(this.recordingMode === 'error' && { checkoutEveryNms: ERROR_CHECKOUT_TIME }), | ||
| emit: this._handleRecordingEmit, | ||
| onMutation: (mutations: unknown[]) => { |
There was a problem hiding this comment.
I think you should be able to import {mutationRecord} from '@sentry-internal/rrweb'
There was a problem hiding this comment.
IMHO we can leave this as unknown as we really don't care about what type of array that is (for now), one less import to care about 😅
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
This updates rrweb to 1.105.0, which fixes a bug and introduces
onMutationoption.We use this
onMutationto create a breadcrumb when we encounter a large mutation. We can later use this to analyse how often this happens.