[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8445

ycombinator · 2018-09-25T23:17:16Z

Porting over elastic/elasticsearch#33860 to the Metricbeat Elasticsearch module (X-Pack Monitoring code path).

This PR teaches Elasticsearch X-Pack Monitoring to collect cluster metadata, if any is set, and index it into cluster_stats docs in .monitoring-es-6-mb-*.

After this PR, cluster_stats docs in .monitoring-es-6-mb-* will contain an additional top-level cluster_settings field like so:

{
   ...
   "cluster_settings": {
     "cluster": {
       "metadata": {
         ...
       }
     }
   }
}

ruflin · 2018-09-27T07:30:02Z

metricbeat/module/elasticsearch/elasticsearch.go

+	}
+
+	if !hasSettingGroup {
+		return common.MapStr{}, nil


Do you want to return here nil or an empty MapStr?

If we return nil here, then on the caller side we'll have to check for it and construct an empty MapStr. Otherwise this line will throw a runtime panic:

https://github.com/elastic/beats/blob/d38f1dd5801ab4fb7fb320aedc504f5e4ea060c2/metricbeat/module/elasticsearch/elasticsearch.go#L302

Thinking about the end JSON document, what is your suggestion on what should be there?

I decided that if there are no cluster settings, we should not have that field in the end JSON document either. This also makes it consistent with the internal ES collector implementation:

https://github.com/elastic/elasticsearch/blob/00e66bab362b1a8cd72c5fc483b8523c3888890e/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/cluster/ClusterStatsMonitoringDoc.java#L169-L184

I've implemented this logic in 43fed80.

ruflin

LGTM.

Only left a question for the nil part. Good with all options.

ycombinator · 2018-10-01T19:25:44Z

There is some discussion going on on the corresponding ES PR (elastic/elasticsearch#34023) so I'm holding off on making further changes to this PR for now.

ycombinator · 2018-11-05T19:00:26Z

The discussion in elastic/elasticsearch#34023 has settled and the PR has been merged so I'm resuming work on this PR. I will shortly bring it up to date with the ES internal collector implementation and also address the review feedback above. Stay tuned...

ycombinator · 2018-11-05T23:46:25Z

@ruflin I've made this PR's implementation now consistent with the internal ES collector implementation. Ready for your review again. Thanks!

ycombinator · 2018-11-06T00:32:25Z

jenkins, test this

ruflin

Code LGTM.

ruflin · 2018-11-06T12:03:49Z

metricbeat/module/elasticsearch/cluster_stats/data_xpack.go

@@ -142,6 +142,22 @@ func apmIndicesExist(clusterState common.MapStr) (bool, error) {
 	return false, nil
 }

+func getClusterMetadataSettings(m *MetricSet) (common.MapStr, error) {
+	// For security reasons we only get the display_name setting


Why is that?

Users can set whatever they want in the cluster metadata. They might set sensitive information, and we would end up indexing it into monitoring indices. So, to be on the safer side, we decided to just collect the specific key, display_name, from cluster metadata for now.

See complete discussion about this starting here: elastic/elasticsearch#34023 (comment).

Note that even though we only collect the specific key for now, we are indexing the complete nesting structure for the key. That way, if we later decide to allow more keys or all keys, there will be no break in the expected structure for the UI.

ruflin · 2018-11-06T12:04:22Z

Should we start to add changelog entries also for the internal monitoring?

ycombinator · 2018-11-07T12:47:16Z

Should we start to add changelog entries also for the internal monitoring?

Yeah, I think we can start doing that now. The modules are getting mature enough IMO. Added for this PR in 3226279.

@ruflin I also answered your question above: #8445 (comment). Does this PR still LGTY?

ycombinator · 2018-11-07T15:39:34Z

jenkins, test this

ycombinator · 2018-11-07T23:34:07Z

jenkins, test this

ycombinator · 2018-11-08T05:05:36Z

jenkins, test this

…elastic#8445) * Add cluster_metadata to cluster_stats docs * Fixing double nesting * Only collect display_name (for now) * Do not index cluster_settings field if there are no cluster settings * Adding CHANGELOG entry (cherry picked from commit 29be670)

…data to cluster_stats docs (#8990) Cherry-pick of PR #8445 to 6.x branch. Original message: Porting over elastic/elasticsearch#33860 to the Metricbeat Elasticsearch module (X-Pack Monitoring code path). This PR teaches Elasticsearch X-Pack Monitoring to collect cluster metadata, if any is set, and index it into `cluster_stats` docs in `.monitoring-es-6-mb-*`. After this PR, `cluster_stats` docs in `.monitoring-es-6-mb-*` will contain an additional top-level `cluster_settings` field like so: ``` { ... "cluster_settings": { "cluster": { "metadata": { ... } } } } ```

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

ycombinator requested a review from ruflin September 25, 2018 23:17

ruflin reviewed Sep 27, 2018

View reviewed changes

ruflin approved these changes Oct 1, 2018

View reviewed changes

ycombinator mentioned this pull request Oct 1, 2018

[Monitoring] Add cluster metadata to cluster_stats docs (#33860) elastic/elasticsearch#34023

Merged

ycombinator added in progress Pull request is currently in progress. and removed review labels Oct 1, 2018

ycombinator removed needs_backport PR is waiting to be backported to other branches. v6.5.0 labels Oct 14, 2018

ycombinator added 2 commits November 5, 2018 10:59

Add cluster_metadata to cluster_stats docs

141cf4d

Fixing double nesting

c305306

ycombinator force-pushed the metricbeat-elasticsearch-cluster-stats-add-cluster-settings-metadata branch from d38f1dd to c305306 Compare November 5, 2018 18:59

ycombinator added 2 commits November 5, 2018 15:22

Only collect display_name (for now)

de67cf1

Do not index cluster_settings field if there are no cluster settings

43fed80

ruflin approved these changes Nov 6, 2018

View reviewed changes

ycombinator added needs_backport PR is waiting to be backported to other branches. v6.6.0 labels Nov 6, 2018

Adding CHANGELOG entry

3226279

ycombinator added review and removed in progress Pull request is currently in progress. labels Nov 8, 2018

ruflin approved these changes Nov 8, 2018

View reviewed changes

ycombinator merged commit 29be670 into elastic:master Nov 8, 2018

ycombinator deleted the metricbeat-elasticsearch-cluster-stats-add-cluster-settings-metadata branch November 8, 2018 11:56

ycombinator mentioned this pull request Nov 8, 2018

Cherry-pick #8445 to 6.x: [Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8990

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8445

[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8445

ycombinator commented Sep 25, 2018

ruflin Sep 27, 2018

ycombinator Sep 28, 2018

ruflin Oct 1, 2018

ycombinator Nov 5, 2018 •

edited

Loading

ruflin left a comment

ycombinator commented Oct 1, 2018 •

edited

Loading

ycombinator commented Nov 5, 2018

ycombinator commented Nov 5, 2018

ycombinator commented Nov 6, 2018

ruflin left a comment

ruflin Nov 6, 2018

ycombinator Nov 6, 2018

ruflin commented Nov 6, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 8, 2018

[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8445

[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs #8445

Conversation

ycombinator commented Sep 25, 2018

ruflin Sep 27, 2018

Choose a reason for hiding this comment

ycombinator Sep 28, 2018

Choose a reason for hiding this comment

ruflin Oct 1, 2018

Choose a reason for hiding this comment

ycombinator Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

ycombinator commented Oct 1, 2018 • edited Loading

ycombinator commented Nov 5, 2018

ycombinator commented Nov 5, 2018

ycombinator commented Nov 6, 2018

ruflin left a comment

Choose a reason for hiding this comment

ruflin Nov 6, 2018

Choose a reason for hiding this comment

ycombinator Nov 6, 2018

Choose a reason for hiding this comment

ruflin commented Nov 6, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 7, 2018

ycombinator commented Nov 8, 2018

ycombinator Nov 5, 2018 •

edited

Loading

ycombinator commented Oct 1, 2018 •

edited

Loading