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.

Sign up for GitHub

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

Jump to bottom

Fix unbuffered batch metrics send #128

Merged
merged 3 commits into from
Oct 7, 2018
Merged

Fix unbuffered batch metrics send #128

merged 3 commits into from
Oct 7, 2018

Conversation

martincostello
Copy link
Member

When I added more integration tests in #127, it found a bug where multiple metrics being sent at once using the "old" implementation (the existing one at the time of #104) would be malformed and dropped when sent to statsd.

This PR fixes that by making the following changes and optimisations uncovered while debugging the failing tests:

  1. Fix malformed metrics by sending multiple metrics as different sends, not one send with everything concatenated together. It may be that this is allowed, but I couldn't find what the delimiter would be in that scenario, so I just switched to a foreach liked the buffered implementation does.
  2. Add optimisations to not send empty strings to statsd if the sample rate means that none of the metrics need to be sent.
  3. Add null and zero check for buckets array to fix potential for a NullReferenceException as well as removing the need to allocate an enumerator.

I guess that we should include this with #124 and do a 3.2.2 release with the fix?

@martincostello
Add failing unit tests
25ee74b 
Add failing unit tests for sending multiple counters at once when buffers are not in use.
@martincostello
Fix and optimise for multiple metric sends
56dc7c2 
Fix malformed metrics being sent to statsd when buffers are not used, which caused stats to be dropped, due to incorrect concatenation being used.
Add optimisations to not send empty strings to statsd.
Add null check for buckets array.
@martincostello
Minor formatting changes
baeb06b 
Tidy-up formatting slightly.
@martincostello martincostello requested a review from a team as a code owner October 7, 2018 14:35
@martincostello martincostello mentioned this pull request Oct 7, 2018
Merged
@martincostello
Copy link
Member Author

We should merge this before #115 or #118 so it's easy to cherry-pick into the release/3xx branch.

@martincostello martincostello merged commit abdc04a into justeattakeaway:master Oct 7, 2018
@martincostello martincostello deleted the Fix-Unbuffered-Batch-Metrics-Send branch October 7, 2018 16:26
@martincostello martincostello added this to the v3.2.2 milestone Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnthonySteele AnthonySteele AnthonySteele approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v3.2.2
Development

Successfully merging this pull request may close these issues.
2 participants
@martincostello @AnthonySteele