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

Added Consul health checks state monitoring. #1294

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Added Consul health checks state monitoring. #1294

merged 1 commit into from
Jun 1, 2016

Conversation

harnash
Copy link
Contributor

@harnash harnash commented May 30, 2016

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

This is the Input Plugin for gathering metrics on Consul's Health Checks. The data it pulls from Consul are not someone would call stats but they can be used to monitor services using Kapacitor. More appropriate stats can be already sent using StatsD reporter built-in into Consul itself.

API which is used to poll the data is here

}

func init() {
inputs.Add("consul", func() telegraf.Input {
Copy link
Contributor

@robinpercy-xm robinpercy-xm May 31, 2016

Choose a reason for hiding this comment

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

Would you mind renaming this measurement to consul_health or something else specific to the endpoint? I'm working on a consul input for the agent endpoints and would be happy to combine it with this one once it's merged. That is, unless @sparrc perfers otherwise.

Copy link
Contributor Author

@harnash harnash May 31, 2016

Choose a reason for hiding this comment

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

@robinpercy-xm sure no problem.
Here is the change d1c6609. I've decided to use consul_health_checks since it describes the measurement better.
I'm not sure we should change the input name of the plugin since it provides all the necessary tools to connect to Consul. If more measurements were to be added it should go into one place (correct me if I'm wrong here).

Choose a reason for hiding this comment

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

Thanks @harnash. And yeah, I was only referring to the measurement name.

@sparrc
Copy link
Contributor

sparrc commented May 31, 2016

OK, please update the readme and rebase your changes too

@harnash
Copy link
Contributor Author

harnash commented May 31, 2016

@sparrc Rebased and README updated.

@sparrc
Copy link
Contributor

sparrc commented May 31, 2016

Looks good, but you actually haven't updated the CHANGELOG or the README (the README.md file for the entire repo, you need to add your input plugin under the list of available inputs)

@harnash
Copy link
Contributor Author

harnash commented May 31, 2016

Sorry about that. Fixed it now.

}

func (c *Consul) Gather(acc telegraf.Accumulator) error {
client, err := c.createAPIClient()
Copy link
Contributor

Choose a reason for hiding this comment

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

here you should make client an object of the Consul struct. As stated here: https://godoc.org/github.com/hashicorp/consul/api#DefaultConfig it's recommended that you reuse client objects.

So you should only call createAPIClient() if client==nil

Copy link
Contributor

Choose a reason for hiding this comment

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

that or use DefaultNonPooledConfig, but probably best to reuse the connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sparrc I've moved the client to the Consul struct in 45c498b.

@sparrc sparrc merged commit 7921d87 into influxdata:master Jun 1, 2016
@sparrc
Copy link
Contributor

sparrc commented Jun 1, 2016

thanks @harnash

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.

4 participants