re-architect the client #91
Conversation
it was only benchmarking a small chunck of the code and there were some random stuff in there too
This reverts commit a551161.
… ... rely on the interface instead of the implementation. I removed all the tests that were testing the implementation. The main goal here is to detach the existing unit tests from the implementation to be able to change the implementation and still test it with the old unit tests. A few benchmarks that were not useful were also removed at the same time.
…ement) setting the precision to 6 appears to be much more expensive even when formating floats with more than 6 digits after the decimal point using 6 digits was introduced here #32 most other open source projects seems to use -1 the only exception is timing which will often have a high number of digits after the decimal point. let's keep it at 6 for it
increase the perf by 10-20%
statsd/buffer.go
Outdated
|
|
||
| func newStatsdBuffer(maxSize, maxElements int) *statsdBuffer { | ||
| return &statsdBuffer{ | ||
| buffer: make([]byte, 0, maxSize*2), |
There was a problem hiding this comment.
comment here explaining the *2 would be nice, even if you have to talk a little bit about allocations and internal slice behavior.
| } | ||
| } | ||
|
|
||
| func (b *statsdBuffer) reset() { |
There was a problem hiding this comment.
These are not thread-safe.... we'll have to synchronize access to the buffer (or maybe you've dealt with it cleverly somewhere else ;)
There was a problem hiding this comment.
That was intended, this struct is not thread safe. It's the role of what's using the struct to make sure it's not accessed concurrently
| } | ||
|
|
||
| func appendWithoutNewlines(buffer []byte, s string) []byte { | ||
| // fastpath for strings without newlines |
There was a problem hiding this comment.
Is it really a fastpath? won't it do something really close to what you're doing below anyways? (just wondering if you'll be traversing the string twice unnecessarily).
There was a problem hiding this comment.
This was ported from the old code. I was surprised as well but IndexByte has an optimized assembly implementation that is indeed faster in benchmarks https://golang.org/src/internal/bytealg/index_amd64.s
Optimizing for no newlines sounds good to me as this should be the normal case
statsd/statsd.go
Outdated
| c.commands = c.commands[cmdsFlushed+1:] | ||
| // flush the current buffer. Lock must be held by caller. | ||
| // flushed buffer writen to the network asynchronously. | ||
| func (c *Client) flushLocked() { |
There was a problem hiding this comment.
I feel this name is confusing. I'd prefer something like flushUnsafe or something.
statsd/statsd.go
Outdated
| return "" | ||
| } | ||
|
|
||
| func (c *Client) writeMetric(m metric) error { |
There was a problem hiding this comment.
Also unsafe, right? I feel like this should be noted.
statsd/statsd.go
Outdated
| if b != '\n' { | ||
| buf = append(buf, b) | ||
| } | ||
| if err := c.Flush(); err != nil { |
There was a problem hiding this comment.
should an error flushing prevent us from closing the sender?
There was a problem hiding this comment.
Probably not. Let's do a best effort flush and ignore the error ?
| case b := <-p.pool: | ||
| return b | ||
| default: | ||
| return newStatsdBuffer(p.bufferMaxSize, p.bufferMaxElements) |
There was a problem hiding this comment.
I've got mixed feelings about this... if we're using a pool, expected behavior would be to pull the buffer from the pool, the pool allows setting a bound on the resources used. Wondering if we should set a flag, with respect to getting strict pool behavior, or allowing us to dynamically grow the pool. Any thoughts?
There was a problem hiding this comment.
I decided to go with this as it allows for some flexibility if you have a use case with sudden bursts of metrics. I see this as being somewhat similar to sync.Pool but with a maximum number of element that should be kept and no cleanup on gc.
What does this PR do ?
The main goal of this PR is to fix a bunch of bad behaviors this client had that would result in drops, increased resource consumption on the agent and back pressure on the instrumented app. Since some big architectural changes were needed to fix most of them, I thought it would make sense to fix them all at the same time.
The goal is also to make the client performant OOB. This includes enabling buffering and setting sensible values for high throughput.
1432, the optimal size for local UDP). Fixes No way to change OptimalPayloadSize #671432when using UDP and8192when using UDS.Architecture
The high level idea is:
Every-time the instrumented app makes a call to the client we format the metric and store it in the current buffer. When this buffer gets full (or we reach a timeout) it is forwarded to a queue of buffers that should be sent over UDP or UDS by the sender asap. A new buffer is then taken from a buffer pool and used as the current buffer. When the sender is done with the buffer we added to the queue, it puts it back in the buffer pool.
There are two main goals with this design:
Disclaimer: I took some inspiration from https://github.com/smira/go-statsd with the architecture.
Upgrade notes
bufferedoption has been removed as the client can only be buffered. If for some reason you need to have only one dogstatsd message per payload you can still use theWithMaxMessagesPerPayloadoption set to 1.asyncUDSoption has been removed as the networking layer is now running in a separate Goroutine.Benchmarks against master
This was not the main goal of this PR but we get some nice improvements in raw performance:
Result against the default configuration:
Results against the "optimal" configuration (MAX_INT elements in the buffer, AsyncUDS):