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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment on why we return true here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm let me double check the breadcrumbs, iirc we just display them all
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I'll add an _experiments
option for this, and will remove message/label for now!
packages/replay/src/replay.ts
Outdated
mutationsCount: count, | ||
}, | ||
}); | ||
this._createCustomBreadcrumb(breadcrumb); |
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 think this is a good start to see how often this happens
packages/replay/src/replay.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a bit more generic and use value
?
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.
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?
Updated this:
|
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.
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)
@@ -198,6 +198,23 @@ export class ReplayContainer implements ReplayContainerInterface { | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to import {mutationRecord} from '@sentry-internal/rrweb'
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.
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 😅
Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
This updates rrweb to 1.105.0, which fixes a bug and introduces
onMutation
option.We use this
onMutation
to create a breadcrumb when we encounter a large mutation. We can later use this to analyse how often this happens.