Buffered multi-metric packets #18
Conversation
All the secure client implementation has been moved into the SecureUDPClient class; the `StatsD` core no longer has any secure-related features. Hence, the secure client must be initialized directly with the shared key, or with `StatsD#add_shard`, which has been updated to transparently forward its arguments to the client constructor. The hashing/HMAC is now performed right before writing to the socket, which allows for greater composability.
| @@ -140,7 +201,7 @@ def send(stat, delta, type, sample_rate=1) | |||
| stat.gsub!(/::/, ".".freeze) | |||
| stat.gsub!(RESERVED_CHARS_REGEX, "_".freeze) | |||
|
|
|||
| msg = "" | |||
| msg = String.new | |||
There was a problem hiding this comment.
What's the reason for this change? I didn't see anything in the commit messages. (My guess it's to be compatible with frozen-by-default string literals in a future Ruby version.)
There was a problem hiding this comment.
Correct guess! In 2.3 (if you enable frozen by default), and in 3.0 (for all strings), the empty string will come as frozen, so we won't be able to append to it. This small change will hopefully save us a small version bump! :)
(in the future we should also be able to remove all the .freeze calls in the file, but leaving them there causes no harm at all)
|
@aroben: Pretty happy with the refactoring in the first commit. It seems like we added "Secure Mode" before we added support for custom Client Classes, so the logic for the secure mode was left mushed up inside the main class. I think keeping all the Statsd-secure code inside a custom client class makes it look much cleaner! |
| sock.write(@buffer) | ||
| @buffer.clear | ||
| rescue => boom | ||
| nil |
There was a problem hiding this comment.
Is there a more restricted set of exceptions we can rescue here?
There was a problem hiding this comment.
This rescue has been cargo-culted all the way from the original gem. I agree we should narrow this down to UDPSocket exceptions. Let me investigate if there's a definite list of those.
| signature + payload | ||
| end | ||
| class Buffer | ||
| DEFAULT_BUFFER_CAP = 512 |
There was a problem hiding this comment.
Any reason for choosing this? Just curious if it is intentional or a best guess.
There was a problem hiding this comment.
@jnunemaker: Although y'all in dotcom usually lean towards long PR bodies, in Systems (given that we contribute a lot of code upstream) we go for small, well defined commits and detailed commit messages: a06bb3a ;)
There was a problem hiding this comment.
...However, if you think it'd be easier for other people in the future, I can copy paste the commit message as a comment.
There was a problem hiding this comment.
Duh. Forgot about that. Should have read the commits. Thanks!
The new `Buffer` wrapper allow wrapping any UDP clients in a "buffering mode" that batches incoming metrics into a buffer and eventually flushes them to the statsd-compatible server. The receiving server *must* support the multi-metric StatsD protocol (namely, parsing several metrics in the same packet, separated by newlines). Both the original StatsD and now Brubeck support this. By sending several metrics in a single packet, we reduce the amount of network operations (both in the client and the server) and the amount of context switches to the kernel UDP stack, significantly increasing the throughput of the server implementation. The default maximum packet size has been set at 512 bytes, which is a "safe size" to reliably transmit an atomic packet through the internet: The MTU on the internet is 576, and the size of the IPv4 header is 20 bytes, and the UDP header 8 bytes. This leaves 548 bytes available for user data. We cap it at 512 to add some headspace (most other UDP-based protocols, such as DNS, do the same thing).
This should greatly increase the throughput of the StatsD client and server. See the detailed commit messages for explanations and design decisions.
cc @ross @aroben