Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Monitoring] Add cluster metadata to cluster_stats docs #33860

Merged
merged 12 commits into from Sep 25, 2018
Merged

[Monitoring] Add cluster metadata to cluster_stats docs #33860

merged 12 commits into from Sep 25, 2018

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Sep 19, 2018 •
edited

Resolves #33691.

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

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

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@ycombinator ycombinator force-pushed the x-pack/monitoring/collect-cluster-metadata branch from 65263ff to 8f3dfb3 Compare September 19, 2018 15:20
@ycombinator
Copy link
Contributor Author

Chatting with @pickypg off-PR, he suggested I could simply grab the settings from cluster state, which is already available to the ClusterStatsCollector. I've started going down this much simpler path in 8d86d4f. Thanks @pickypg!

@ycombinator ycombinator added review and removed WIP labels Sep 19, 2018
@ycombinator
Copy link
Contributor Author

This is ready for an initial review now.

As discussed in #33691, I'm deliberately not adding anything to the index template or .monitoring-es-6-*, since we don't want to actually index the new cluster_settings field, just store it.

I don't believe we need any version checking in the code (owing to cluster metadata only being added in 6.5), but I'd like reviewers to confirm that.

I still need to add/update tests.

gwbrown
gwbrown approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the settings front (given adding tests as mentioned), but you may want another signoff from someone more familiar with monitoring.

pickypg
pickypg approved these changes Sep 20, 2018
View reviewed changes
Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. You won't need version checks because none of this leaves the node in a way that requires serialization between nodes.

@ycombinator ycombinator force-pushed the x-pack/monitoring/collect-cluster-metadata branch from 81404dc to f5d204e Compare September 24, 2018 16:36
@ycombinator ycombinator merged commit 9b86e9a into elastic:master Sep 25, 2018
@ycombinator ycombinator deleted the x-pack/monitoring/collect-cluster-metadata branch September 25, 2018 01:52
ycombinator added a commit to ycombinator/elasticsearch that referenced this pull request Sep 25, 2018
@ycombinator
[Monitoring] Add cluster metadata to cluster_stats docs (elastic#33860)
4cb9e0e 
* WIP

* Adding cluster metadata to cluster stats monitoring doc

* Fixing checkstyle errors

* Adding missing license header

* Updating tests

* Getting cluster settings from cluster state

* Removing more unnecessary changes

* Adding cluster metadata settings to cluster_stats docs

* Updating test to include cluster metadata

* Fixing checkstyle

* Guarding against NPE

* Updating test fixture
This was referenced Sep 25, 2018
Merged
Merged
Merged
atript8 pushed a commit to atript8/elasticsearch that referenced this pull request Sep 28, 2018
@ycombinator @atript8
[Monitoring] Add cluster metadata to cluster_stats docs (elastic#33860)
15c8836 
* WIP

* Adding cluster metadata to cluster stats monitoring doc

* Fixing checkstyle errors

* Adding missing license header

* Updating tests

* Getting cluster settings from cluster state

* Removing more unnecessary changes

* Adding cluster metadata settings to cluster_stats docs

* Updating test to include cluster metadata

* Fixing checkstyle

* Guarding against NPE

* Updating test fixture
@chrisronline chrisronline mentioned this pull request Oct 24, 2018
Merged
kcm pushed a commit that referenced this pull request Oct 30, 2018
@ycombinator @kcm
[Monitoring] Add cluster metadata to cluster_stats docs (#33860)
d607fb0 
* WIP

* Adding cluster metadata to cluster stats monitoring doc

* Fixing checkstyle errors

* Adding missing license header

* Updating tests

* Getting cluster settings from cluster state

* Removing more unnecessary changes

* Adding cluster metadata settings to cluster_stats docs

* Updating test to include cluster metadata

* Fixing checkstyle

* Guarding against NPE

* Updating test fixture
ycombinator added a commit to ycombinator/elasticsearch that referenced this pull request Nov 1, 2018
@ycombinator
[Monitoring] Add cluster metadata to cluster_stats docs (elastic#33860)
18a62dc 
* WIP

* Adding cluster metadata to cluster stats monitoring doc

* Fixing checkstyle errors

* Adding missing license header

* Updating tests

* Getting cluster settings from cluster state

* Removing more unnecessary changes

* Adding cluster metadata settings to cluster_stats docs

* Updating test to include cluster metadata

* Fixing checkstyle

* Guarding against NPE

* Updating test fixture
ycombinator added a commit that referenced this pull request Nov 5, 2018
@ycombinator
[Monitoring] Add cluster metadata to cluster_stats docs (#33860) (#34023
ee5aacb 
)

Backport of #33860 and #34040.

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

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

```
{
   ...
   "cluster_settings": {
     "cluster": {
       "metadata": {
         ...
       }
     }
   }
}
```
@ycombinator ycombinator mentioned this pull request Nov 8, 2018
Merged
ycombinator added a commit to elastic/beats that referenced this pull request Nov 8, 2018
@ycombinator
Cherry-pick #8445 to 6.x: [Elasticsearch Monitoring] Add cluster_meta…
31173e8 
…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": {
         ...
       }
     }
   }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pickypg pickypg pickypg approved these changes

@gwbrown gwbrown gwbrown approved these changes

@tlrx tlrx Awaiting requested review from tlrx

Assignees
No one assigned
Labels
:Data Management/Monitoring >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ycombinator @elasticmachine @pickypg @gwbrown @colings86