Add basic index recovery metricset #7225

ruflin · 2018-05-31T14:01:18Z

Add basic metricset for index recovery. By default only data about indices is fetched that are currently recovering.

Further changes:

Fix NO_COMPOSE flag for Golang integration tests
Clean up elasticsearch.GetInfo to also use fetchPath function
Enhance testing to also support settings metricset configs.

houndci-bot · 2018-05-31T14:01:40Z

metricbeat/module/elasticsearch/index_recovery/index_recovery.go

@@ -0,0 +1,66 @@
+package index_recovery


don't use an underscore in package name

houndci-bot · 2018-05-31T14:01:40Z

metricbeat/module/elasticsearch/index_recovery/data_test.go

@@ -0,0 +1,13 @@
+// +build !integration
+
+package index_recovery


don't use an underscore in package name

houndci-bot · 2018-05-31T14:01:40Z

metricbeat/module/elasticsearch/index_recovery/data.go

@@ -0,0 +1,53 @@
+package index_recovery


don't use an underscore in package name

houndci-bot · 2018-05-31T14:01:41Z

metricbeat/module/elasticsearch/index_recovery/index_recovery.go

@@ -0,0 +1,66 @@
+package index_recovery


don't use an underscore in package name

houndci-bot · 2018-05-31T14:01:41Z

metricbeat/module/elasticsearch/index_recovery/data_test.go

@@ -0,0 +1,13 @@
+// +build !integration
+
+package index_recovery


don't use an underscore in package name

houndci-bot · 2018-05-31T14:01:41Z

metricbeat/module/elasticsearch/index_recovery/data.go

@@ -0,0 +1,53 @@
+package index_recovery


don't use an underscore in package name

jsoriano · 2018-07-11T08:47:15Z

metricbeat/module/elasticsearch/index_recovery/data.go

+			event := mb.Event{}
+			event.ModuleFields = common.MapStr{}
+			event.ModuleFields.Put("index.name", indexName)
+			event.MetricSetFields, err = schema.Apply(data)


err is not used

I added an if err check and send an the error event. I wonder if this is the right thing to do here.

I now skiped the error completely. I initially reported it as an event. The good thing about it is that I detected a problem I had in the schema. The bad thing was that now instead of in the logs the user would have tons of documents in his index with errors which I think is not good.

Maybe we could log the errors instead of sending them, or collect them in a multierror and send them all together as an only event.

jsoriano · 2018-07-11T08:47:52Z

metricbeat/module/elasticsearch/index_recovery/data.go

