Rabbitmq node only reports metrics of itself by default #6971

jsoriano · 2018-04-28T11:04:19Z

A configuration option is added, node.collect that can be set to
cluster to have the previous behaviour. The default is node.

Fixes #6556

houndci-bot · 2018-04-28T11:04:29Z

metricbeat/module/rabbitmq/node/node.go

+	return &apiOverview, nil
+}
+
+func (m *MetricSet) Fetch() (common.MapStr, error) {


exported method MetricSet.Fetch should have comment or be unexported

houndci-bot · 2018-04-28T11:04:30Z

metricbeat/module/rabbitmq/node/node.go

 	}, nil
 }

-func (m *MetricSet) Fetch() ([]common.MapStr, error) {
-	content, err := m.HTTP.FetchContent()
+type ApiOverview struct {


exported type ApiOverview should have comment or be unexported
type ApiOverview should be APIOverview

houndci-bot · 2018-04-28T11:04:30Z

metricbeat/module/rabbitmq/node/node.go

+	return &apiOverview, nil
+}
+
+func (m *MetricSet) Fetch() (common.MapStr, error) {


exported method MetricSet.Fetch should have comment or be unexported

houndci-bot · 2018-04-28T11:04:30Z

metricbeat/module/rabbitmq/node/node.go

 	}, nil
 }

-func (m *MetricSet) Fetch() ([]common.MapStr, error) {
-	content, err := m.HTTP.FetchContent()
+type ApiOverview struct {


exported type ApiOverview should have comment or be unexported
type ApiOverview should be APIOverview

ruflin

I like the change to the default to only have 1 node by default. I'm wondering if there should be a config option to still be able to receive information for all nodes. This has the advantage that it allows to monitor rabbitmq nodes also in the case Metricbeat can't be installed on each node and is monitored from a central place. This can also happen in a follow up PR. WDYT?

ruflin · 2018-04-30T07:44:49Z

metricbeat/module/rabbitmq/node/node.go

 	}, nil
 }

-func (m *MetricSet) Fetch() ([]common.MapStr, error) {
-	content, err := m.HTTP.FetchContent()
+type apiOverview struct {


We normally keep all the mapping code in data.json. But happy to change this convention :-)

jsoriano · 2018-04-30T15:31:52Z

Ok, agree to allow the user to still collect metrics from all the nodes.

houndci-bot · 2018-04-30T16:32:38Z

metricbeat/module/rabbitmq/node/node.go

+	overview *helper.HTTP
+}
+
+type AllMetricSet struct {


exported type AllMetricSet should have comment or be unexported

houndci-bot · 2018-04-30T16:32:38Z

metricbeat/module/rabbitmq/node/node.go

@@ -27,33 +34,108 @@ func init() {
 	)
 }

