add a non-blocking implementation for UDS #81
Conversation
arbll
changed the title
| // asyncUdsWriter is an internal class wrapping around management of UDS connection | ||
| type asyncUdsWriter struct { | ||
| // Address to send metrics to, needed to allow reconnection on error | ||
| addr net.Addr |
There was a problem hiding this comment.
I would probably put addr, conn and writeTimeout in a separate structure that both the async and blocking writer structs would include in their definitions.
edit: see note regarding using the async writer as a blocking writer.
There was a problem hiding this comment.
Let's discuss https://github.com/DataDog/datadog-go/pull/81/files#r266391555 and come back to this if it still make sense
| addr: udsAddr, | ||
| conn: nil, | ||
| writeTimeout: defaultUDSTimeout, | ||
| // 8192 * 8KB = 65.5MB |
There was a problem hiding this comment.
Let's make sure this is clearly documented - this will have an immediate impact in memory footprint on higher throughput clients.
There was a problem hiding this comment.
Agreed. I am still missing a PR to add a bunch of docs before releasing this and #82
| } | ||
|
|
||
| // Looks like we might need to connect - try again with write locking. | ||
| if w.conn != nil { |
There was a problem hiding this comment.
This seems redundant with the above (https://github.com/DataDog/datadog-go/pull/81/files#diff-95ff682af2f28c4955d9ba7de9d1ef8dR100), am I missing something?
| } | ||
| } | ||
|
|
||
| func (w *asyncUdsWriter) write(data []byte) (int, error) { |
There was a problem hiding this comment.
This function is concurrency unsafe, we should maybe add a note in case it ever occurs to us we want multiple writers for added performance.... Not sure we'd ever be able to do that with deadlines anyways, so feel free to ignore this comment.
| } | ||
|
|
||
| // New returns a pointer to a new blockingUdsWriter given a socket file path as addr. | ||
| func newBlockingUdsWriter(addr string) (*blockingUdsWriter, error) { |
There was a problem hiding this comment.
I'm not sure we need a specific implementation for a blocking writer, I believe in effect the async writer with a datagram queue depth of 1 is essentially the same thing. There might be some details we want to look at - channel blocking vs mutex locking, etc, but I think it'd behave exactly as expected, it would perhaps reduce the amount of duped code here by thinking about that.
There was a problem hiding this comment.
The issue is error checking. Unlike UDP, UDS will error out if the agent does not receive the payload. With the blocking implementation you would get alerted if your message did not reach the agent.
However, in my opinion it's pretty flawed especially if the client buffers. In this case you only get one error on the call that actually sends the payload.
I think the best path might be:
- Merge this and configuration at client creation #82
- (Optional) Set the blocking implementation as default
- Release 2.x
- Remove the blocking implementation
- Release 3.0
WDYT ?
cc @xvello you might have an opinion
|
OK, I see #82 addresses some of my observations here, feel free to ignore anything you've addressed there ;) |
|
Just noticed tests are failing. It seems travis was not working previously. I guess we need to maintain support for those EOL'd version in 2.x ? |
* introduce options * configuration at client creation
What does this PR do
Use a non-blocking UDS client implementation by default. Another PR will follow this one to allow choosing between the old blocking behavior that allowed error checking and this one.
Non-blocking UDS client
Motivation
More details are available in the original POC #79.
Notes
The old blocking implementation is kept in case someone started relying on error checking.
Being able to configure the client to use the blocking implementation requires some configuration refactoring. It will be done in #82.
You should probably review this PR commit by commit.