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

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

Merged
merged 3 commits into from
Jan 31, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Jan 28, 2019

Cherry-pick of PR #10341 to 6.x branch. Original message:

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

  • Supported versions are documented
  • Supported operating systems are documented (if applicable)
  • Integration tests
  • System tests
  • Automated checks that all fields are documented
  • Documentation
  • Example data.json exists and an automated way to generate it exists (go test -data)
  • Fields follow naming conventions: https://www.elastic.co/guide/en/beats/devguide/master/event-conventions.html
  • Dashboards exists (if applicable)
  • Kibana Home Tutorial (if applicable)

(cherry picked from commit 3895a23)

# Conflicts:
#	metricbeat/module/zookeeper/fields.go
#	x-pack/metricbeat/module/mssql/performance/data_integration_test.go
@ruflin
Copy link
Member

ruflin commented Jan 28, 2019

You probably have to adjust test_zookeeper a bit as it also checks for service field which was not around in 6.x yet. Also regenerate the data.json files as the content might look a bit different.

@@ -71,7 +71,6 @@ def test_output(self):
self.assertEqual(len(output), 1)
evt = output[0]

self.assertItemsEqual(self.de_dot(ZK_FIELDS), evt.keys())
Copy link
Member

Choose a reason for hiding this comment

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

I see you remove this line. What were the errors you got? Perhaps there is an eays way to fix it instead of removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error in Travis was pointing directly to this line which was pointing to a service field (service.name to be precise) that wasn't found. As far as I saw, this field comes from https://github.com/elastic/beats/blob/master/metricbeat/tests/system/metricbeat.py#L9 with this value

COMMON_FIELDS = ["@timestamp", "beat", "metricset.name", "metricset.host",
                 "metricset.module", "metricset.rtt", "host.name", "event"]

A way to fix this is to use a customized COMMON_FIELDS which doesn't seem very ideal. But in any case I'm not sure if it should contain a service.name or not

Copy link
Member

Choose a reason for hiding this comment

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

I think what should work here is:

self.assertItemsEqual(self.de_dot(ZK_FIELDS + ["sevice"]), evt.keys())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's customized way but I'm a bit afraid that it could bite us in the future (by getting outdated somehow).

Anyways, test passes with that error but now I get a Key 'service.version' found in event is not documented which is true but I don't understand why CI hasn't detected it in master 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm really confused because service.version is a ECS field, right? And it's not backported to 6.x AFAIK. But what should I do, update it in fields.yml of... where? 😄

Copy link
Member

Choose a reason for hiding this comment

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

ECS is not in 6.x. The best way here as it's not used anywhere else is to add service.version to the zookeeper module (not fileset).

Copy link
Member

Choose a reason for hiding this comment

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

For the outdated part: I would not worry too much as I don't expect many more changes will end up in 6.7 and if it gets outdated, CI will tell us.

@sayden
Copy link
Contributor Author

sayden commented Jan 30, 2019

@ruflin Friendly reminder here 🙂

@sayden sayden merged commit 7f2ab44 into elastic:6.x Jan 31, 2019
@sayden sayden self-assigned this Feb 8, 2019
@sayden sayden deleted the backport_10341_6.x branch October 29, 2021 08:56
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