Closed jchauncey closed 8 years ago
@sstarcher is a potential reviewer of this pull request based on my analysis of git blame
information. Thanks @jchauncey!
LGTM - not that it matters ;)
Yeah blame bot is an equal opportunity blamer :) On Jul 11, 2016 9:33 PM, "Shane Starcher" notifications@github.com wrote:
LGTM - not that it matters ;)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/deis/monitor/pull/119#issuecomment-231913905, or mute the thread https://github.com/notifications/unsubscribe/AAaRGK_ja9Dm9viQspWpuZ02bfSY7bOQks5qUu7ogaJpZM4JJrJM .
LGTM assuming https://github.com/deis/monitor/pull/119#discussion_r70496850 is addressed
I haven't reviewed the code changes, but I have been using this PR for a couple days and I'm pretty happy with it. The one odd thing I would note is that I am sometimes seeing a number of Redis clients reported that is not a whole number. I'm not sure, for instance, what 4.6 clients might mean.
Yeah that value is coming directly from redis so whatever they report we show.
The code (mostly JSON) here looks good to me. I haven't manually tested it and @krancour I know you have, so I'd like you to decide on the final LGTM (I recall you had an issue like https://github.com/deis/monitor/pull/119#issuecomment-232730498 recently, but that's possibly related to https://github.com/deis/charts/pull/304)
Just tested this, but noticed that connected clients is N/A
until the logger starts writing to redis. I haven't seen it report anything but 1
so far either, even when I scale up the number of loggers
interesting.
Just tested, and got clarification on the graph. Also, I noticed that I had an outdated logbomb running. Once I updated my logbomb, the N/A
s mostly went away. I was able to repro them one more time (after a simple restart-and-upgrade of grafana), but since then, doing simple restart-in-place of grafana has not resulted in any more N/A
s.
I'm still LGTM on this patch
This PR adds the ability to monitor our redis instance. Depends on - https://github.com/deis/charts/pull/293
It also updates all of the dashboards to remove some unnecessary fields in the json and clean up their reported versions.
Manual Testing:
make build push upgrade
within the telegraf directorymake build push upgrade
within the grafana directorygrafana.mydomain.com
and login admin/admin