-
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
Prevent caching of PDH counter instances by default #2654
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2654 +/- ##
==========================================
- Coverage 84.72% 76.42% -8.31%
==========================================
Files 662 45 -617
Lines 37611 3313 -34298
Branches 4507 387 -4120
==========================================
- Hits 31867 2532 -29335
+ Misses 4428 684 -3744
+ Partials 1316 97 -1219 |
About the benchmarks:
|
@@ -182,3 +138,51 @@ def check(self, instance): | |||
except Exception as e: | |||
# don't give up on all of the metrics because one failed | |||
self.log.error("Failed to get data for %s %s: %s" % (inst_name, dd_name, str(e))) | |||
|
|||
def _make_counters(self, key, counter_data): |
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.
Nice factorization here!
@@ -167,8 +115,16 @@ def __init__(self, name, init_config, agentConfig, instances, counter_list): | |||
def check(self, instance): | |||
self.log.debug("PDHBaseCheck: check()") | |||
key = hash_mutable(instance) | |||
refresh_counters = is_affirmative(instance.get('refresh_counters', True)) | |||
|
|||
if refresh_counters: |
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.
Why are we having this in the check method now?
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.
behind flag in case someone encounters perf issues
|
||
entry = [inst_name, dd_name, m, obj] | ||
self.log.debug('{}: {}'.format(message, entry)) | ||
self._metrics[key].append(entry) |
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.
Because of this method usage at https://github.com/DataDog/integrations-core/pull/2654/files#diff-14769541de478cc114862b41f3cdcfe2R122 , isn't it a risk that the key won't exist?
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.
Key will always exist
integrations-core/datadog_checks_base/datadog_checks/base/checks/win/winpdh_base.py
Line 55 in 8567b6e
self._metrics[key] = [] |
Motivation
Some checks' counters may change frequently e.g. active VMs
Benchmarks
https://ci.appveyor.com/project/Datadog/integrations-core/builds/20973815#L868
It looks like the change has near linear scaling: on mocks and real setups it's about up to 3 times slower compared to caching everything forever. The timing for each case seems to simply depend on the number of counter instances.
hyperv w/ 2 VMs running:
active_directory w/ our mocks
aspdotnet w/ our mocks
dotnetclr w/ our mocks
exchange_server w/ our mocks
iis on appveyor
pdh_check w/ our mocks