Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Nov 25, 2022

In replay, we attach errors that happened during replay-time via a global event handler.
However, due to various reasons some of these errors may be filtered out later. This leads to a weird user experience when there seem to be more errors that actually exist in sentry.

This PR fixes this (to a decent extend) by making sure we remove any error id that was actually dropped (due to rate limiting or because an integration/callback decided to drop it). For this, we monkey patch recordDroppedEvent.

Closes #6285

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Nov 25, 2022
@mydea mydea requested review from a team and Lms24 November 25, 2022 08:42
@mydea mydea self-assigned this Nov 25, 2022
@mydea mydea requested review from lobsterkatie and removed request for a team November 25, 2022 08:42
@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.6 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.69 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.37 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 54.26 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.13 KB (0%)
@sentry/browser - Webpack (minified) 65.83 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.16 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.96 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.44 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.87 KB (+0.02% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 41.74 KB (+0.33% 🔺)
@sentry/replay - Webpack (gzipped + minified) 37.23 KB (+0.5% 🔺)

@mydea mydea force-pushed the fn/replay-remove-dropped-events branch 3 times, most recently from 30619a0 to 93feea9 Compare November 28, 2022 10:07
@mydea mydea requested a review from billyvg November 28, 2022 12:37
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.

Sorry I'm on mobile atm, lmk if something is not clear and I can add a better explanation.

/**
* We overwrite `client.recordDroppedEvent`, but store the original method here so we can restore it.
*/
private _originalRecordDroppedEvent?: Client['recordDroppedEvent'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been omitting the underscore prefix with private members since it's marked as private already but since this seems to be a pattern in js sdk, maybe we should update the replay sdk to follow this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is def. part of the todo list! :)

@mydea mydea force-pushed the fn/replay-remove-dropped-events branch 9 times, most recently from eb289af to 553b49c Compare December 2, 2022 14:02
@mydea mydea force-pushed the fn/replay-remove-dropped-events branch from 553b49c to e99110c Compare December 2, 2022 14:19
@mydea mydea merged commit 682efa5 into master Dec 2, 2022
@mydea mydea deleted the fn/replay-remove-dropped-events branch December 2, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Package: replay Issues related to the Sentry Replay SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure replays do not have filtered out errors attached

4 participants