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.

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

Various minor fixes to elasticsearch/node_stats metricset x-pack code #8474

Conversation

ycombinator
Copy link
Contributor

Based on recent testing I found a few issues in the elasticsearch/nodes_stats metricset's x-pack code path:

  • The ES Node Stats API doesn't report load stats for 1m, 5m, and 15m until it has collected them. So we should make these fields optional in our schema.

  • The ES Node Stats API doesn't report cgroup stats unless ES is running in a container. So we should make this field optional in our schema.

  • The code was incorrectly double nesting the license field, that is "license": { "license": { ... } }.

  • The code was calling other ES APIs but not passing the correct reset URI to the functions making these API calls. As a result only one node_stats document would get indexed into .monitoring-es-6-mb-* the first time the metricset's Fetch() function. After that no new node_stats documents would get indexed because the content received by the eventMappingXPack function did not contain Node Stats API response data!

This PR fixes these issues.

@ycombinator 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 27, 2018
@ycombinator ycombinator requested a review from ruflin September 27, 2018 20:13
@@ -188,6 +160,12 @@ var (
}),
}),
}

threadPoolStatsSchema = s.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice simplification.

@@ -204,13 +182,13 @@ func eventsMappingXPack(r mb.ReporterV2, m *MetricSet, content []byte) {
// master node will not be accurate anymore as often in these cases a proxy is in front
// of ES and it's not know if the request will be routed to the same node as before.
for nodeID, node := range nodesStruct.Nodes {
clusterID, err := elasticsearch.GetClusterID(m.HTTP, m.HostData().SanitizedURI, nodeID)
clusterID, err := elasticsearch.GetClusterID(m.HTTP, m.HTTP.GetURI(), nodeID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have CI detecting such issue in the future.

@ycombinator ycombinator merged commit 22e4022 into elastic:master Oct 1, 2018
@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Oct 1, 2018
ycombinator added a commit that referenced this pull request Oct 2, 2018
…#8474) (#8511)

* Making load stats optional because they may not have been collected yet

* Making cgroups stats optional as ES may not be in a containerized environment

* Reusing schema

* Fixing license field

* Fixing reset URI

(cherry picked from commit 22e4022)
@ycombinator ycombinator deleted the metricbeat-elasticsearch-node-stats-fixes branch December 25, 2019 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants