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

[Metricbeat] Add Zookeeper connections metricset #11070

Merged

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Mar 4, 2019

Zookeeper connections Metricset parses the incoming information of cons 4 letter command from Zookeeper and emit a single event for each connection that appears in the output.

Relates to this #10475

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 4, 2019
@sayden sayden self-assigned this Mar 4, 2019
@sayden sayden added the in progress Pull request is currently in progress. label Mar 13, 2019
@sayden sayden marked this pull request as ready for review March 13, 2019 10:21
@sayden sayden requested review from a team as code owners March 13, 2019 10:21
@sayden sayden added review and removed in progress Pull request is currently in progress. labels Mar 13, 2019
@sayden
Copy link
Contributor Author

sayden commented Mar 13, 2019

jenkins, test this please

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks for adding this new metricset!

metricbeat/module/zookeeper/connections/connections.go Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/data.go Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/_meta/fields.yml Outdated Show resolved Hide resolved
"period": "5s"
}])
proc = self.start_beat()
self.wait_until(lambda: self.output_lines() > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is there going to be always connections available to report?
Should we keep a connection with zookeeper open here to avoid flakiness?

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 don't follow you. If you want to change the behaviour of the module or the system tests it should go in a different PR, this is just to add a Metricset with the current behaviour and Cloud is waiting for it 😉

Copy link
Member

Choose a reason for hiding this comment

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

If there is no connection to the zookeeper service, no event is generated and then this will fail. I was wondering if we should open a connection in the test to avoid flakiness because of that.
Nothing to change in the module, just maybe in the test.
If there is always going to be a connection here because of other reasons then you can ignore my comment 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Also this, if it is not failing now I think it won't fail, to be revisited if there are related flaky tests.

@sayden sayden added in progress Pull request is currently in progress. and removed review labels Mar 19, 2019
@sayden sayden force-pushed the feature/mb/zookeeper-connections-metricset branch from 52ce129 to 17d2111 Compare March 19, 2019 20:59
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Ups, forgot to hit submit after reviewing :-(

metricbeat/docs/modules/zookeeper/connections.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/_meta/fields.yml Outdated Show resolved Hide resolved
connections
fields:
- name: ip
type: keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

type: ip ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now they are part of ECS as requested by Jaime

metricbeat/module/zookeeper/connections/connections.go Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/connections.go Outdated Show resolved Hide resolved
metricbeat/module/zookeeper/connections/connections.go Outdated Show resolved Hide resolved
} else {
m.checkRegexAndSetInt(metricsetFields, v, k, &oneParsingIsCorrect)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could schema be used here for these mappings?

Copy link
Member

Choose a reason for hiding this comment

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

Lets leave this change for a follow up as this is blocking other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks Jaime!


if oneParsingIsCorrect {
result = append(result, mb.Event{MetricSetFields: metricsetFields, RootFields: rootFields})
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we log at least at the debug level if nothing parsed correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

@jsoriano jsoriano dismissed their stale review April 12, 2019 17:24

Comments were addressed

} else {
m.checkRegexAndSetInt(metricsetFields, v, k, &oneParsingIsCorrect)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Lets leave this change for a follow up as this is blocking other things.

"period": "5s"
}])
proc = self.start_beat()
self.wait_until(lambda: self.output_lines() > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Also this, if it is not failing now I think it won't fail, to be revisited if there are related flaky tests.

@sayden
Copy link
Contributor Author

sayden commented Apr 15, 2019

Error seems unrelated. Merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants