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 prometheus_client service output module, update prometheus client #318

Closed

Conversation

oldmantaiter
Copy link
Contributor

1 - Add prometheus_client service output module, update prometheus client

  • Adds a client implementation using the prometheus go_client library
    that exposes metrics.
  • Adds a new type of output "ServiceOutput" which follows inline with
    the "ServicePlugin", adding a Stop and Start method for the service

This change also requires the newer prometheus/client_golang code, so
the prometheus plugin needed to be changed.

Added the following to Godep:
- bitbucket.org/ww/goautoneg (in github.com/common/expfmt/encode.go)
- prometheus/common/expfmt (in plugins/prometheus.go)
- github.com/prometheus/common/model (in plugins/prometheus.go)
- github.com/prometheus/procfs (in github.com/client_golang/prometheus)
- github.com/beorn7/perks/quantile (in github.com/client_golang/prometheus)

@oldmantaiter
Copy link
Contributor Author

Writing unit tests now. Just getting the initial work into a PR.

@oldmantaiter oldmantaiter force-pushed the prometheus-client-output branch from f2f45c7 to 61888f7 Compare October 24, 2015 19:14
@otoolep
Copy link

otoolep commented Oct 24, 2015

Did you really mean to add all those files?

@oldmantaiter
Copy link
Contributor Author

@otoolep Yes, I added some dependencies and based on how previous deps were added I included them (eg. https://github.com/influxdb/telegraf/tree/master/Godeps/_workspace/src/github.com/eapache/queue). Is this an incorrect assumption?

@sparrc
Copy link
Contributor

sparrc commented Oct 25, 2015

@oldmantaiter Did you edit the files manually or use godep? If you used godep it's OK

@oldmantaiter
Copy link
Contributor Author

@sparrc Yes, it was godep. I'll double check with a fresh environment that I didn't add anything that is not supposed to be there.

@sparrc
Copy link
Contributor

sparrc commented Oct 25, 2015

@oldmantaiter it's normal to add this many files when using godep, don't worry about it :-)

@oldmantaiter oldmantaiter force-pushed the prometheus-client-output branch 7 times, most recently from 373e458 to cfdf414 Compare October 26, 2015 01:31
@oldmantaiter
Copy link
Contributor Author

Sorry for all the commit amends if they resulted in emails. It seems that CircleCI is having trouble with one of the tests that is passing locally, I'll keep looking into it.

@sparrc
Copy link
Contributor

sparrc commented Oct 26, 2015

@oldmantaiter If you're connecting to the docker container using testutil.GetLocalHost(), you should make that a "long" test.

see https://github.com/influxdb/telegraf/blob/master/plugins/mysql/mysql_test.go#L14-L16

@sparrc
Copy link
Contributor

sparrc commented Oct 26, 2015

Also don't worry about the amend commits, I don't get emails and I prefer PRs with amended commits :-)

@oldmantaiter
Copy link
Contributor Author

@sparrc I have been using that function to get the hostname, I can make them short tests, but since they don't rely on external services in the testing, I was hoping to leave them in.

@oldmantaiter oldmantaiter force-pushed the prometheus-client-output branch 3 times, most recently from a278196 to fa7e19f Compare October 26, 2015 12:51
- Adds a client implementation using the prometheus go_client library
  that exposes metrics.

- Adds a new type of output "ServiceOutput" which follows inline with
  the "ServicePlugin", adding a Stop and Start method for the service

This change also requires the newer prometheus/client_golang code, so
the prometheus plugin needed to be changed.

Added the following to Godep:
    - bitbucket.org/ww/goautoneg (in github.com/common/expfmt/encode.go)
    - prometheus/common/expfmt (in plugins/prometheus.go)
    - github.com/prometheus/common/model (in plugins/prometheus.go)
    - github.com/prometheus/procfs (in github.com/client_golang/prometheus)
    - github.com/beorn7/perks/quantile (in github.com/client_golang/prometheus)

X-Github-Meta: closes influxdata#306
@oldmantaiter oldmantaiter force-pushed the prometheus-client-output branch from fa7e19f to 8579456 Compare October 26, 2015 13:05
@sparrc
Copy link
Contributor

sparrc commented Oct 26, 2015

@oldmantaiter in that case do not make it a "short" test. But are you sure it is not relying on an external service? The error in CircleCI is that it's not able to connect to a service:

No error is expected but got error making HTTP request to http://localhost:9126/metrics: 
Get http://localhost:9126/metrics: dial tcp 127.0.0.1:9126: getsockopt: connection refused

@oldmantaiter
Copy link
Contributor Author

@sparrc The prometheus_client service output starts an HTTP server within the same scope as the telegraf agent (defaulting to localhost on port 9126). The tests use the prometheus plugin to query the started prometheus_client, locally these tests pass, but not on CircleCI

@sparrc
Copy link
Contributor

sparrc commented Oct 26, 2015

But I don't see Start() getting called in the unit tests, maybe I'm missing something?

@sparrc
Copy link
Contributor

sparrc commented Oct 26, 2015

I see it get called once, but I think you need to start it within each test

@oldmantaiter
Copy link
Contributor Author

@sparrc I'll take another look (been sidetracked with other stuff today), I thought if something was global it would remain untouched through the tests, but that may not be the case as you pointed out.

@sparrc
Copy link
Contributor

sparrc commented Oct 28, 2015

@oldmantaiter I see what you mean, the http.ListenAndServe() can't really be stopped within the same scope. I think that Circle also has a problem with ":" addresses and I've seen that get fixed by just changing it to "localhost:". I have some ideas, I'll play around with this a bit and I think I can get it working

sparrc added a commit that referenced this pull request Oct 28, 2015
sparrc added a commit that referenced this pull request Oct 28, 2015
@otoolep
Copy link

otoolep commented Oct 28, 2015

We tend to set up our own lower-level TCP listeners, and pass those to HTTP servers, when needing full control over shutdown for testing.

@oldmantaiter
Copy link
Contributor Author

@otoolep Understood, based on @sparrc's latest commit it was an issue of me relying on a test to setup the server to be called instead of throwing it in init.

I've been a little busy so haven't had time to pick this up again, thanks for the assistance in getting the tests going. I'll close this PR and update in #329 RE: tests and any other items I may have overlooked.

allenj pushed a commit to allenj/telegraf that referenced this pull request Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants