-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
implement iis as pdh check #927
Conversation
b9d2a52
to
ea63ad2
Compare
Update example config. Allow for querying all sites. Add capability for adding additional metrics not already collected Switch to dd-agent branch which has required libs; remove this change when that branch is merged Fix adding too many service checks dd-agent now has correct libs Add proper handing of 'TOTAL'
ea63ad2
to
f4fe925
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have one small comment on this. I think this looks great, otherwise. (We already talked about submitting for all sites, and I agree it's a good idea.)
appveyor.yml
Outdated
@@ -8,7 +8,7 @@ environment: | |||
NOSE_FILTER: not unix and not fixme and not winfixme | |||
PYWIN_PATH: C:\projects\integrations-core\.cache\pywin32-py2.7.exe | |||
SKIP_LINT: true | |||
DD_AGENT_BRANCH: master | |||
DD_AGENT_BRANCH: db/pdh_base_updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change this back?
iis/check.py
Outdated
# don't give up on all of the metrics because one failed | ||
self.log.error("IIS Failed to get data for %s %s: %s" % (inst_name, dd_name, str(e))) | ||
pass | ||
self.log.debug("expected sites is now %s" % str(expected_sites)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to make it a bit more informative? Something like "failed to grab metrics for...", so that we don't have to know the internal code of the check to interpret it.
Additional questions:
Do we want to log this on info?
Do we want to log it on warning, so it shows up on the info page?
05c6ecc
to
ed363bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekwbrown I think this is good to go. I reran the tests, we'll wait for them to finish. Unless you think there's some reason to sit on this, let's merge it.
Redo iis check as PDH rather than WMI.
Update example config.
Allow for querying all sites.
Add capability for adding additional metrics not already collected
Requires PDH support in 5.22/6.0+