server Metricset for Zookeeper Metricbeat module #10341

sayden · 2019-01-25T14:48:50Z

The current metricset uses the srvr 4 letter word command from Zookeeper to extract server information. Includes all the fields that are returned from the mentioned call. It looks like the following:

Zookeeper version: 3.4.13-2d71af4dbe22557fda74f9a9b4309b15a7487f03, built on 06/29/2018 04:05 GMT
Latency min/avg/max: 1/2/3
Received: 46
Sent: 45
Connections: 1
Outstanding: 0
Zxid: 0x700601132
Mode: standalone
Node count: 4
Proposal sizes last/min/max: -3/-999/-1

You can see how the output is gonna look like in the data.json file.

A better description of the fields is missing. I have just written what I guess what they are but I might be wrong. I'm trying to find a good source of documentation to write a proper description of them.

GA

metricbeat/module/zookeeper/server/_meta/data.json

ruflin

Looks pretty good already. Left a few minor comments, changelog is missing.

metricbeat/docs/modules/zookeeper.asciidoc

metricbeat/module/zookeeper/server/_meta/data.json

metricbeat/module/zookeeper/server/_meta/fields.yml

metricbeat/module/zookeeper/server/data_integration_test.go

metricbeat/module/zookeeper/server/server.go

metricbeat/module/zookeeper/server/server_integration_test.go

ruflin · 2019-01-25T15:56:54Z

metricbeat/module/zookeeper/server/server.go

