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

perf(breadcrumbs): use unix epoch for timestamps #1525

Merged
merged 2 commits into from
Nov 25, 2021

Conversation

kattrali
Copy link
Contributor

Goal

Improve performance on the breadcrumb storage "hot path" by removing Date formatting. Adds support for millisecond time resolution as a bonus.

Design

Created an encoding for timestamps where the date value is sent using the pattern t[0-9]+, where the number is the count of milliseconds since the epoch and the t indicates that timestamp parsing should be used. The breadcrumb added state event serializes the timestamp into this format, which is what is now persisted to disk in the event of a crash.

Changeset

  • Added a new utility function to decode milliseconds from a string into ISO8601 timestamps into a pre-allocated buffer. Kept this in the same file as breadcrumb serialization to avoid exporting an extra symbol or defining the function body in a header, which is not a pattern we currently have. In the event that decoding fails, the original string is then sent as the breadcrumb timestamp.

Questions

What would be the best way to express optimization changes as a part of a PR? We haven't done a lot of perf-specific improvements and we probably need some standardization here. For example, I can diff the bugsnag-benchmarks entry for simple and complex breadcrumbs and include that in the description?

A flame graph of the breadcrumb path is basically the same as before, just chopping all of the date parsing logic from the top. 🪓

Testing

  • Altered a unit test to check both the old and new storage formats are decoded correctly
  • All e2e tests go through this path now and should get valid timestamps

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

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