Get rid of errors generated by xpack code for `elasticsearch/shard` metricset #8192

ycombinator · 2018-08-31T20:37:35Z

I was testing the elasticsearch/shard metricset with xpack.enabled: true on master and ran into a few errors:

3 errors: wrong format in `relocating_node`: expected string, found <nil>; wrong format in `node`: expected string, found <nil>; key `number` not found"

Investigating a bit I found that:

node and relocating_node can sometimes be returned as null in the GET _cluster/state/version,master_node,nodes,routing_table Elasticsearch API response used by this metricset, and
the number field has been renamed in the Elasticserach API response to shard now. I can't find when it was number but it is definitely shard now.

This PR updates the code to get rid of these errors.

It also un-hardcodes the name of the monitoring index set in data_xpack.go.

Testing this PR

Enable the elasticsearch metricset:

metricbeat modules enable elasticsearch

Edit modules.d/elasticsearch.yml and set metricsets: [ "shard" ] and xpack.enabled: true.
Run metricbeat:
```
metricbeat -e
```
Check in Elasticsearch that you don't have any documents in metricbeat-* for the elasticsearch module (since we are using xpack.enabled: true).
```
GET metricbeat-*/_search?q=metricset.module:elasticsearch
```
Check in Elasticsearch that you have documents in .monitoring-es-6-mb-* with type:shard
```
GET .monitoring-es-6-mb-2*/_search?q=type:shards
```

ruflin · 2018-09-03T06:24:15Z

metricbeat/module/elasticsearch/shard/data.go

