Adding x-pack code for `elasticsearch/index` metricset #8260

ycombinator · 2018-09-07T01:30:47Z

This PR teaches metricbeat to index index_stats documents into .monitoring-es-6-mb-* indices.

jsoriano

I have added some comments, mostly suggestions. There is also a test failing that seems related.

metricbeat/module/elasticsearch/index/data_xpack.go

jsoriano · 2018-09-07T08:32:10Z

metricbeat/module/elasticsearch/index/data_xpack.go

+		event := mb.Event{}
+		indexStats, err := xpackSchema.Apply(index)
+		if err != nil {
+			continue


log the error here? even if it is only on debug?

For the x-pack path, we decided not to report the error (via r mb.ReporterV2) as it would end up in the metricbeat-* index instead of the .monitoring-es-6-mb-* index, where all the other data ends up.

However, I think you are referring to logging the error in the metricbeat log, right?

Yes, I refer to logging the error 🙂

👍 will do that here and other places in this PR where you have noted.

BTW, all the other metricsets (with x-pack monitoring code paths) will need this too so I'm going to make their logging part of a separate PR.

jsoriano · 2018-09-07T08:32:21Z

metricbeat/module/elasticsearch/index/data_xpack.go

+
+		err = addClusterStateFields(name, indexStats, clusterState)
+		if err != nil {
+			continue


jsoriano · 2018-09-07T08:33:41Z

metricbeat/module/elasticsearch/index/data_xpack.go

+		return err
+	}
+
+	// TODO:


Empty TODO? 🙂

metricbeat/module/elasticsearch/index/data_xpack.go

jsoriano · 2018-09-07T08:39:37Z

metricbeat/module/elasticsearch/index/data_xpack.go

+	// "index_stats.shards.unassigned_primaries",
+	// "index_stats.shards.unassigned_replicas",
+	// "index_stats.shards.initializing",
+	// "index_stats.shards.relocating",


Are these comments needed?

Probably not. I was using them to make sure I was computing the same fields that are currently being computed by the internal ES collector. Perhaps I will replace them with a link to the internal ES code, just for future reference?

A comment referencing the source of the list could be nice yes 👍

jsoriano · 2018-09-07T08:41:31Z

metricbeat/module/elasticsearch/index/data_xpack.go

+	// "index_stats.version.created", <--- don't think this is being used in the UI, so can we skip it?
+	// "index_stats.version.upgraded", <--- don't think this is being used in the UI, so can we skip it?
+
+	// "index_stats.status",


This comment may indicate that the following lines could be in its own funcion, as you do with getIndexStatus() and getIndexShardsStats() (even if here it'd be a smaller function). The same for index_stats.created before.

jsoriano · 2018-09-07T08:44:16Z

metricbeat/module/elasticsearch/index/data_xpack.go

+	// "index_stats.shards.relocating",
+	shardStats, err := getIndexShardStats(shards)
+	if err != nil {
+		return err


This function returns errors in several places, you may want to make use of errors.Wrap() or similar to give more info about when it failed, for example here it could be return errors.Wrap(err, "failed to get index shard stats").

metricbeat/module/elasticsearch/index/data_xpack.go

metricbeat/module/elasticsearch/index/index.go

ruflin · 2018-09-07T14:19:27Z

metricbeat/module/elasticsearch/index/_meta/test/stats.700-alpha1.json

+      "primaries": {
+        "docs": {
+          "count": 1257,
+          "deleted": 11


We should probably "standardise" on the indentation of .json files. Do you use .editorconfig? https://github.com/elastic/beats/blob/master/.editorconfig#L15

I'm guessing I don't right now. I'll look into it so formatting can be standardized.

ruflin · 2018-09-07T14:21:27Z

metricbeat/module/elasticsearch/index/index.go

@@ -33,7 +33,7 @@ func init() {
 }

 const (
-	statsPath = "/_stats"
+	statsPath = "/_stats/docs,fielddata,indexing,merge,search,segments,store,refresh,query_cache,request_cache"


Could we define the "items" as a separate const to separate the path from the params? (I know, technically it's part of the path)

Issues addressed, only pending the failing CI

ycombinator · 2018-09-11T22:35:06Z

@jsoriano @ruflin Because GitHub will collapse these two comments, I want to re-mention them here:

Besides that, I need to fix the failing CI as well on this PR.

houndci-bot · 2018-09-12T11:46:10Z

metricbeat/module/elasticsearch/index/data.go

@@ -29,6 +29,10 @@ import (
 	"github.com/elastic/beats/metricbeat/module/elasticsearch"
 )

+type IndicesStruct struct {


exported type IndicesStruct should have comment or be unexported

ycombinator · 2018-09-17T15:02:52Z

The Elasticsearch team has decided not to move forward with elastic/elasticsearch#33614 for now so the logic to compute index status will have to remain in the metricbeat code for now. I've closed the ES PR and removed it as a dependency of this PR.

This PR removes fields that are not actually used by the Monitoring UI. This will greatly simplify the eventual migration to using Metricbeat for monitoring Elasticsearch (see elastic/beats#8260 (comment) for more context and discussion around removing these fields from ES collection).

ycombinator · 2018-09-18T01:38:06Z

elastic/elasticsearch#33584, which was a dependency of this PR has now been merged into master. So merging this PR now...

* Updating test fixture * Extract indicesStruct for reuse within package * Only request specific stats * Add X-Pack switch * Adding explanatory comment * Fleshing it out some more * Remove empty TODO * Adding reference comment for fields + refactoring into helper funcs * Re-formatting * Extract stats API metrics into own const * Using errors.Wrap * Adding logger to elasticsearch metricset * Log errors * Sharing the type not the variable 🤦 (cherry picked from commit cfee760)

This PR removes fields that are not actually used by the Monitoring UI. This will greatly simplify the eventual migration to using Metricbeat for monitoring Elasticsearch (see elastic/beats#8260 (comment) for more context and discussion around removing these fields from ES collection).

* [Monitoring] Removing unused version.* fields (#33584) This PR removes fields that are not actually used by the Monitoring UI. This will greatly simplify the eventual migration to using Metricbeat for monitoring Elasticsearch (see elastic/beats#8260 (comment) for more context and discussion around removing these fields from ES collection). * Fixing assertions in integration test

…` metricset (#8337) Cherry-pick of PR #8260 to 6.x branch. Original message: This PR teaches metricbeat to index `index_stats` documents into `.monitoring-es-6-mb-*` indices.

ycombinator added 4 commits September 6, 2018 18:27

Updating test fixture

76c690f

Extract indicesStruct for reuse within package

4becb30

Only request specific stats

b24c911

Add X-Pack switch

b691c87

ycombinator added in progress Pull request is currently in progress. Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 monitoring v6.5.0 labels Sep 7, 2018

ruflin mentioned this pull request Sep 7, 2018

Monitor the Elastic Stack with Metricbeat #7035

Closed

39 tasks

ycombinator added 2 commits September 6, 2018 18:32

Adding explanatory comment

18b84b9

Fleshing it out some more

ccae367

ycombinator requested a review from ruflin September 7, 2018 03:49

ycombinator added review and removed in progress Pull request is currently in progress. labels Sep 7, 2018

jsoriano previously requested changes Sep 7, 2018

View reviewed changes

ruflin reviewed Sep 7, 2018

View reviewed changes

ycombinator added 7 commits September 10, 2018 02:56

Remove empty TODO

1632a59

Adding reference comment for fields + refactoring into helper funcs

27ae53c

Re-formatting

e1008b1

Extract stats API metrics into own const

51af6e9

Using errors.Wrap

7943776

Adding logger to elasticsearch metricset

df463f2

Log errors

e33c0ac

ycombinator mentioned this pull request Sep 10, 2018

[Monitoring] Removing unused version.* fields elastic/elasticsearch#33584

Merged

ycombinator mentioned this pull request Sep 11, 2018

Add index status to cluster state API response (under metadata metric) elastic/elasticsearch#33614

Closed

ycombinator added the blocked label Sep 11, 2018

Sharing the type not the variable 🤦

e92acb8

houndci-bot reviewed Sep 12, 2018

View reviewed changes

ycombinator mentioned this pull request Sep 18, 2018

[Monitoring] Removing unused version.* fields elastic/elasticsearch#33790

Merged

ycombinator removed the blocked label Sep 18, 2018

ycombinator merged commit cfee760 into elastic:master Sep 18, 2018

ycombinator deleted the metricbeat-elasticsearch-index-x-pack branch September 18, 2018 01:38

ycombinator mentioned this pull request Sep 18, 2018

Cherry-pick #8260 to 6.x: Adding x-pack code for elasticsearch/index metricset #8337

Merged

ycombinator added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Sep 18, 2018

gbanasiak mentioned this pull request Mar 18, 2021

index health in index_stats documents is not aligned with Elasticsearch #24643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding x-pack code for `elasticsearch/index` metricset #8260

Adding x-pack code for `elasticsearch/index` metricset #8260

ycombinator commented Sep 7, 2018 •

edited

jsoriano left a comment •

edited

jsoriano Sep 7, 2018

ycombinator Sep 7, 2018

jsoriano Sep 7, 2018

ycombinator Sep 7, 2018

jsoriano Sep 7, 2018

jsoriano Sep 7, 2018

jsoriano Sep 7, 2018

jsoriano Sep 7, 2018

ycombinator Sep 7, 2018

jsoriano Sep 7, 2018

jsoriano Sep 7, 2018

jsoriano Sep 7, 2018

ruflin Sep 7, 2018

ycombinator Sep 7, 2018

ruflin Sep 7, 2018

ycombinator commented Sep 11, 2018 •

edited

houndci-bot Sep 12, 2018

ycombinator commented Sep 17, 2018

ycombinator commented Sep 18, 2018

Adding x-pack code for elasticsearch/index metricset #8260

Adding x-pack code for elasticsearch/index metricset #8260

Conversation

ycombinator commented Sep 7, 2018 • edited

jsoriano left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented Sep 11, 2018 • edited

Choose a reason for hiding this comment

ycombinator commented Sep 17, 2018

ycombinator commented Sep 18, 2018

Adding x-pack code for `elasticsearch/index` metricset #8260

Adding x-pack code for `elasticsearch/index` metricset #8260

ycombinator commented Sep 7, 2018 •

edited

jsoriano left a comment •

edited

ycombinator commented Sep 11, 2018 •

edited