Provide same data structure as X-Pack for ES node_stats #6807

ruflin · 2018-04-09T15:11:49Z

This PR adds a flag xpack.enabled to the Elasticsearch Metricbeat module. If enabled the data is transformed to the format as X-Pack Monitoring needs for the UI. It overwrites the index with monitoring.

A config to enable the feature currently looks as following:

metricbeat.modules:
- module: elasticsearch
  metricsets: ["node_stats"]
  hosts: ["localhost:9200"]
  period: 1s
  xpack.enabled: true

Data is sent to the index .monitoring-es-6-mb-{date}. This is hard coded.
To enabled the feature xpack.enabled has to be set in the module
If a node is master is checked on each request as it can change over time. The master request is not atomic so this information could be inaccurate. The master detection also only works correctly, if data is fetched from only 1 node.
The cluser_uuid is fetched on the first request and cached from then based on the assumption that the cluster_id for a node id does not change over time.
limit_in_bytes and usage_in_bytes are provided as string by ES and not converted to int as they could overflow in ES.
Experimental message is logged when used. The feature is not documented yet on purpose.

As the data goes into a separate index it is expected that monitoring already has loaded the template.

houndci-bot · 2018-04-09T15:11:55Z

metricbeat/module/elasticsearch/elasticsearch.go

+		Even better would be to have the cluster id as part of the response
+	*/
+
+	clusterId := "1234"


var clusterId should be clusterID

houndci-bot · 2018-04-09T15:11:55Z

metricbeat/module/elasticsearch/elasticsearch.go