-type MetricSet struct {
+type SelfMetricSet struct {


exported type SelfMetricSet should have comment or be unexported

houndci-bot · 2018-04-30T16:32:38Z

metricbeat/module/rabbitmq/node/config.go

+package node
+
+const (
+	ConfigModeSelf = "self"


exported const ConfigModeSelf should have comment (or a comment on this block) or be unexported

jsoriano · 2018-04-30T16:34:13Z

Added node.mode config flag to choose between self (new default) and all (previous behaviour), to collect metrics only from the node we are connecting to, or from the whole cluster.

jsoriano · 2018-04-30T16:45:16Z

CHANGELOG.asciidoc

@@ -86,6 +86,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
 - Don't stop Metricbeat if aerospike server is down. {pull}6874[6874]
 - disk reads and write count metrics in RabbitMQ queue metricset made optional. {issue}6876[6876]
 - Add mapping for docker metrics per cpu. {pull}6843[6843]
+- RabbitMQ node metricset only collects metrics of the instance it connects to, `node.mode: all` can be used to collect all nodes as before. {issue}6556[6556] {pull}6971[6971]


I wonder if we should move it to breaking changes.

Going forth and back on this too. It is a breaking change but because the previous implementation was a bug instead of a feature.

We could move it under breaking change but mention there that it was a bug?

ruflin · 2018-05-02T06:12:45Z

metricbeat/module/rabbitmq/node/_meta/docs.asciidoc

@@ -1 +1,10 @@
 This is the `node` metricset of the RabbitMQ module.
+
+It collects metrics of nodes, it has two modes that can be selected with the


This is the node metricset of the RabbitMQ module and collects metrics about RabbitMQ nodes.

The metricset has two modes to collect data which can be selected with the node.mode setting:

ruflin · 2018-05-02T06:14:48Z

metricbeat/module/rabbitmq/node/_meta/docs.asciidoc

+It collects metrics of nodes, it has two modes that can be selected with the
+`node.mode` setting:
+
+* `self`: collects metrics only from the node from whose endpoint `metricbeat`


collects metrics only from the node metricbeat is connected to.

Should we add a note here that the recommended one is localhost? In the docker case multiple nodes will be configured in the hosts but still self should be used.

ruflin · 2018-05-02T06:17:36Z

metricbeat/module/rabbitmq/node/_meta/docs.asciidoc

+* `self`: collects metrics only from the node from whose endpoint `metricbeat`
+  is collecting metrics. This is the default, and the recommended mode when
+  `metricbeat` is deployed in the same node as the RabbitMQ server.
+* `all`: collects metrics from all the nodes. This is recommended when


Should we call the config node.collection and the options are node or cluster. I'm trying to find an other name for mode that better describes what it does and the config option on what data it collects. Either it's only from the node or from all nodes / cluster. Writing this perhaps node and all would fit?

Ok, changing to node.collect with values node and cluster.

ruflin · 2018-05-02T06:21:45Z

metricbeat/module/rabbitmq/node/node.go

+}
+
+// Fetch metrics from all rabbitmq nodes in the cluster
+func (m *AllMetricSet) Fetch() ([]common.MapStr, error) {


Instead of using 2 different MetricSet types we could move the Metricset to the reporter interface instead (see elasticsearch metricsets). Like this we would not have to make a difference if one or multiple events are reported.

houndci-bot · 2018-05-09T16:32:45Z

metricbeat/module/rabbitmq/node/node.go

@@ -27,33 +33,113 @@ func init() {
 	)
 }

-type MetricSet struct {
+// NodeMetricSet is the MetricSet type used when node.collect is "node"
+type NodeMetricSet struct {


type name will be used as node.NodeMetricSet by other packages, and that stutters; consider calling this MetricSet

jsoriano · 2018-05-09T16:33:41Z

Implemented using reporter V2 api now.

jsoriano · 2018-05-09T16:38:58Z

metricbeat/module/rabbitmq/node/data.go

-func eventMapping(node map[string]interface{}) (common.MapStr, *s.Errors) {
-	return schema.Apply(node)
+func eventMapping(r mb.ReporterV2, node map[string]interface{}) {
+	event, _ := schema.Apply(node)


I tried not to ignore errors here, but Apply is returning an error that seems like an empty list of missing fields (Literally: "Required fields are missing: "). This happens also with the existing code, so this is not a regression. I leave investigating this for a future PR.

Yes, we need a better general approach here. LGTM for now. Check out my recent changes in the elasticsearch module. There I use the errors for testing purpose only.

ruflin · 2018-05-11T12:39:27Z

jenkins, test this

A configuration option is added, `node.collect` that can be set to `cluster` to have the previous behaviour. The default is `node`.

jsoriano added module review Metricbeat Metricbeat labels Apr 28, 2018

houndci-bot reviewed Apr 28, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch 3 times, most recently from 8a51a6a to b5cb2d8 Compare April 28, 2018 11:09

ruflin reviewed Apr 30, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch from b5cb2d8 to 6963b42 Compare April 30, 2018 16:32

houndci-bot reviewed Apr 30, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch from 6963b42 to 2350220 Compare April 30, 2018 16:38

jsoriano changed the title ~~Rabbitmq node only reports metrics of itself~~ Rabbitmq node only reports metrics of itself by default Apr 30, 2018

jsoriano commented Apr 30, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch from 2350220 to 36d43ae Compare May 1, 2018 10:13

ruflin reviewed May 2, 2018

View reviewed changes

jsoriano added in progress Pull request is currently in progress. and removed review labels May 8, 2018

jsoriano force-pushed the rabbitmq-self-node branch from 36d43ae to d292b1f Compare May 9, 2018 16:32

houndci-bot reviewed May 9, 2018

View reviewed changes

jsoriano commented May 9, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch 2 times, most recently from 7fd0bde to ed5c789 Compare May 9, 2018 16:49

jsoriano added review and removed in progress Pull request is currently in progress. labels May 11, 2018

ruflin approved these changes May 11, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-self-node branch 2 times, most recently from 9ea5ee9 to 19fab91 Compare May 15, 2018 08:32

Rabbitmq node only reports metrics of itself by default

4ea4b0d

A configuration option is added, `node.collect` that can be set to `cluster` to have the previous behaviour. The default is `node`.

jsoriano force-pushed the rabbitmq-self-node branch from 19fab91 to 4ea4b0d Compare May 15, 2018 08:54

ruflin merged commit f85dbf8 into elastic:master May 15, 2018

ruflin mentioned this pull request May 16, 2018

[Metricbeat] Document Cluster Level vs. Node Level Metricsets #7110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rabbitmq node only reports metrics of itself by default #6971

Rabbitmq node only reports metrics of itself by default #6971

jsoriano commented Apr 28, 2018 •

edited

Loading

houndci-bot Apr 28, 2018

houndci-bot Apr 28, 2018

houndci-bot Apr 28, 2018

houndci-bot Apr 28, 2018

ruflin left a comment

ruflin Apr 30, 2018

jsoriano commented Apr 30, 2018

houndci-bot Apr 30, 2018

houndci-bot Apr 30, 2018

houndci-bot Apr 30, 2018

jsoriano commented Apr 30, 2018

jsoriano Apr 30, 2018

ruflin May 2, 2018

ruflin May 2, 2018

ruflin May 2, 2018

ruflin May 2, 2018

jsoriano May 4, 2018

ruflin May 2, 2018

houndci-bot May 9, 2018

jsoriano commented May 9, 2018

jsoriano May 9, 2018

ruflin May 11, 2018

ruflin commented May 11, 2018

		@@ -1 +1,10 @@
		This is the `node` metricset of the RabbitMQ module.

		It collects metrics of nodes, it has two modes that can be selected with the

Rabbitmq node only reports metrics of itself by default #6971

Rabbitmq node only reports metrics of itself by default #6971

Conversation

jsoriano commented Apr 28, 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

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented May 11, 2018

jsoriano commented Apr 28, 2018 •

edited

Loading