-
Notifications
You must be signed in to change notification settings - Fork 95
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
Consolidate honeycomb metrics to use single lock & fix concurrent read/write #511
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one small change for the read unlock
Co-authored-by: Vera Reynolds <vreynolds@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 🎇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I left a few tiny suggestions as I went through, mostly just for clarification 👍
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
…d/write (honeycombio#511) Co-authored-by: Vera Reynolds <vreynolds@users.noreply.github.com> Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Which problem is this PR solving?
Honeycomb metrics currently utilises 3 locks, one for each map of counters, gauges and histograms and is unnecessary. In addition, the locks were only used when registering each metric and collecting metrics values - they were missing the lock to retrieve an entry for updating which risks a read / write error.
This PR updates honeycomb metrics to utilise a single lock that is used to control all three types of metrics and is also used when attempting to retrieve a metric from a map.
Short description of the changes