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

Add etcd module (#4970) #5176

Merged
merged 43 commits into from
Oct 17, 2017
Merged

Add etcd module (#4970) #5176

merged 43 commits into from
Oct 17, 2017

Conversation

berfinsari
Copy link
Contributor

The Etcd module uses Etcd v2 API to collect metrics.

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@berfinsari
Copy link
Contributor Author

berfinsari commented Sep 14, 2017

I run make fmt command to cleanup the styles but I still get the same error. Is there anything that I can do for this unsuccessful check?

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.

I don't think the failure is related to fmt as it looks as following:

command [go test -tags integration -cover -coverprofile /tmp/gotestcover-027846761 github.com/elastic/beats/metricbeat/module/etcd/store]: exit status 1
docker-compose.yml is locked, waiting
docker-compose.yml is locked, waiting
--- FAIL: TestData (62.45s)
	compose.go:64: Timeout waiting for services to be healthy
docker-compose.yml is locked, waiting
--- FAIL: TestFetch (61.52s)
	compose.go:64: Timeout waiting for services to be healthy
FAIL
coverage: 80.0% of statements

@exekias might know here more.

@berfinsari It seems you checked in some files that should be under .gitignore. Can you remove them again?

@@ -0,0 +1,452 @@
########################## Metricbeat Configuration ###########################
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file was added by accident?

@@ -0,0 +1,6769 @@
- key: common
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be checked in.

@exekias
Copy link
Contributor

exekias commented Sep 18, 2017

Hi @berfinsari, thank you for contributing! As @ruflin pointed out, there is an issue with the testing container, you will need to add a valid HEALTHCHECK, for some reason the current one is failing, so we get a timeout waiting for the service to be ready to run tests

@@ -0,0 +1,6 @@
- module: etcd
metricsets: ["leader", "self", "store"]
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you drop the enabled field? We leave it to the default now

@tsg
Copy link
Contributor

tsg commented Sep 25, 2017

jenkins, test it

@exekias exekias self-assigned this Sep 27, 2017
@exekias
Copy link
Contributor

exekias commented Sep 27, 2017

I'll have a look to the healthcheck

@exekias
Copy link
Contributor

exekias commented Oct 9, 2017

I've fixed conflicts and health check (curl was not available on that image so I switched to wget), let see if tests pass this time

@exekias
Copy link
Contributor

exekias commented Oct 9, 2017

jenkins test it please

@exekias
Copy link
Contributor

exekias commented Oct 9, 2017

jenkins test it, please

@exekias
Copy link
Contributor

exekias commented Oct 11, 2017

I fixed the container health issue, but tests seem to be broken, @berfinsari do you want to have a look? I can take it tomorrow if not

@berfinsari
Copy link
Contributor Author

@exekias Thanks for the healthcheck, i check the integration tests

@ruflin
Copy link
Contributor

ruflin commented Oct 16, 2017

jenkins, test it

@@ -0,0 +1,2 @@
ETCD_HOST=localhost
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to put ETCD_HOST=etcd here, it will probably fix tests

@exekias
Copy link
Contributor

exekias commented Oct 17, 2017

jenkins test it please

@exekias
Copy link
Contributor

exekias commented Oct 17, 2017

Finally tests are green 💚, thank you for your contribution!!

@exekias exekias merged commit 3add505 into elastic:master Oct 17, 2017
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.

5 participants