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

Allow for force gathering ES cluster stats #4345

Merged
merged 2 commits into from
Nov 19, 2018

Conversation

PierreF
Copy link
Contributor

@PierreF PierreF commented Jun 26, 2018

Add option to gather Elasticsearch cluster stats from all node (default is old behavior, stats are only gathered from master).

Also documented the fact that gathering only from master node only works if local = true.
Slightly changed behavior when local=false and cluster_stats=true. Previous behavior was undetermined (it gather cluster stats only if the last node is the master node. The order was based on the order of a map which is probably not fixed). New behavior is the documented one: if local = false, we gather from all node.

@PierreF
Copy link
Contributor Author

PierreF commented Jun 27, 2018

I don't think the circleci error is due to my change. It's failing on dep ensure but I didn't changed anything on dependencies. I suspect a timeout while fetching dependencies.
Can I retry the circleci jobs, if yes how ?

@glinton
Copy link
Contributor

glinton commented Jun 27, 2018

I'm thinking we'll update these lines to be vendor-{{ checksum "Gopkg.lock" }} so there is something to start from. It is indeed a timeout while fetching dependencies.

@danielnelson danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/elasticsearch labels Jul 10, 2018
@danielnelson
Copy link
Contributor

The setting of isMaster has a long time race condition: #2650

@PierreF Is there a reason we can't collect cluster_stats when local=false?

@PierreF
Copy link
Contributor Author

PierreF commented Jul 11, 2018

We can always collect cluster_stats, and (with this PR) as soon as cluster_stats = True, they will be collected.
We can't however collect them only from the master when local=false. This is because we determine the master when we query the node stats, if we does only for the local node (local=true), we can know if the local node is the master. But when local=false and we query all nodes, we have no way to know which node is the local one.

@danielnelson
Copy link
Contributor

Makes sense, do you think it is possible for us to know which host is the one we connected to when using local=true and /_nodes/stats? I feel like this would be helpful in simplifying the config.

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@

### Features

- [#4345](https://github.com/influxdata/telegraf/pull/4345): Allow to force gathering Elasticsearch cluster stats on non-master node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only suggestion is to remove update to the changelog to prevent merge conflict

@PierreF
Copy link
Contributor Author

PierreF commented Oct 24, 2018

Sorry for the delay. I've removed the changelog entry to avoid merge conflict.

About @danielnelson query: the behavior (the PR didn't change this) is:

  • with local=true, query /_nodes/_local/stats which include only stats of Elastic node Telegraf is currently connected to
  • with local=false, query /_nodes/stats which include stats of all Elastic nodes in the cluster. The output is the same on every nodes, so we don't have any way to find to which node Telegraf connected to.

We could do an additional query on /_node/_local to known the current node ID, and the query /_nodes/stats, but that means one additional query at each gather.

@PierreF
Copy link
Contributor Author

PierreF commented Nov 19, 2018

Do you want me to do the additional query on each gather to simplify configuration ? Or is it anything else missing to merge this PR ?

@danielnelson danielnelson added this to the 1.10.0 milestone Nov 19, 2018
@danielnelson danielnelson merged commit 0772076 into influxdata:master Nov 19, 2018
@danielnelson
Copy link
Contributor

Let's go with this for now. I wonder if we should consider dropping local=false mode completely though, I feel like the flexibility of this plugin is complicating things and it might be better to just have one clear way to monitor the cluster.

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/elasticsearch feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants