-
Notifications
You must be signed in to change notification settings - Fork 134
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
configuration at client creation #82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very, very clean - love what you've done here. Just a couple of questions, nits.
} | ||
|
||
// Option is a client option. Can return an error if validation fails. | ||
type Option func(*Options) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan 👌
statsd/options.go
Outdated
// DefaultMaxMessagesPerPayload is the default value for the MaxMessagesPerPayload option | ||
DefaultMaxMessagesPerPayload = 16 | ||
// DefaultBlockingUDS is the default value for the BlockingUDS option | ||
DefaultBlockingUDS = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking UDS the default behavior earlier, so this would change the behavior for customer clients possibly unwillingly. I personally would love the gains, but maybe the buffered UDS sender should be an opt-in feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep blocking in 2.x
statsd/options.go
Outdated
} | ||
|
||
// BlockingUDS sets the BlockingUDS option. | ||
func BlockingUDS() Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WithBlockingUDS
for consistency, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
* move old uds writer to uds blocking * add async implementation * refactor tests * remove some useless code in ensureConnection * add comment about write thread safety * configuration at client creation (#82) * introduce options * configuration at client creation * remove EOL'd go versions from testing
What does this PR do
This PR adds a generic way to deal with the client configuration. I decided to use function options here for the implementation. This is discussable and definitely has it's pro and cons. However I prefer it over it's alternative, using a configuration struct, since it allows for dumb default value logic while using a struct would add quite a bit of complexity dealing with Golang zero values. I'm happy to discuss if someone has an opinion here.
Motivation
The list of options is growing and some new ones require to be known at the client creation. Creating new constructors like
NewBuffered
is not scalable.Allowing options to be mutable after the client creation isn't a very good practice IMO.