+	}
+
+	for indexName, d := range data {
+		for _, data := range d["shards"] {


Would we want to explicitely check if shards exists?

jsoriano · 2018-07-11T08:56:09Z

metricbeat/tests/system/config/metricbeat.yml.j2

+  {{ k }}: {{ v }}
+  {% endfor %}
+  {% endfor %}
+


I wouldn't add more variables to this template, for this kind of settings, the extra option is used In system and munin modules, this could be also used here (or we could also rename the extra to metricset_options if this is the use case for extra as it seems to be used for similar things).

removed, didn't reliase extra is there.

jsoriano · 2018-07-11T09:05:53Z

metricbeat/module/elasticsearch/index_recovery/index_recovery.go

+// Fetch gathers stats for each index from the _stats API
+func (m *MetricSet) Fetch(r mb.ReporterV2) {
+
+	isMaster, err := elasticsearch.IsMaster(m.HTTP, m.HostData().SanitizedURI+m.recoveryPath)


Is recoveryPath used to check master state?

The full url must be passed as this is the url that is revert to afterwards again. If not passed parts of the path will go missing. This is because of a hack to reuse the http client.

ycombinator · 2018-07-12T01:50:02Z

metricbeat/module/elasticsearch/index_recovery/_meta/fields.yml

+  description: >
+    index
+  fields:
+    - name: id


Should this be shard_id?

Kind of, see my comment in the code: https://github.com/elastic/beats/pull/7225/files#diff-d198e8deee0000763245926eefe134eaR32

TBH I'm still struggling a bit with what shard.id is as I would expect it to be a unique identifier but it's a counting integer related to an index as far as I can see.

ycombinator · 2018-07-12T01:52:16Z

metricbeat/module/elasticsearch/index_recovery/_meta/fields.yml

+    - name: source.name
+      type: keyword
+      description: >
+        Source node name.


What about other fields such as translog.*, start_time, stop_time, etc.? Are you planning on adding them later?

Add them later.

ycombinator · 2018-07-12T02:02:21Z

metricbeat/module/elasticsearch/index_recovery/data.go

+	}
+)
+
+func eventsMapping(r mb.ReporterV2, info elasticsearch.Info, content []byte) error {


Maybe I'm missing something but at first glance it looks like info isn't actually being used here?

You are right, I added it out of habit as the plan is to add the xpack part where it will be need I think. Should I remove it?

Yeah, my preference would be to remove it until we need it. Otherwise it might be confusing as to why it's there.

Ok, removed and new version pushed.

ycombinator · 2018-07-12T02:03:05Z

metricbeat/module/elasticsearch/index_recovery/index_recovery.go

+		return
+	}
+
+	info, err := elasticsearch.GetInfo(m.HTTP, m.HostData().SanitizedURI+m.recoveryPath)


See my other comment about info not being used.

Add basic metricset for index recovery. By default only data about indices is fetched that are currently recovering. Further changes: * Fix `NO_COMPOSE` flag for Golang integration tests * Clean up elasticsearch.GetInfo to also use `fetchPath` function * Enhance testing to also support settings metricset configs.

ruflin added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels May 31, 2018

houndci-bot reviewed May 31, 2018

View reviewed changes

ruflin force-pushed the index-recovery branch 2 times, most recently from 0173924 to 6de0631 Compare July 10, 2018 07:49

ruflin added review and removed in progress Pull request is currently in progress. labels Jul 10, 2018

ruflin force-pushed the index-recovery branch 2 times, most recently from 52e9a1d to e82ad36 Compare July 10, 2018 11:56

ruflin changed the title ~~add basic index recovery metricset~~ Add basic index recovery metricset Jul 10, 2018

jsoriano reviewed Jul 11, 2018

View reviewed changes

ruflin force-pushed the index-recovery branch from a439d23 to 1091b6b Compare July 11, 2018 13:18

ycombinator reviewed Jul 12, 2018

View reviewed changes

ruflin mentioned this pull request Jul 12, 2018

Monitor the Elastic Stack with Metricbeat #7035

Closed

39 tasks

ruflin added 7 commits July 13, 2018 15:48

improve testing

cc9b504

remove template part

988b562

update generated file

f23eb14

skip mapping errors

e04a187

rebase

3b2b331

remove info as not used yet

0f84366

ruflin force-pushed the index-recovery branch from 1091b6b to 0f84366 Compare July 13, 2018 14:04

jsoriano approved these changes Jul 13, 2018

View reviewed changes

ycombinator merged commit 14888de into elastic:master Jul 14, 2018

ruflin deleted the index-recovery branch July 20, 2018 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic index recovery metricset #7225

Add basic index recovery metricset #7225

ruflin commented May 31, 2018 •

edited

Loading

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

jsoriano Jul 11, 2018

ruflin Jul 11, 2018

ruflin Jul 11, 2018

jsoriano Jul 11, 2018

jsoriano Jul 11, 2018

ruflin Jul 11, 2018

jsoriano Jul 11, 2018

ruflin Jul 11, 2018

jsoriano Jul 11, 2018

ruflin Jul 11, 2018

ycombinator Jul 12, 2018

ruflin Jul 13, 2018

ycombinator Jul 12, 2018

ruflin Jul 13, 2018

ruflin Jul 13, 2018

ycombinator Jul 12, 2018

ruflin Jul 13, 2018

ycombinator Jul 13, 2018

ruflin Jul 13, 2018

ycombinator Jul 12, 2018

		@@ -0,0 +1,13 @@
		// +build !integration

		package index_recovery

Add basic index recovery metricset #7225

Add basic index recovery metricset #7225

Conversation

ruflin commented May 31, 2018 • edited Loading

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

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

ruflin commented May 31, 2018 •

edited

Loading