@@ -62,6 +62,21 @@ func eventsMapping(r mb.ReporterV2, content []byte) {
 				event := mb.Event{}

 				fields, _ := schema.Apply(shard)
+
+				// Handle node field: could be string or null
+				err = elasticsearch.PassThruField("node", shard, fields)


What is the expected behaviour if a field is null? What I mean by that what should it be in the event sent to Elasticsearch? Should the field not exist or have a default value?

Could you add a test json output with such null values? https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/shard/_meta/data.json (this data.json should probably be versioned too as in other places)

Hmm, good question, thanks for bringing it up. I think in the non-xpack case, we should not index the field if it's value is null. This is the convention we've been following in the rest of Metricbeat, IIRC, and I don't see any reason specific to this metricset to deviate from it. I'll fix the code here accordingly.

And I'll add a sample data.json output, with version.

SGTM for null values.

I actually above linked to the wrong file. Where I wanted to link to is here: https://github.com/elastic/beats/tree/master/metricbeat/module/elasticsearch/shard/_meta/test Inside this directory we can add different versions for testing. Sorry about the confusion related to this I created.

ycombinator · 2018-09-04T15:45:35Z

metricbeat/module/elasticsearch/shard/data.go

+				if err != nil {
+					continue
+				}
+
 				event.ModuleFields = common.MapStr{}
 				event.ModuleFields.Put("node.name", fields["node"])


@ruflin Should this field be at the module level or the metricset level in the event document? As you can see it is currently at the module level. However, it is not mentioned in https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/_meta/fields.yml#L12-L26

It should be on the module level and it works because it's defined here: https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/node/_meta/fields.yml#L7 Not sure where we should put it as part of the module fields.yml or as part of the node fields.yml

ycombinator · 2018-09-04T15:49:13Z

@ruflin This PR is ready for review again.

I made sure via testing that if node or relocating_node are null in the source API response, they are not being indexed by metricbeat.

I also regenerated data.json (and renamed it to data.700.json). The only real difference was the addition of the elasticsearch.shard.number field.

Finally, I also added relocating_node.name to the elasticsearch/shard metricset's fields.yml.

ycombinator · 2018-09-04T15:57:41Z

@ruflin The latest CI failure is this:

08:53:33 diff --git a/metricbeat/docs/modules/elasticsearch/shard.asciidoc b/metricbeat/docs/modules/elasticsearch/shard.asciidoc
08:53:33 index 9c13a65..990840b 100644
08:53:33 --- a/metricbeat/docs/modules/elasticsearch/shard.asciidoc
08:53:33 +++ b/metricbeat/docs/modules/elasticsearch/shard.asciidoc
08:53:33 @@ -15,9 +15,3 @@ include::../../../module/elasticsearch/shard/_meta/docs.asciidoc[]
08:53:33  For a description of each field in the metricset, see the
08:53:33  <<exported-fields-elasticsearch,exported fields>> section.
08:53:33  
08:53:33 -Here is an example document generated by this metricset:
08:53:33 -
08:53:33 -[source,json]
08:53:33 -----
08:53:33 -include::../../../module/elasticsearch/shard/_meta/data.json[]
08:53:33 -----
08:53:33 metricbeat/docs/modules/elasticsearch/shard.asciidoc: needs update
08:53:33 make: *** [check] Error 1
08:53:33 Makefile:74: recipe for target 'check' failed

Basically, now that I've renamed data.json to data.700.json, I will need to remove the example generated doc from the docs for CI to be happy. Should I do this or leave data.json without a version? Or something else?

ruflin · 2018-09-05T09:38:05Z

I would leave data.json without a version. It's definitively not perfect but we currently don't have a way to show multiple versions in the docs.

ruflin · 2018-09-05T12:34:43Z

See my comment in the PR about data.json. I think that should clarify it as I confused 2 files.

ycombinator · 2018-09-11T12:15:55Z

The only CI failure on this PR is #8208. So merging this PR now...

…etricset (#8192) (#8285) * Refactoring: Extract passthruFields function for broader use * Passthru fields that could sometimes by null * Do not report errors in xpack path as they will go into metricbeat index * Un-hardcode index name * More fixes * Regenerating data.json * Fixing order * Renaming data JSON file to include version * Renaming data.700.json back to data.json * Adding unit test fixture for 7.0.0

ycombinator added review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 monitoring v6.5.0 v6.4.1 labels Aug 31, 2018

ycombinator requested a review from ruflin August 31, 2018 20:37

ruflin reviewed Sep 3, 2018

View reviewed changes

ycombinator commented Sep 4, 2018

View reviewed changes

ruflin approved these changes Sep 6, 2018

View reviewed changes

ycombinator changed the title ~~Get rid of errors generated by xpack code for elasticsearch/shards metricset~~ Get rid of errors generated by xpack code for elasticsearch/shard metricset Sep 7, 2018

ycombinator added 10 commits September 7, 2018 13:41

Refactoring: Extract passthruFields function for broader use

76275fa

Passthru fields that could sometimes by null

c2adca3

Do not report errors in xpack path as they will go into metricbeat index

5cb0622

Un-hardcode index name

248478a

More fixes

c57d056

Regenerating data.json

cfb04c3

Fixing order

5deed12

Renaming data JSON file to include version

e3b37fb

Renaming data.700.json back to data.json

182fa6e

Adding unit test fixture for 7.0.0

a2aba37

ycombinator merged commit b2416da into elastic:master Sep 11, 2018

ycombinator mentioned this pull request Sep 11, 2018

Cherry-pick #8192 to 6.x: Get rid of errors generated by xpack code for elasticsearch/shard metricset #8285

Merged

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Sep 25, 2018

ycombinator deleted the metricbeat-elasticsearch-shards-x-pack branch December 25, 2019 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of errors generated by xpack code for `elasticsearch/shard` metricset #8192

Get rid of errors generated by xpack code for `elasticsearch/shard` metricset #8192

ycombinator commented Aug 31, 2018 •

edited

Loading

ruflin Sep 3, 2018 •

edited

Loading

ycombinator Sep 4, 2018

ruflin Sep 5, 2018

ycombinator Sep 4, 2018 •

edited

Loading

ruflin Sep 5, 2018

ycombinator commented Sep 4, 2018 •

edited

Loading

ycombinator commented Sep 4, 2018

ruflin commented Sep 5, 2018

ruflin commented Sep 5, 2018

ycombinator commented Sep 11, 2018

Get rid of errors generated by xpack code for elasticsearch/shard metricset #8192

Get rid of errors generated by xpack code for elasticsearch/shard metricset #8192

Conversation

ycombinator commented Aug 31, 2018 • edited Loading

Testing this PR

ruflin Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

ycombinator Sep 4, 2018

Choose a reason for hiding this comment

ruflin Sep 5, 2018

Choose a reason for hiding this comment

ycombinator Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

ruflin Sep 5, 2018

Choose a reason for hiding this comment

ycombinator commented Sep 4, 2018 • edited Loading

ycombinator commented Sep 4, 2018

ruflin commented Sep 5, 2018

ruflin commented Sep 5, 2018

ycombinator commented Sep 11, 2018

Get rid of errors generated by xpack code for `elasticsearch/shard` metricset #8192

Get rid of errors generated by xpack code for `elasticsearch/shard` metricset #8192

ycombinator commented Aug 31, 2018 •

edited

Loading

ruflin Sep 3, 2018 •

edited

Loading

ycombinator Sep 4, 2018 •

edited

Loading

ycombinator commented Sep 4, 2018 •

edited

Loading