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

elasticsearch input: ability to get individual node stats #3304

Merged

Conversation

meilke
Copy link
Contributor

@meilke meilke commented Oct 4, 2017

We found that the plugin generates a lot of data and also makes Elasticsearch work quite hard to generate the data (in some cases not necessary, if you are not interested in all the data).

New configurability:

  • ability to get individual node stats

Example configuration to just get only JVM information:

 [[inputs.elasticsearch]]
   servers = ["http://elasticsearch-host:9200"]
   local = false
   cluster_health = false
   indices_health = false
   cluster_stats = true
   individual_stats = ["jvm"]

Required for all PRs:

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

@@ -259,13 +290,17 @@ func (e *Elasticsearch) gatherNodeStats(url string, acc telegraf.Accumulator) er

now := time.Now()
for p, s := range stats {
if !e.individualStatIncluded(p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't understand why this is done. It seems to me that the stats not included won't be in the slice we are ranging over and so this will never be hit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are ranging over the property-to-string-mapping stats. I cannot think of a better way of dynamically mapping the names. We go from snake case to pascal case back to snake case. Is there a way to re-use the original JSON mappings (JSON to struct) to get the snake case names back. We are defining these mappings twice right now...

I added a simple nil check for now.

f := jsonparser.JSONFlattener{}
// parse Json, ignoring strings and bools
err := f.FlattenJSON("", s)
if err != nil {
return err
}
acc.AddFields("elasticsearch_"+p, f.Fields, tags, now)
measurement := "elasticsearch_" + p
acc.AddFields(measurement, f.Fields, tags, now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this hunk, it's just a style change and no reason to make history more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it is a remainder of something I tried earlier.

@@ -340,7 +375,8 @@ func (e *Elasticsearch) gatherClusterStats(url string, acc telegraf.Accumulator)
if err != nil {
return err
}
acc.AddFields("elasticsearch_clusterstats_"+p, f.Fields, tags, now)
measurement := "elasticsearch_clusterstats_" + p
acc.AddFields(measurement, f.Fields, tags, now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this hunk for the same reasons

}

return url + individualStatsUrlPart
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be simplified to:

+func (e *Elasticsearch) generateStatsUrl(baseUrl string) string {
	var url string

	if e.Local {
		url = baseUrl + statsPathLocal
	} else {
		url = baseUrl + statsPath
	}

	return url + e.generateIndividualStatsUrlPart()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

@meilke meilke force-pushed the elasticsearch-input-individual-stats branch from bc8697f to 2cf7c6b Compare October 5, 2017 07:09
@danielnelson danielnelson added this to the 1.5.0 milestone Oct 5, 2017
@@ -33,6 +33,10 @@ or [cluster-stats](https://www.elastic.co/guide/en/elasticsearch/reference/curre
## Master node.
cluster_stats = false

## Set individual_stats to a list of sub-stats you are interested in
## default is all stats
# individual_stats = ["jvm"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be difficult to know what these values apply to as is. Would it help if we call this like node_stats = ["jvm"]? We should show the default in the commented out line, and it will also help if we list the current valid options:

+  ## Node stats are the statistic categories to gather for each node.  Valid
+  ## options are `indices`, `os`, `process`, `jvm`, `transport`, `http`, `fs`,
+  ## `breaker` and `thread_pool`.  If empty, all categories are gathered.
+  # nodes_stats = []

return "/" + strings.Join(e.IndividualStats, ",")
}

func (e *Elasticsearch) generateStatsUrl(baseUrl string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename nodeStatsUrl(

@@ -229,6 +229,26 @@ func (e *Elasticsearch) createHttpClient() (*http.Client, error) {
return client, nil
}

func (e *Elasticsearch) generateIndividualStatsUrlPart() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename nodeStatsUrlPart(, or maybe just integrate into the parent function since it is not called from anywhere else.

@meilke meilke force-pushed the elasticsearch-input-individual-stats branch from 2cf7c6b to fe6d678 Compare October 6, 2017 07:25
@meilke
Copy link
Contributor Author

meilke commented Oct 6, 2017

Done! 😉

@danielnelson danielnelson merged commit 75567d5 into influxdata:master Oct 6, 2017
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.

None yet

2 participants