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

Use ring buffer to store breadcrumbs #1286

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Jun 22, 2021

Goal

Breadcrumbs are stored in a ConcurrentLinkedQueue and then trimmed as they are added. This was identified as an area where performance could potentially be improved as over an application's lifecycle thousands of breadcrumbs might be added to Bugsnag.

Changeset

BreadcrumbState has been altered to use a ring buffer instead of a ConcurrentLinkedQueue. Every time a breadcrumb is added an index is incremented by one, and the breadcrumb is stored in a backing array. If the number of breadcrumbs logged is greater than config.maxBreadcrumbs then the implementation wraps around to zero and overwrites the oldest breadcrumb added.

When an error is captured breadcrumbs must be added to an Event in the order in which they were captured. Previously this was a case of transforming a Queue into a List but using a ring buffer means that the breadcrumbs can be out of order. The solution chosen here is to create two sublists based on the position of the most recent breadcrumb and then combine them together.

Performance improvements

Adding 1000 breadcrumbs to the store took ~9ms with these changes, and ~56ms without, a difference of ~45ms.

The BreadcrumbState#copy() implementation has added on ~0.5ms to the Bugsnag.notify() call.

changes_add_crumbs

baseline_crumbs

Tradeoffs of this changeset

There are a couple of tradeoffs made with this changeset:

  • BreadcrumbState#copy() does not sort elements or use synchronisation, so in rare cases this could lead to some breadcrumbs being ordered incorrectly. Sorting the collection was profiled as being expensive, but this is something that could potentially be added in if needed.
  • This increases the time taken to execute Bugsnag.notify(), by the equivalent of adding ~11 breadcrumbs. This feels like a reasonable trade off as breadcrumbs should typically be added much more frequently than Bugsnag.notify() is called.

Testing

Updated existing test coverage to match new method signatures, and added additional unit test cases for the ring buffer wrapping round. Additionally added a benchmark test for copying breadcrumbs (which happens whenever an event is generated).

@fractalwrench fractalwrench force-pushed the PLAT-6755/breadcrumb-ring-buffer branch from d8ddbdd to 99fe79b Compare June 22, 2021 14:13
@fractalwrench fractalwrench requested a review from lemnik June 22, 2021 14:32
@fractalwrench fractalwrench marked this pull request as ready for review June 22, 2021 14:32
@fractalwrench fractalwrench marked this pull request as draft June 23, 2021 11:04
@fractalwrench fractalwrench force-pushed the PLAT-6755/breadcrumb-ring-buffer branch from 99fe79b to e686ebe Compare June 23, 2021 14:01
@fractalwrench fractalwrench marked this pull request as ready for review June 23, 2021 14:09
@fractalwrench fractalwrench force-pushed the PLAT-6755/breadcrumb-ring-buffer branch from e686ebe to c98490c Compare June 23, 2021 14:17
Copy link
Contributor

@lemnik lemnik left a comment

Choose a reason for hiding this comment

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

LGTM

@fractalwrench fractalwrench requested a review from kstenerud June 24, 2021 10:01
@fractalwrench
Copy link
Contributor Author

@kstenerud would you mind giving a secondary review for this changeset, as it has altered the core breadcrumb functionality?

@fractalwrench fractalwrench force-pushed the PLAT-6755/breadcrumb-ring-buffer branch from c98490c to 308025c Compare June 24, 2021 15:05
@fractalwrench fractalwrench merged commit 3014cc6 into next Jun 25, 2021
@fractalwrench fractalwrench deleted the PLAT-6755/breadcrumb-ring-buffer branch June 25, 2021 08:18
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.

3 participants