Conor/windows services #1225
Conor/windows services #1225
Conversation
The check will submit a metric tagged by service name that has a value depending on the state of the service. We could extend this check to provide more information later on.
| wmi_service = results[0] | ||
| self._collect_metrics(wmi_service, tags) | ||
|
|
||
| def _collect_metrics(self, wmi_service, custom_tags): |
There was a problem hiding this comment.
That should probably be renamed, we are not sending metrics anymore
|
Looks great besides the nitpick comments i made! I don't think it's the case but do you think it's possible to write an interesting test for that ? |
| """ | ||
| tags = [u'service:%s' % wmi_service.Name] | ||
| tags.extend(custom_tags) | ||
| state_value = self.STATE_TO_VALUE.get(wmi_service.State, -1) |
There was a problem hiding this comment.
What about defaulting to AgentCheck.UNKNOWN instead of -1 which doesn't map to a status?
|
What do you mean by interesting test? From my knowledge on windows services, it seems every is accounted for? Unless @conorbranagan knows of some weird things that might happen? |
|
The goal of adding tests is to prevent things from getting broken in the future. But testing that specific feature is not straight forward as we can't easily mock windows services. |
| """ Given an instance of a wmi_object from Win32_Service, write any | ||
| performance counters to be gathered and flushed by the collector. | ||
| """ | ||
| tags = [u'service:%s' % wmi_service.Name] |
There was a problem hiding this comment.
Tags should probably include the host (and if it's "." (current host) then replace by the agent hostname)
| performance counters to be gathered and flushed by the collector. | ||
| """ | ||
| if host == ".": | ||
| host_name = self.hostname |
There was a problem hiding this comment.
I think you forgot the:
else:
host_name = hostThere was a problem hiding this comment.
yup i did thanks
| self.wmi_conns = {} | ||
|
|
||
| def _get_wmi_conn(self, host, user, password): | ||
| key = "%s:%s:%s" % (host, user, password) |
There was a problem hiding this comment.
It's probably better to not hash the password in the key here, you cannot have 1 user for several password anyway I guess, but it's maybe nitpicky.
There was a problem hiding this comment.
@whatarthurcodes could you sort this out, so we can merge it for 5.2!
There was a problem hiding this comment.
@LeoCavaille did my change reflect your concern?
@remh