getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.87k stars 1.55k forks source link

Update max. breadcrumb handling on scope #12399

Open mydea opened 3 months ago

mydea commented 3 months ago

Today, we have a maxBreadcrumbs option on the client. However, due to the way when/how scopes are created (=before the client), plus the fact that a scope may not have a client directly attached to it, we have currently implemented it in a way that we always pass maxBreadcrumbs as a second argument to scope.addBreadcrumb(breadcrumb, maxBreadcrumbs).

This is kind of weird for multiple reasons (it's already weird to have this argument on that API), but especially because you can change the max. size with each addBreadcrumb() call, which is not really ergonomic.

Additionally, it may make sense and be more performant to rewrite this to use a ringbuffer instead - today, we keep cloning the breadcrumbs array every time once you hit the max. breadcrumbs threshold. However, using a ringbuffer with the current implementation is tricky because the size is technically dynamic and handling this becomes quite cumbersome and adds a decent amount of bundle size.

We should revisit if we can find a way to handle this better in the future.

timfish commented 3 months ago

Additionally there is the issue that it's currently max breadcrumbs per scope.

With current, isolation and global scopes merged, potentially you could end up sending maxBreadcrumbs * 3

mydea commented 3 months ago

Yeah, that too! I think this is "OK", as we generally mostly will have breadcrumbs on the isolation scope only (unless a user really manually adds breadcrumbs somewhere else, and even then, there is still an upper limit).