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

datadog integration for twemproxy, options for haproxy #326

Merged
merged 13 commits into from
Jul 21, 2017

Conversation

swwolf
Copy link
Contributor

@swwolf swwolf commented May 10, 2017

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)

@swwolf swwolf changed the title adding datadog integration for twemproxy datadog integration for twemproxy, options for haproxy May 11, 2017
@truthbk
Copy link
Member

truthbk commented May 11, 2017

Nice, and big thanks for the spec tests. This looks promising, I'll review very soon! :)

@truthbk truthbk self-requested a review May 11, 2017 14:19
@truthbk truthbk added this to the 1.11.0 milestone May 11, 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.

@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).

# }
#
class datadog_agent::integrations::twemproxy(
$servers = [{'host' => 'localhost', 'port' => '22222'}]
Copy link
Member

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.

@swwolf
Copy link
Contributor Author

swwolf commented Jun 28, 2017

@truthbk I did the necessary changes 😃

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.

This now looks good!! Merging! 🙇

@truthbk truthbk merged commit e396468 into DataDog:master Jul 21, 2017
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Jan 31, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants