Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

redis check should not have a default host and port #200

Closed
xcolour opened this issue Sep 25, 2012 · 8 comments
Closed

redis check should not have a default host and port #200

xcolour opened this issue Sep 25, 2012 · 8 comments
Assignees
Labels

Comments

@xcolour
Copy link
Contributor

xcolour commented Sep 25, 2012

see: https://github.com/DataDog/dd-agent/blob/master/checks/db/redisDb.py#L98

The check is enabled based on whether or not 'redis' can be imported, which is probably wrong to begin with. Either way, it shouldn't assume a port and host if one isn't given.

@ghost ghost assigned clutchski Sep 25, 2012
@alq666 alq666 closed this as completed in dd5a740 Sep 25, 2012
@olidb2
Copy link
Member

olidb2 commented Sep 26, 2012

But we should have a default host/port in the config, right? Maybe just commented out?

On Sep 25, 2012, at 3:01 PM, Matt Singleton wrote:

see: https://github.com/DataDog/dd-agent/blob/master/checks/db/redisDb.py#L98

The check is enabled based on whether or not 'redis' can be imported, which is probably wrong to begin with. Either way, it shouldn't assume a port and host if one isn't given.


Reply to this email directly or view it on GitHub.

@clutchski
Copy link
Contributor

I agree that it should support default host and port.

The real problem is how this check is enabled or disabled. Every check
should have an enabled = true|false flag, so that it's easy to disable
potentially misbehaving checks with chef or puppet.

On Wed, Sep 26, 2012 at 10:13 AM, olidb2 notifications@github.com wrote:

But we should have a default host/port in the config, right? Maybe just
commented out?

On Sep 25, 2012, at 3:01 PM, Matt Singleton wrote:

see:
https://github.com/DataDog/dd-agent/blob/master/checks/db/redisDb.py#L98

The check is enabled based on whether or not 'redis' can be imported,
which is probably wrong to begin with. Either way, it shouldn't assume a
port and host if one isn't given.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-8891143.

@alq666
Copy link
Member

alq666 commented Sep 26, 2012

Already the case.
On Sep 26, 2012 10:13 AM, "olidb2" notifications@github.com wrote:

But we should have a default host/port in the config, right? Maybe just
commented out?

On Sep 25, 2012, at 3:01 PM, Matt Singleton wrote:

see:
https://github.com/DataDog/dd-agent/blob/master/checks/db/redisDb.py#L98

The check is enabled based on whether or not 'redis' can be imported,
which is probably wrong to begin with. Either way, it shouldn't assume a
port and host if one isn't given.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-8891143.

@clutchski
Copy link
Contributor

The enabled/disabled part is standard for every check?

@conorbranagan
Copy link
Contributor

I think he was responding to Oli's thing. As far as having an enabled/disabled part, I'm fine with that. Although, I had always thought whether or not you had a config file would say if something was enabled/disabled.

@clutchski
Copy link
Contributor

Removing the config should remove the check for good, but in the heat of an operational crisis, it's a bit harder to remove a file with chef as opposed to just flipping a config value. I'm not sure I'm right, but just a feeling.

@alq666
Copy link
Member

alq666 commented Sep 26, 2012

My vote goes to sticking with commenting things in and out (as is the
standard for all unix configuration). The issue I can see with a flag is
having, enabled = True and the configuration itself commented out. Or
everything configured but missing enabled = True. Extra complexity and a
deviation from convention.
On Wednesday, September 26, 2012, Matt Perpick wrote:

Removing the config should remove the check for good, but in the heat of
an operational crisis, it's a bit harder to remove a file with chef as
opposed to just flipping a config value. I'm not sure I'm right, but just a
feeling.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-8894839.

*Alexis Lê-Quôc *+1-917-512-6452

Try Datadog for free (http://goo.gl/swO1h) http://goo.gl/swO1h no cc
req'd

And follow @datadoghq https://twitter.com/#!/datadoghq on twitter

@conorbranagan
Copy link
Contributor

Yeah I think that makes sense. So to "disable" something, you'd comment out the instances for that check. If there are no instances, then the check won't be run.

alq666 added a commit that referenced this issue Sep 28, 2012
It triggers spurious error messages.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants