Allow using Unix Domain Socket instead of UDP #472
Conversation
|
Hooray! All contributors have signed the CLA. |
There was a problem hiding this comment.
Thanks @rubenruizdegauna . I would prefer not to make this the default behaviour applied to a metrics-addr string. We could use either a uri form unix:///path/to/socket (as some other systems do) or an abs path /path/to/socket to indicate unix domain sockets. I'd be happy with the abs path and we can identify a unix socket by looking for prefix '/'.
README.md
Outdated
| @@ -47,6 +47,10 @@ to run `gostatsd` with a graphite backend and a grafana dashboard. | |||
| While not generally tested on Windows, it should work. Maximum throughput is likely to be better on | |||
| a linux system, however. | |||
|
|
|||
| The server listens for UDP packets by default. Unix Domain Sockets (UDS) can be used specifying a file path instead | |||
There was a problem hiding this comment.
Rather than abbreviating to UDS (which afaik is not generally used) we could use a shorthand of 'unix sockets' or 'IPC sockets'. I don't want to nitpick but we should use the accepted terminology - if i'm wrong and UDS is a commonly used acronym for this then that's fine.
There was a problem hiding this comment.
👍 I've used unix sockets and rephrased it a bit in b17ebf5
README.md
Outdated
| @@ -47,6 +47,10 @@ to run `gostatsd` with a graphite backend and a grafana dashboard. | |||
| While not generally tested on Windows, it should work. Maximum throughput is likely to be better on | |||
| a linux system, however. | |||
|
|
|||
| The server listens for UDP packets by default. Unix Domain Sockets (UDS) can be used specifying a file path instead | |||
| of `address:port` with the `metrics-addr` configuration option. This only works on linux and will ignore `conn-per-reader` | |||
| configuration option. | |||
There was a problem hiding this comment.
Somewhere the docs should mention the socket mode is SOCK_DGRAM not SOCK_STREAM
There was a problem hiding this comment.
I've added it to this paragraph in b17ebf5. I wasn't sure if it was too much info about the socket in this section and I thought about adding another section just for this. Let me know what do you think and I can quickly change it.
| @@ -2,12 +2,15 @@ package statsd | |||
|
|
|||
| import ( | |||
| "context" | |||
| "net" | |||
| "os" | |||
| "runtime" | |||
| "sync" | |||
| "testing" | |||
| "time" | |||
|
|
|||
| "github.com/magiconair/properties/assert" | |||
There was a problem hiding this comment.
I didn't realize we had other assert libs. Raised #480 to remove others.
pkg/statsd/statsd_test.go
Outdated
| @@ -2,8 +2,10 @@ package statsd | |||
|
|
|||
| import ( | |||
| "context" | |||
| "github.com/stretchr/testify/assert" | |||
There was a problem hiding this comment.
There should be a check somewhere for this, not sure why the build didn't hit it. Import ordering is:
- stdlib
- 3rd party
- local
|
Thanks @aelse @tiedotguy for the quick review. I have made the requested changes in b17ebf5 |
rubenruizdegauna
force-pushed
the
unix_socket_server
branch
from
11d015b to
b17ebf5
Compare
|
lgtm. If @tiedotguy is happy we can merge. |
Hi @tiedotguy , do you think this can be merged? Thanks! |
Allow using Unix Domain Sockets instead of UDP for the server.
If the
metrics-addris not empty and not[host]:portcombination it will be considered a Unix Socket path and it will create and listen in that socket instead of UDP.