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

gather elasticsearch indices and shard stats #2872

Conversation

MatthewOHaraTR
Copy link
Contributor

Required for all PRs:

  • [x ] Signed CLA.
  • [x ] Associated README.md updated.
  • [x ] Has appropriate unit tests.

@MatthewOHaraTR
Copy link
Contributor Author

@danielnelson Much easier with a new fork. I'd accumulated too much while the old one was open over the last 8 months. Thanks for your patience.

@MatthewOHaraTR
Copy link
Contributor Author

@danielnelson The circle test failed, but unless I'm reading it incorrectly, its unrelated to any changes of mine. Specifically, the inputs/elasticsearch test did complete OK. The only error I noted was:

--- FAIL: TestMemcachedGeneratesMetrics (0.00s)
Error Trace: memcached_test.go:25
Error: Received unexpected error:
dial tcp 127.0.0.1:11211: getsockopt: connection refused
FAIL
FAIL github.com/influxdata/telegraf/plugins/inputs/memcached 0.022s

clusterFields := map[string]interface{}{
"cluster_name": healthStats.ClusterName,
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly this is adding a new field named cluster_name after having changed the previous tag cluster_name to name with the exact same information. Is there any reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably better to simply contain this information in the cluster_name tag.

indexFields := map[string]interface{}{
"index_name": name,
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to have index_name as field and as a tag at same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can get rid of the redundant (non-tag) index_name.

@lpic10
Copy link
Contributor

lpic10 commented Jun 1, 2017

@mhohara I will try to test this soon, you need to update the README after the latest changes

@MatthewOHaraTR
Copy link
Contributor Author

@lpic10 Readme updated. And thanks for looking at this.

@eran-totango
Copy link

@mhohara @lpic10 any ETA for the indicestats feature?

@MatthewOHaraTR
Copy link
Contributor Author

@lpic10 Any progress on your review?

@florck
Copy link

florck commented Jul 19, 2017

👍

@MatthewOHaraTR
Copy link
Contributor Author

@danielnelson @lpic10 Any progress here? I don't see much in the way of other updates to the ES plugin as of yet.

@danielnelson
Copy link
Contributor

Sorry no progress to report, we still need to do #2650 #2711 first.

@MatthewOHaraTR
Copy link
Contributor Author

Are you waiting for something from me on those two?

@danielnelson
Copy link
Contributor

No, but you are certainly welcome to give fixing #2650 a shot.

@MatthewOHaraTR
Copy link
Contributor Author

@danielnelson I'm not sure I understand the issue in #2650 . Could the e.masterNodeId from the catmaster be compared to a cat/nodeattrs call to localhost:9200 to get the NodeId of the node its on, to see whether 'it' is the master?

@danielnelson
Copy link
Contributor

The problem is that there is a race condition, each server is collected concurrently via the anonymous goroutine in Gather, so the order in which e.isMaster is assigned too here is not defined. The isMaster value should probably not be a field on the plugin struct.

@MatthewOHaraTR
Copy link
Contributor Author

In this PR's version of the code, the isMaster flag is only used in the Gather routine. Perhaps the necessary ES API calls to determine whether the node that telegraf is running on is the cluster master need to be put into the Gather routine?

@danielnelson
Copy link
Contributor

I remember the PR code did not fix the problem, the routine that determines master status is less important than not storing it on the struct where it will be available/overwritten by other routines.

@MatthewOHaraTR
Copy link
Contributor Author

The flag was put in the struct because it is the only output of the gatherNodeStats routine which did the ES API calls to get the node information and hence could decide whether a node was master. So it seemed the best/only way to return the isMaster information back to the calling Gather routine. Moving
the call needed to decide whether the node is master to the Gather routine seems a reasonable way to fix the issue (if I understand the issue correctly).

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
@hgfischer
Copy link

Is this abandoned? Do you need help? I am interested in this feature.

@danielnelson
Copy link
Contributor

@hgfischer We need to address #2650 and #2711 before completing this issue.

@danielnelson
Copy link
Contributor

Closed in #6060

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.

6 participants