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.

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

Add UDS support #37

Merged
merged 8 commits into from
Aug 9, 2017
Merged

Add UDS support #37

merged 8 commits into from
Aug 9, 2017

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Aug 7, 2017

Add Unix Domain Socket support, as implemented in DSD6.

Protocol and error handling follows the specs at https://github.com/DataDog/datadog-agent/wiki/Unix-Domain-Sockets-support

Documentation will be updated once code is OK.

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid!

statsd/statsd.go Outdated
UDSTimeout holds the default timeout for UDS socket writes, as they can get
blocking when the receiving buffer is full.
*/
const DefaultUDSTimeout = 1 * time.Millisecond
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to export this?

statsd/statsd.go Outdated
conn net.Conn
// function pointer to use to write data to the connection
write func([]byte) error
Copy link
Member

@truthbk truthbk Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

If we export Write, Client would implement the Writer interface, which might be desireable (I'm not sure just a thought). Although we might also want precisely the opposite to prevent the users from writing directly to the socket - yeah, I think we don't want to export Write :)

What we could do to maybe do is have this be an interface to io.Writer.

type Client struct {
   ...
   sock io.Writer,
   ...
}

And have two different structs, one for UDP and one for UDS, both implementing Writer. We can assign the right one during the creation of the Client. Should be straight-forward, but let me know if I'm missing something.

You could then do cli.sock.Write([]byte) in the remaining code.

It's tremendously similar to what you're already doing, so it may be overkill. But it would perhaps give us some flexibility for the future.

statsd/statsd.go Outdated
c.writeTimeout = d
return nil
} else {
return errors.New("SetWriteTimeout: only supported for UDS connexions")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny typo: connexions -> connections

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like the new approach, it's a little more code. But more decoupled, idiomatic, maintainable. 👍

@xvello xvello changed the title [WIP] add UDS support Add UDS support Aug 8, 2017
@truthbk truthbk added this to the 1.2.0 milestone Aug 9, 2017
Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @xvello :) merging!

@truthbk truthbk merged commit b10af4b into DataDog:master Aug 9, 2017
// A statsdWriter offers a standard interface regardless of the underlying
// protocol. For now UDS and UPD writers are available.
type statsdWriter interface {
Write(data []byte) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @xvello and @truthbk

Do you mind explaining why you chose a custom "Write" method and deviated from the std lib io.Writer interface?

I was fixing my PR and dismayed by this change.

If that was an oversight I can fix that in that PR as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cihangir we should make it implement io.Writer indeed, can you fix it in #31?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants