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

Aerospike module #4560

Merged
merged 2 commits into from
Jun 28, 2017
Merged

Aerospike module #4560

merged 2 commits into from
Jun 28, 2017

Conversation

alexshadow007
Copy link
Contributor

It uses http://www.aerospike.com/docs/reference/info to collect metrics.

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@elasticmachine
Copy link
Collaborator

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

@exekias
Copy link
Contributor

exekias commented Jun 27, 2017

Jenkins test it

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Awesome contribution! I've to say I'm impressed by the quality of this PR, congrats! 🎉

I left some minor comments, also, could you add an integration test (like https://github.com/elastic/beats/blob/master/metricbeat/tests/system/test_apache.py), it should make it easier to ensure all fields are documented :)


config := struct{}{}

logp.Warn("EXPERIMENTAL: The aerospike namespace metricset is experimental")
Copy link
Contributor

Choose a reason for hiding this comment

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

We now offer logp.Experimental, could you change this to use it?

@@ -0,0 +1,8 @@
== aerospike Module

This is the aerospike Module. It uses http://www.aerospike.com/docs/reference/info [Info command ] to collect metrics.
Copy link
Contributor

Choose a reason for hiding this comment

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

spurious space after Info command?

@alexshadow007
Copy link
Contributor Author

@exekias Done

@exekias
Copy link
Contributor

exekias commented Jun 28, 2017

Thank you for this awesome contribution @alexshadow007, this is ready to go from my POV, could you add an entry to CHANGELOG.asciidoc?

@alexshadow007
Copy link
Contributor Author

@exekias Done

@exekias exekias merged commit 105105d into elastic:master Jun 28, 2017
@tsg
Copy link
Contributor

tsg commented Jun 28, 2017

@alexshadow007 Do you happen to have a dashboard created for this module? It would be nice to include one as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants