Skip to content
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

Removing Windows Specifc performance counter dependency with cross platfrom EventCounters #1924

Closed
madhub opened this issue Dec 2, 2021 · 6 comments

Comments

@madhub
Copy link

madhub commented Dec 2, 2021

Currently Redis client implementation depends PerformanceCounter class to get CPU time, which brings additional Windows specific nuget package Microsoft.Win32.Registry,System.Configuration.ConfigurationManager each of these inturn brings additional packages creating dependency hell , instead of this it is better to use cross platform EventCounters .
As EventCounters is available in .NET 3.1 on wards , we can use it for .NET Core 3.1 target & older targets still use the PerformanceCounter as is.

Sample implementation of how to get cpu-usage is from Event Counter

Reference Resource

@mgravell
Copy link
Collaborator

mgravell commented Dec 2, 2021

Sounds good to me

@NickCraver
Copy link
Collaborator

I may be missing it here, but I don't think this counter exists in the EventCounters space. There's a subtle but important difference: we're using _Total, which is total CPU by all processes (the goal being to see if the box is pegged and unable to handle the load). The EventCounters space is for this process. For example the process could be at 2% CPU while some beast beside it is eating the other 98% and the box would be pegged...but we wouldn't get a useful metric there.

If there's a way to get global in a fewer dependency way I am also all for it, just not aware of that path at the moment :)

@madhub
Copy link
Author

madhub commented Jan 3, 2022

@NickCraver I saw below code in RavenDB , to calculate CPU usage for Windows/Linux/Mac
Is this something can be used in this project ( if source license allows ) ?
https://github.com/ravendb/ravendb/blob/f773b7da6e8afccad88549ee528fd1165878baa1/src/Raven.Server/Utils/Cpu/CpuUsageCalculator.cs

@NickCraver
Copy link
Collaborator

@madhub it's possible, but that's baking in multiple system calls cross-platform into the library which I think makes things worse for cases like #1442 that we want to solve with net6.0-<platform> potentially. It'd be nice if someone had a library for this specific stat, maybe it does exist...or we should propose the API for .NET 7 as a longer-term solution.

@NickCraver
Copy link
Collaborator

Given this approach isn't viable in the library but the over platform TFMs are on the table, I'm going to tidy up this issue and we'll remain open to adding those TFMs if needed.

@NickCraver
Copy link
Collaborator

Heads up: we revisited this based on issue data and me crawling old issues again to see effectiveness. Not fixed in terms of how to fetch, just removed as not-worth-it tradeoff given even more downsides with vulnerabilities downstream. Version 2.6.80+ will no longer reference System.Diagnostics.PerformanceCounter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants