Rabbitmq exchanges metricset #6607

kvalev · 2018-03-20T17:50:52Z

This pull requests add a new metricset for RabbitMQ exchanges.

Related to #6442

elasticmachine · 2018-03-20T17:50:53Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

ruflin

Thanks for the contribution. Left a few comments but still need to have a closer look.

ruflin · 2018-03-21T13:12:08Z

metricbeat/docs/modules/rabbitmq.asciidoc

@@ -21,7 +21,7 @@ in <<configuration-metricbeat>>. Here is an example configuration:
 ----
 metricbeat.modules:
 - module: rabbitmq
-  metricsets: ["node", "queue", "connection"]
+  metricsets: ["node", "queue", "connection", "exchange"]


Reminds me we should sort metricsets alphabetical (I can do that in a follow up PR).

ruflin · 2018-03-21T13:13:36Z

metricbeat/module/rabbitmq/exchange/_meta/data.json

@@ -0,0 +1,19 @@
+{


Any chance you could create a data.json here? The command you need is as following:

go test -tags=integration github.com/elastic/beats/metricbeat/module/rabbitmq/exchange/... -data

It assumes you have rabbitmq running on localhost.

This command does not generate the full json file (the message stats are missing). Am I missing something?

{ "@timestamp": "2017-10-12T08:05:34.853Z", "beat": { "hostname": "host.example.com", "name": "host.example.com" }, "metricset": { "host": "localhost:15672", "module": "rabbitmq", "name": "exchange", "rtt": 115 }, "rabbitmq": { "exchange": { "arguments": {}, "auto_delete": false, "durable": true, "internal": false, "name": "", "type": "direct", "vhost": "/" } } }

I wonder if something is wrong in the metricset if they are missing?

ruflin · 2018-03-21T13:15:07Z

metricbeat/module/rabbitmq/exchange/data.go

+					"rate": c.Float("rate"),
+				}),
+			},
+			"publish_in": s.Object{


We could make the above publish.total.count and this one here publish.in and the following publish.out. Is it correct the the above is just the total?

The advantage of keeping it as publish_in is that all the groups of metrics under messages have the same structure, and they are also kept as a more direct mapping of source metrics.

I'm always in favor or having all the related metrics under on group but it's really hard to have a clear rule here. I'm good with both.

TBH I am not sure what "publish" is supposed to be. The official documentation says "Count of messages published.", but in my environment it is always 0, although publish_in and publish_out are both 0+.
Regarding the naming - it is up to you as I am not familiar with the metric set naming guidelines.

Let's keep it as is for now

ruflin · 2018-03-21T13:18:10Z

metricbeat/module/rabbitmq/exchange/exchange_test.go

+	assert.EqualValues(t, false, event["auto_delete"])
+	assert.EqualValues(t, false, event["internal"])
+
+	messages := event["messages"].(common.MapStr)


I wonder if for the comparison here you could create a common.MapStr with all fields inside with the expected values and then compare it? Would probably make the code a bit more readable and easier to extend.

I tried doing that, but the Equals comparison fails for some reason, although the maps are identical (albeit in a different order):

Error: Not equal: expected: common.MapStr{"return_unroutable":common.MapStr{"count":40, "details":common.MapStr{"rate":123.123}}, "redeliver":common.MapStr{"count":30, "details":common.MapStr{"rate":0.123}}, "publish":common.MapStr{"count":123, "details":common.MapStr{"rate":0.1}}, "publish_in":common.MapStr{"count":100, "details":common.MapStr{"rate":0.5}}, "publish_out":common.MapStr{"count":99, "details":common.MapStr{"rate":0.9}}, "ack":common.MapStr{"count":60, "details":common.MapStr{"rate":12.5}}, "deliver_get":common.MapStr{"details":common.MapStr{"rate":43.21}, "count":50}, "confirm":common.MapStr{"count":120, "details":common.MapStr{"rate":98.63}}} actual : common.MapStr{"ack":common.MapStr{"count":60, "details":common.MapStr{"rate":12.5}}, "deliver_get":common.MapStr{"count":50, "details":common.MapStr{"rate":43.21}}, "confirm":common.MapStr{"count":120, "details":common.MapStr{"rate":98.63}}, "return_unroutable":common.MapStr{"count":40, "details":common.MapStr{"rate":123.123}}, "redeliver":common.MapStr{"count":30, "details":common.MapStr{"rate":0.123}}, "publish":common.MapStr{"count":123, "details":common.MapStr{"rate":0.1}}, "publish_in":common.MapStr{"count":100, "details":common.MapStr{"rate":0.5}}, "publish_out":common.MapStr{"details":common.MapStr{"rate":0.9}, "count":99}}

Lets leave it as is then for now. I remember there was something like DeepEqual but we can look into this later.

jsoriano · 2018-03-28T17:18:11Z

@kvalev branch needs rebase

spotlesscoder · 2018-04-11T19:49:06Z

@kvalev will you find the time to finish this PR in the near future? I need this feature soon

ruflin · 2018-04-12T06:10:53Z

@kvalev I case you can't find the time to finish this PR, lets us know so we can take it over (or perhaps @CodingSpiderFox? ) to get it merged.

ruflin

@kvalev Just realised that there was also some feedback from our side missing. Sorry about that. Could you rebase the PR on master?

ruflin · 2018-04-13T06:20:39Z

metricbeat/module/rabbitmq/exchange/_meta/data.json

@@ -0,0 +1,19 @@
+{


I wonder if something is wrong in the metricset if they are missing?

ruflin · 2018-04-13T06:21:16Z

metricbeat/module/rabbitmq/exchange/data.go

+					"rate": c.Float("rate"),
+				}),
+			},
+			"publish_in": s.Object{


Let's keep it as is for now

spotlesscoder · 2018-04-17T11:12:56Z

I'm not good doing git stuff. Still learning. How can I do the rebase? Fork this repo, pull the branch kvalev and push it to my repo, rebase it on master and then open another PR?

kvalev · 2018-04-17T18:16:40Z

@ruflin I verified it again against my local RabbitMQ instance and it seems that in case the exchanges were not used recently (probably since last restart), the messages_stats section is missing. Even if there were some messages, not all subsection will be returned by the RabbitMQ - only the ones that are not 0. This is also the reason why the data.json generation returns a very basic data skeleton. If I understand it correctly, the data generation uses the Dockerfile to start a RabbitMQ instance and fetch the data from there. However given that the instance was just started it does not report any stats back. If I publish some test messages (using rabbitmqadmin), parts of the stats sections are present. Should I change the Dockerfile to add some test data, so that the data.json generation returns something meaningful? I dont know whether this will break something else.
I also made all exchange stat sections optional - I hope this was the right way to do it.
I also had to cast all integers to int64 in the unit test for the map comparison to work.

jsoriano · 2018-04-18T08:04:39Z

jenkins, test this please

ruflin · 2018-04-19T11:45:11Z

@kvalev I like the idea of populating some data before the test. It could also be part of the test instead of the docker image, like this it also work if done with a local instance.

spotlesscoder · 2018-04-25T11:45:24Z

@kvalev I like the idea of populating some data before the test. It could also be part of the test instead of the docker image, like this it also work if done with a local instance.

So what is the actual TODO here?

jsoriano · 2018-04-26T11:02:09Z

I have created PR #6955 to resolve pending issues

Continues with #6607

ruflin · 2018-05-08T08:07:53Z

Closing as #6955 was merged. @kvalev Thanks a lot for your work and kicking this off.

Continues with elastic#6607

kvalev added 3 commits March 20, 2018 18:48

RabbitMQ exchange metricset

0fd1b3e

Update documentation and register exchange metricset

c367e42

Code documentation

36aebae

Update CHANGELOG

5719f84

kvalev mentioned this pull request Mar 20, 2018

Rabbitmq exchange metricset and queue message rates (#6442) #6586

Closed

Updated documentation

34b28f2

ruflin added module review Metricbeat Metricbeat labels Mar 21, 2018

ruflin reviewed Mar 21, 2018

View reviewed changes

ruflin requested a review from jsoriano March 21, 2018 13:18

ruflin reviewed Apr 13, 2018

View reviewed changes

kvalev and others added 5 commits April 17, 2018 18:29

Merge branch 'master' into rabbitmq_exchanges_metricset

6102a3e

Alphabetical metricset order

16a547d

Make all exchange metric stats optional

73a70cb

Recreated sample data.json file

3e95364

Simplify exchanges unit test

fa9039e

jsoriano mentioned this pull request Apr 26, 2018

Rabbitmq exchanges metricset #6955

Merged

ruflin pushed a commit that referenced this pull request May 8, 2018

Rabbitmq exchanges metricset (#6955)

c522fd1

Continues with #6607

ruflin closed this May 8, 2018

stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018

Rabbitmq exchanges metricset (elastic#6955)

227c877

Continues with elastic#6607

stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018

Rabbitmq exchanges metricset (elastic#6955)

9ab30ac

Continues with elastic#6607

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rabbitmq exchanges metricset #6607

Rabbitmq exchanges metricset #6607

kvalev commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

ruflin left a comment

ruflin Mar 21, 2018

ruflin Mar 21, 2018

kvalev Mar 21, 2018

kvalev Mar 22, 2018

ruflin Apr 13, 2018

ruflin Mar 21, 2018

jsoriano Mar 21, 2018

ruflin Mar 22, 2018

kvalev Mar 22, 2018

ruflin Apr 13, 2018

ruflin Mar 21, 2018

kvalev Apr 12, 2018

ruflin Apr 13, 2018

jsoriano commented Mar 28, 2018

spotlesscoder commented Apr 11, 2018

ruflin commented Apr 12, 2018

ruflin left a comment

ruflin Apr 13, 2018

ruflin Apr 13, 2018

spotlesscoder commented Apr 17, 2018

kvalev commented Apr 17, 2018 •

edited

Loading

jsoriano commented Apr 18, 2018

ruflin commented Apr 19, 2018

spotlesscoder commented Apr 25, 2018

jsoriano commented Apr 26, 2018

ruflin commented May 8, 2018

Rabbitmq exchanges metricset #6607

Rabbitmq exchanges metricset #6607

Conversation

kvalev commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

ruflin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Mar 28, 2018

spotlesscoder commented Apr 11, 2018

ruflin commented Apr 12, 2018

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spotlesscoder commented Apr 17, 2018

kvalev commented Apr 17, 2018 • edited Loading

jsoriano commented Apr 18, 2018

ruflin commented Apr 19, 2018

spotlesscoder commented Apr 25, 2018

jsoriano commented Apr 26, 2018

ruflin commented May 8, 2018

kvalev commented Apr 17, 2018 •

edited

Loading