-
Notifications
You must be signed in to change notification settings - Fork 260
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
datadog integration for twemproxy, options for haproxy #326
Conversation
Nice, and big thanks for the spec tests. This looks promising, I'll review very soon! :) |
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.
@swwolf this stuff looks really good. It's almost ready to go, I'd just like to us to follow the widespread pattern for multi-instance checks (even though I don't love it).
manifests/integrations/twemproxy.pp
Outdated
# } | ||
# | ||
class datadog_agent::integrations::twemproxy( | ||
$servers = [{'host' => 'localhost', 'port' => '22222'}] |
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'd rather respect the pattern we have in similar checks where we have arguments host
and port
and maybe some basic options (for single instance) and then an instances
argument for users who wish to specify multiple instances for the check. Don't get me wrong, this is perfectly fine, but it breaks a pattern used widely across the check.
I am aware that mongo
and zk
both follow this approach you have here, but I'd rather have that be the exception. Most integrations follow the pattern you can see haproxy
. If you could fix that up, and the erb
, which should be easy, that would be great.
@truthbk I did the necessary changes 😃 |
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.
This now looks good!! Merging! 🙇
* add instances parameter * building config from instances * spec test for instances set * adding datadog integration for twemproxy * add options to haproxy integration * following instances pattern for twemproxy integration * fix twemproxy template typo * fix twemproxy template typo * fix twemproxy param error * fix spec test
I added twemproxy integration, and needed a way to add optional parameters for the haproxy integration (e.g. set "collect_aggregates_only: False" for an instance)