+	return output, nil
+}
+
+func parseXid(line string) (common.MapStr, error){


Any chance to get some unit tests for the parse functions?

Maybe you have missed it but those functions are tested by the global parseSrvr that uses all of them. In that test all possible values are checked so I considered it tested (75% of coverage is achieved too).

Do you want individual tests too or it is enough with the global test?

Works for me for the moment. In general I think we mainly test the "correct" cases but probably also should check the error cases. Not critical here.

ruflin · 2019-01-25T15:57:43Z

metricbeat/module/zookeeper/server/_meta/data.json

+            "sent": 47,
+            "version": {
+                "date": "06/29/2018 04:05 GMT",
+                "id": "3.4.13-2d71af4dbe22557fda74f9a9b4309b15a7487f03"


The service version should go under service.version as it is an ECS field. Interesting to see that you extraced the date here. @webmat Thoughts?

Version has been moved to service.version and I have left the date there as version_date. Maybe I should simply remove it as it's something that it's not going to change and it provides not much value?

If you easily can skip the date then let's not add it to the event. Not sure how it would be used.

FWIW - I don't feel there's a use-case for version_date. It's not breaking BC if we introduce it in the future, so we might as well drop it (at least for now).

sayden · 2019-01-25T16:40:42Z

Thanks for your comments @ruflin I'll address the necessary changes asap. I have unset the review flag and set in_progress until then and I find a good description for the fields (which I'm going to find in old Zookeeper releases or books... I hope 😄 )

sayden · 2019-01-25T16:41:56Z

@pmoust Forgot to mention about that structured I wrote on the PR. I don't have a strong opinion but if you prefer it that way and @ruflin is happy about it I'm fine too 🙂

webmat

Left one nice to have suggestion on service fields

webmat · 2019-01-25T20:58:26Z

metricbeat/module/zookeeper/server/_meta/data.json

+        "name": "server"
+    },
+    "service": {
+        "address": "localhost:2181",


Would be nice if this was also broken up as service.ip and service.port.

But we would still keep service.address too I would assume. As ip does not match localhost for example.

BTW: This is not something related to this PR as it's done in the module logic. @webmat Could you open a separte issue for this? Nice thing here is we can always add this logic as it's not breaking but addition.

Yes, we'd keep service.address as the source of truth.

sayden · 2019-01-28T11:43:45Z

x-pack/metricbeat/metricbeat.reference.yml

@@ -707,7 +707,7 @@ metricbeat.modules:
 #------------------------------ ZooKeeper Module ------------------------------


It seems that this file in x-pack must be manually updated now. @ruflin do you think that I should open an issue to automate this when running make update from OSS?

manually = running mage update in the x-pack directory or manually you mean by hand and CI will not detect it has missing content?

What behaviour do we have in filebeat here?

+1 on creating an issue to follow up on the discussion.

Sorry, I mean by manually running mage update in x-pack folder. Opening issue #10369

ruflin

LGTM. I suggest we backport this to 6.x if there are no major conflicts?

(cherry picked from commit 3895a23) # Conflicts: # metricbeat/module/zookeeper/fields.go # x-pack/metricbeat/module/mssql/performance/data_integration_test.go

…module (#10375) Cherry pick wasn't completed clean as we needed to adjust `service.version` field, which is documented in `fields.yml` on the module as (in master it is in ECS but ECS is not merged in 6.x)

sayden added enhancement review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Jan 25, 2019

sayden self-assigned this Jan 25, 2019

sayden requested review from a team as code owners January 25, 2019 14:48

sayden mentioned this pull request Jan 25, 2019

Track Zkid counter reported from zookeeper srvr command #8938

Closed

pmoust reviewed Jan 25, 2019

View reviewed changes

metricbeat/module/zookeeper/server/_meta/data.json Outdated Show resolved Hide resolved

ruflin reviewed Jan 25, 2019

View reviewed changes

sayden added in progress Pull request is currently in progress. and removed review labels Jan 25, 2019

webmat reviewed Jan 25, 2019

View reviewed changes

sayden added 9 commits January 28, 2019 09:29

Atomic commit

0e82ebe

Headers missing added.

be788ed

Comments addressed

bb8d3fb

data.json updated

1f05fff

Fields doc needed an update

d386b86

It seems that X-pack must also be updated manually (yet)

d2ed29d

Improve error handling and messages.

bb62dbe

Add changelog entry

2bdf989

Skip data generation test

b9e63be

sayden force-pushed the feature/mb/zookeeper-server-metricset branch from 0687ce5 to b9e63be Compare January 28, 2019 10:20

sayden commented Jan 28, 2019

View reviewed changes

Some improvements on the error handling

b5fab40

sayden force-pushed the feature/mb/zookeeper-server-metricset branch from f9e6c11 to b5fab40 Compare January 28, 2019 12:00

sayden added review and removed in progress Pull request is currently in progress. labels Jan 28, 2019

ruflin approved these changes Jan 28, 2019

View reviewed changes

sayden merged commit 3895a23 into elastic:master Jan 28, 2019

sayden mentioned this pull request Jan 28, 2019

Cherry-pick #10341 to 6.x: server Metricset for Zookeeper Metricbeat module #10375

Merged

10 tasks

sayden added the v6.7.0 label Jan 28, 2019

ruflin mentioned this pull request Jan 30, 2019

Fix server_integration_test.go version_date #10420

Merged

matschaffer mentioned this pull request Feb 1, 2019

Track client connection information reported from zookeeper cons/stat command #10475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server Metricset for Zookeeper Metricbeat module #10341

server Metricset for Zookeeper Metricbeat module #10341

sayden commented Jan 25, 2019 •

edited

Loading

ruflin left a comment

ruflin Jan 25, 2019

sayden Jan 28, 2019

ruflin Jan 28, 2019

ruflin Jan 25, 2019

sayden Jan 28, 2019

ruflin Jan 28, 2019

pmoust Jan 28, 2019

sayden commented Jan 25, 2019

sayden commented Jan 25, 2019

webmat left a comment

webmat Jan 25, 2019

ruflin Jan 28, 2019

webmat Jan 28, 2019

sayden Jan 28, 2019

ruflin Jan 28, 2019

ruflin Jan 28, 2019

sayden Jan 28, 2019

ruflin left a comment

		@@ -707,7 +707,7 @@ metricbeat.modules:
		#------------------------------ ZooKeeper Module ------------------------------

server Metricset for Zookeeper Metricbeat module #10341

server Metricset for Zookeeper Metricbeat module #10341

Conversation

sayden commented Jan 25, 2019 • edited Loading

GA

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

sayden commented Jan 25, 2019

sayden commented Jan 25, 2019

webmat 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

ruflin left a comment

Choose a reason for hiding this comment

sayden commented Jan 25, 2019 •

edited

Loading