Skip to content
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

Remove in-memory breadcrumbs #861

Merged

Conversation

nickdowell
Copy link
Contributor

Goal

This reduces memory consumption by removing the in-memory copy of breadcrumbs.

Design

Breadcrumbs are now loaded from disk whenever access is required.

As a side-effect this removes the possibility of crashes due to multithreaded access to breadcrumbs.

Testing

This behaviour is covered by unit tests (which required some modifications) and E2E test.

Verified that crash reports in the dashboard still contain the expected breadcrumbs.

Verified that breadcrumb modifications in an OnSendErrorBlock still alter what is sent to the dashboard.

@sethfri
Copy link

sethfri commented Oct 26, 2020

👋 Customer question if you don't mind: How does this impact performance for reading/writing breadcrumbs? Why was the decision made to remove the in-memory version rather than synchronizing access to the resource?

@nickdowell nickdowell merged commit 4699107 into integration/breadcrumbs Oct 27, 2020
@nickdowell nickdowell deleted the feature/remove-breadcrumbs-in-memory branch October 27, 2020 08:07
@nickdowell
Copy link
Contributor Author

Hi @sethfri this change reduces the memory overhead of writing breadcrumbs, and other related changes have significantly reduced the CPU overhead of doing so. It will now be slower to read the breadcrumbs, but since writes are much more common than reads it makes sense to optimize it this way round.

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.

4 participants