-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix part 2 of #1291 - add PDH_FMT_NOCAP100 to format options in win_perf_counters #2483
Fix part 2 of #1291 - add PDH_FMT_NOCAP100 to format options in win_perf_counters #2483
Conversation
added PDH_FMT_NOCAP100 format option
cc @PierreF @TheFlyingCorpse could you review? |
I'm not sure to fully understand PDH_FMT_NOCAP100. From the documentation
But I do already have value larger that 100 (e.g. filesystem space used, it does not loop at 100 bytes !) I don't have multi-core under the hand right now, the biggest issue I could see is how would be reported global CPU usage. Before this change (like unix one) 100% idle or 100% used means all core full utilization. Does this change break that and make global CPU between 0-400 % (core 4-core) ? If it's not changed, very fine and +1. Note: I understand that this bug fix issue for CPU usage of one process. I do talk about CPU usage of whole system. |
@PierreF I have multi-core server and total cpu usage look fine after this fix. I'm also worried that there can be some counters that will be broken by this patch. We use this version on production for 2 days and haven't seen any problem yet, but we use small amount of counters namely: total CPU, per core CPU, Available Memory, network IO (bytes per second), Private memory per process. I think there is some counters in windows that capped to 100 and some that are not, but i don't know |
Perfect. LGTM. |
@VVvKamper the first graph there is your total CPU usage, isn't it? you need to show an example of pre-change total CPU and post-change total CPU that is greater than 100%. ie, your total CPU usage is below 100 so it is not getting capped regardless. |
total CPU usage (if that what I called system CPU/global CPU) could never be above 100% (and that what we want). What I wanted to avoid, is that with pre-change 2 core out of 4-core used = 50% and with post-change 2 core out of 4-core used = 200%. |
@sparrc Yes it's |
got it 👍 |
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.
LGTM
added PDH_FMT_NOCAP100 format option closes influxdata#2483
added PDH_FMT_NOCAP100 format option closes influxdata#2483
added PDH_FMT_NOCAP100 format option closes influxdata#2483
added PDH_FMT_NOCAP100 format option closes #2483
Required for all PRs:
This addresses part 2 of #1291.
As i understand
pdh_GetFormattedCounterValue
function by default capping some values to 100.In this PR i override this behaviour by adding PDH_FMT_NOCAP100 to format options.