+func GetClusterId(nodeId, clusterName string) string {
+
+	// Check if cluster id already cached. If yes, return it.
+	if clusterId, ok := clusterIdCache[nodeId]; ok {


var clusterId should be clusterID

houndci-bot · 2018-04-09T15:11:55Z

metricbeat/module/elasticsearch/elasticsearch.go

+// Global clusterIdCache. Assumption is that the same node id never can belong to a different cluster id
+var clusterIdCache = map[string]string{}
+
+func GetClusterId(nodeId, clusterName string) string {


exported function GetClusterId should have comment or be unexported
func GetClusterId should be GetClusterID
func parameter nodeId should be nodeID

houndci-bot · 2018-04-09T15:11:55Z

metricbeat/module/elasticsearch/elasticsearch.go

+package elasticsearch
+
+// Global clusterIdCache. Assumption is that the same node id never can belong to a different cluster id
+var clusterIdCache = map[string]string{}


var clusterIdCache should be clusterIDCache

houndci-bot · 2018-04-09T15:11:56Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

@@ -0,0 +1,220 @@
+package node_stats


don't use an underscore in package name

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+	// 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)


var clusterId should be clusterID

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+	// it will provid the data for multiple nodes. This will mean the detection of the
+	// 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 {


range var nodeId should be nodeID

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/elasticsearch.go

+	json.Unmarshal(content, &nodesStruct)
+
+	// _local will only fetch one node info. First entry is node name
+	for k, _ := range nodesStruct.Nodes {


should omit 2nd value from range; this loop is equivalent to for k := range ...

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/elasticsearch.go

+func GetClusterId(http *helper.HTTP, uri string, nodeId string) (string, error) {
+
+	// Check if cluster id already cached. If yes, return it.
+	if clusterId, ok := clusterIdCache[nodeId]; ok {


var clusterId should be clusterID

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/elasticsearch.go

+	ClusterId   string `json:"cluster_uuid"`
+}
+
+func GetClusterId(http *helper.HTTP, uri string, nodeId string) (string, error) {


exported function GetClusterId should have comment or be unexported
func GetClusterId should be GetClusterID
func parameter nodeId should be nodeID

houndci-bot · 2018-04-11T11:12:24Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+type infoStruct struct {
+	ClusterName string `json:"cluster_name"`
+	ClusterId   string `json:"cluster_uuid"`


struct field ClusterId should be ClusterID

houndci-bot · 2018-04-11T11:12:25Z

metricbeat/module/elasticsearch/elasticsearch.go

+)
+
+// Global clusterIdCache. Assumption is that the same node id never can belong to a different cluster id
+var clusterIdCache = map[string]string{}


var clusterIdCache should be clusterIDCache

pickypg

This looks like it's in good shape. Left some thoughts.

pickypg · 2018-04-11T21:03:57Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+	// Parses the uri to replace the path
+	u, _ := url.Parse(uri)
+	u.Path = "_cluster/state/master_node"


We can probably call GET /_cluster/state/master_node?local=true to avoid having to bounce to the elected master to find out what this node thinks. Since we have to asynchronously fetch this, it's a race condition regardless. If it's wrong, it'll fix itself on the next pass.

Nice, wasn't aware of this param. That means we also get an answer in case the node is disconnected I assume.

pickypg · 2018-04-11T21:05:57Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+	// Parses the uri to replace the path
+	u, _ := url.Parse(uri)
+	u.Path = ""


It's a shame that we have to call this to get the cluster_uuid after invoking the GET /_cluster/state/master_node. Perhaps we can get it added over there so we can do a single request to fetch both details.

That would be great. Can you take this up with the ES team as you probably know best where this change would have to happen?

elastic/elasticsearch#30143

pickypg · 2018-04-11T21:07:20Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+func getNodeName(http *helper.HTTP, uri string) (string, error) {
+
+	// Parses the uri to replace the path


Lines 64-73 are the same for the next 2 methods except for u.Path. Perhaps we can have a helper method that avoids repeating it all?

pickypg · 2018-04-11T21:14:21Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+	// from the path and Metricbeat is not installed on the same machine as the node
+	// it will provid the data for multiple nodes. This will mean the detection of the
+	// 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.


In order to support proxies, we could allow users to specify the node name that they want Beats to monitor and, if unspecified, use _local as the name.

Pitfalls:

Misconfiguration means it's broken.

Duplicate node names can cause confusion.

Requires the module's configuration to be reloaded if the node is renamed.

Advantages:

We'll work, by default, for non-proxies.

We can work through proxies.

That is pretty interesting. I would skip it for the first iteration but I'm pretty sure that is a request that will pop up.

It will probably be a bit tricker for dynamic environments where the user does not have much control around the naming of nodes.

In that case they can set its address, which hopefully they know.

pickypg · 2018-04-11T21:19:48Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+		nodeData, _ := schemaXpack.Apply(node)
+		nodeStats := common.MapStr{
+			// This value is currently hard coded as it's not available in node_stats endpoint
+			"mlockall":    false,


We can ignore this value ~~, which was a holdover from when it was in _nodes/stats~~. We do not use it and it should be in an information document [that does not exist] -- not a stats one.

In case you're wondering, you could fetch it via GET /_nodes/_local/process.

houndci-bot · 2018-04-16T09:46:46Z

metricbeat/module/elasticsearch/elasticsearch.go

+func GetClusterID(http *helper.HTTP, uri string, nodeID string) (string, error) {
+
+	// Check if cluster id already cached. If yes, return it.
+	if clusterId, ok := clusterIDCache[nodeID]; ok {


var clusterId should be clusterID

ruflin · 2018-04-16T09:47:49Z

Review feedback addressed. We should leave this open until our last 6.x merge from master for 6.3 is done.

ruflin · 2018-04-16T09:20:10Z

metricbeat/mb/event.go

@@ -60,6 +61,12 @@ func (e *Event) BeatEvent(module, metricSet string, modifiers ...EventModifier)
 		e.MetricSetFields = nil
 	}

+	// Set index prefix
+	// TODO: does this break if user sets an index himself?


@urso What will happen here with user defined index patterns? Does this get overwritten?

ruflin · 2018-04-16T09:28:30Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+func getNodeName(http *helper.HTTP, uri string) (string, error) {
+
+	// Parses the uri to replace the path


ruflin · 2018-04-16T09:35:54Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+	// Parses the uri to replace the path
+	u, _ := url.Parse(uri)
+	u.Path = "_cluster/state/master_node"


Nice, wasn't aware of this param. That means we also get an answer in case the node is disconnected I assume.

ruflin · 2018-04-16T09:36:39Z

metricbeat/module/elasticsearch/elasticsearch.go

+
+	// Parses the uri to replace the path
+	u, _ := url.Parse(uri)
+	u.Path = ""


That would be great. Can you take this up with the ES team as you probably know best where this change would have to happen?

ruflin · 2018-04-16T09:44:17Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+	// from the path and Metricbeat is not installed on the same machine as the node
+	// it will provid the data for multiple nodes. This will mean the detection of the
+	// 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.


That is pretty interesting. I would skip it for the first iteration but I'm pretty sure that is a request that will pop up.

It will probably be a bit tricker for dynamic environments where the user does not have much control around the naming of nodes.

ruflin · 2018-04-16T09:45:48Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+		nodeData, _ := schemaXpack.Apply(node)
+		nodeStats := common.MapStr{
+			// This value is currently hard coded as it's not available in node_stats endpoint
+			"mlockall":    false,


Extracted from elastic#6807

To allow more flexibility the Elasticsearch node_stats metricset is updated to use the ReporverV2 interface. This is a subset of elastic#6807. * Test data set for ES 6.2.3 added * Reporting of error if json decoding fails * Update node_stats integration tests to use reporter interface * Update generator to also add metricset info * Update data.json for node_stats and index * Set namespace as part of registration * Add service.name to event

To allow more flexibility the Elasticsearch node_stats metricset is updated to use the ReporverV2 interface. This is a subset of #6807. * Test data set for ES 6.2.3 added * Reporting of error if json decoding fails * Update node_stats integration tests to use reporter interface * Update generator to also add metricset info * Update data.json for node_stats and index * Set namespace as part of registration * Add service.name to event

This PR adds a flag `xpack.enabled` to the Elasticsearch Metricbeat module. If enabled the data is transformed to the format as X-Pack Monitoring needs for the UI. It overwrites the index with `monitoring`. A config to enable the feature currently looks as following: ``` metricbeat.modules: - module: elasticsearch metricsets: ["node_stats"] hosts: ["localhost:9200"] period: 1s xpack.enabled: true ``` * Data is sent to the index `.monitoring-es-6-mb-{date}`. This is hard coded. * To enabled the feature `xpack.enabled` has to be set in the module * If a node is master is checked on each request as it can change over time. The master request is not atomic so this information could be inaccurate. The master detection also only works correctly, if data is fetched from only 1 node. * The cluser_uuid is fetched on the first request and cached from then based on the assumption that the cluster_id for a node id does not change over time. * `limit_in_bytes` and `usage_in_bytes` are provided as string by ES and not converted to int as they could overflow in ES. * Experimental message is logged when used. The feature is not documented yet on purpose. As the data goes into a separate index it is expected that monitoring already has loaded the template.

ruflin · 2018-04-26T06:35:33Z

This PR is ready for an other review.

As soon as elastic/elasticsearch#30143 is merged we can do a follow up PR for further simplifying the collection.

ruflin · 2018-04-26T06:40:01Z

One additional note on elastic/elasticsearch#30143 As this will be only for 6.4 forward we will still need the lookup for earlier versions and it means we have to detect which version we are monitoring.

exekias · 2018-04-26T13:31:50Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

+			"node_master": isMaster,
+			"node_id":     nodeID,
+		}
+		nodeStats.DeepUpdate(nodeData)


You could also use:

nodeData["node_master"] = isMaster nodeData["node_id"] = nodeId

That would avoid DeepUpdate iterating over all fields

Good point, changed.

exekias

LGTM, left a comment

ruflin added in progress Pull request is currently in progress. module discuss Issue needs further discussion. Metricbeat Metricbeat labels Apr 9, 2018

houndci-bot reviewed Apr 9, 2018

View reviewed changes

ruflin force-pushed the es-node-stats branch from 72e720f to 907e0cd Compare April 11, 2018 11:12

houndci-bot reviewed Apr 11, 2018

View reviewed changes

pickypg reviewed Apr 11, 2018

View reviewed changes

ruflin changed the title ~~[Experiment] Provide same data structure as X-Pack for ES node_stats~~ Provide same data structure as X-Pack for ES node_stats Apr 16, 2018

houndci-bot reviewed Apr 16, 2018

View reviewed changes

ruflin force-pushed the es-node-stats branch from 9dc6c69 to fbce9c0 Compare April 16, 2018 09:46

ruflin commented Apr 16, 2018

View reviewed changes

ruflin added a commit to ruflin/beats that referenced this pull request Apr 16, 2018

Add support for float type in mapstriface

7af007a

Extracted from elastic#6807

This was referenced Apr 16, 2018

Add support for float type in mapstriface #6870

Merged

Index metricset for elasticsearch Metricbeat module #6881

Merged

ruflin force-pushed the es-node-stats branch from 78b92ed to 6cd42b9 Compare April 20, 2018 19:35

ruflin mentioned this pull request Apr 23, 2018

Update elasticsearch.node_stats to use ReporterV2 #6917

Merged

ruflin force-pushed the es-node-stats branch from d01b486 to 31ee9d5 Compare April 25, 2018 11:11

ruflin added review and removed discuss Issue needs further discussion. labels Apr 25, 2018

pickypg mentioned this pull request Apr 25, 2018

_cluster/state should always return cluster_uuid elastic/elasticsearch#30143

Merged

ruflin removed the in progress Pull request is currently in progress. label Apr 26, 2018

ruflin force-pushed the es-node-stats branch from 31ee9d5 to 5633d7a Compare April 26, 2018 06:34

exekias reviewed Apr 26, 2018

View reviewed changes

remove deep update

7b01883

exekias merged commit ed840fd into elastic:master Apr 26, 2018


		func getNodeName(http *helper.HTTP, uri string) (string, error) {

		// Parses the uri to replace the path

Provide same data structure as X-Pack for ES node_stats #6807

Provide same data structure as X-Pack for ES node_stats #6807

Conversation

ruflin commented Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickypg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickypg Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Apr 26, 2018

ruflin commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias left a comment

Choose a reason for hiding this comment

ruflin commented Apr 9, 2018 •

edited

Loading

pickypg Apr 11, 2018 •

edited

Loading