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

feat(grafana,telegraf): Pull metric data from nsq via telegraf #110

Merged
merged 1 commit into from
Jun 17, 2016

Conversation

jchauncey
Copy link
Member

This PR updates several parts of the monitoring stack to support our move to NSQ. It is in progress for now since we are waiting for telegraf to merge my PR - influxdata/telegraf#1369

@jchauncey jchauncey added this to the v2.1 milestone Jun 14, 2016
@jchauncey jchauncey self-assigned this Jun 14, 2016
@deis-bot
Copy link

@krancour and @sstarcher are potential reviewers of this pull request based on my analysis of git blame information. Thanks @jchauncey!

@arschles
Copy link
Member

@jchauncey looks like you're downloading the telegraf deb and then installing telegraf in the Dockerfile at telegraf/rootfs/Dockerfile, but you also have checked in the telegraf binary. From the description of this PR, sounds like you're gonna download the telegraf binary going forward (either from the official releases or our custom build from our own object storage), so can we get rid of the checked-in binary?

Also updates grafana graphs with correct series names.
@@ -35,6 +35,12 @@ spec:
value: "true"
- name: "ENABLE_PROMETHEUS"
value: "true"
- name: "NSQ_CONSUMER_SERVER"
Copy link
Member

Choose a reason for hiding this comment

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

nit: worth making the prefixes for all these env vars NSQD_?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it so it matches the plugin name. Which is nsq_consumer

@arschles
Copy link
Member

@jchauncey the json diffs are a bit hard to parse, but the changed files themselves look ok. my nitpick is not blocking for this PR. Also, thanks for removing the telegraf binary.

Code LGTM

@arschles arschles added the LGTM1 label Jun 15, 2016
@jchauncey
Copy link
Member Author

Yeah the json diffs suck. easy way to test it is to build and push the image and check the graphs out

@jchauncey jchauncey merged commit 99028fa into deis:master Jun 17, 2016
@jchauncey jchauncey deleted the use-nsq branch June 17, 2016 16:09
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.

4 participants