[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs#8445
[Elasticsearch Monitoring] Add cluster_metadata to cluster_stats docs#8445ycombinator merged 5 commits intoelastic:masterfrom ycombinator:metricbeat-elasticsearch-cluster-stats-add-cluster-settings-metadata
Conversation
There was a problem hiding this comment.
Do you want to return here nil or an empty MapStr?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Thinking about the end JSON document, what is your suggestion on what should be there?
There was a problem hiding this comment.
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:
I've implemented this logic in 43fed80.
ruflin
left a comment
There was a problem hiding this comment.
LGTM.
Only left a question for the nil part. Good with all options.
|
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. |
|
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... |
|
@ruflin I've made this PR's implementation now consistent with the internal ES collector implementation. Ready for your review again. Thanks! |
|
jenkins, test this |
| } | ||
|
|
||
| func getClusterMetadataSettings(m *MetricSet) (common.MapStr, error) { | ||
| // For security reasons we only get the display_name setting |
There was a problem hiding this comment.
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.
|
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? |
|
jenkins, test this |
1 similar comment
|
jenkins, test this |
|
jenkins, test this |
…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": { ... } } } } ```
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_statsdocs in.monitoring-es-6-mb-*.After this PR,
cluster_statsdocs in.monitoring-es-6-mb-*will contain an additional top-levelcluster_settingsfield like so: