Resource monitor #48501
Resource monitor #48501
Conversation
t-davidbrown-msft
commented
Mar 1, 2025
t-davidbrown-msft
commented
- add cpu monitor returns a float between 0 and 1
- add memory monitor currently return a double between 0 and 1 (this is the current amount of memory a process is using divided by the total amount of memory on the host)
- add tests for cpu monitor and memory monitor. Tests check for 0, less than 1, and null
- create method for monitoring CPU usage that works for all frameworks using System.Diagnostics.Process - create one test for ensuring that the monitor works
Merging the latest changes into my feature branch
- add CpuMonitor interface - add CpuMonitor tests - change CurrentProcessorTime to be the current time on the processor minute the previous
- removed unnecessary refactor - add SimulateCpuLoad to test suite to simulate loading the CPU - add CoreCount Property to replace function calls to Environment.ProcessorCount
- readd default constructor - add tests for monitoring interval that is below 100 milliseconds
- create processor directive to create memory monitor for netstandard2.0 or greater, but this is not a great implementation - create method for MonitorMemoryUsage is otherwise good - create tests to check whether it is below 1, above 0, and not null
- add cpu monitor returns a float between 0 and 1 - add memory monitor currently return a double between 0 and 1 (this is the current amount of memory a process is using divided by the total amount of memory on the host) - add tests for cpu monitor and memory monitor. Tests check for 0, less than 1, and null
|
@microsoft-github-policy-service agree company="Microsoft"
|
|
API change check API changes are not detected in this pull request. |
merge main into feature branch
| /// <summary> | ||
| /// Interface for monitoring CPU usage. | ||
| /// </summary> | ||
| public interface IResourceMonitor |
There was a problem hiding this comment.
Any particular reason you needed an interface? While a good .NET practice, we don't typically use them in our SDKs because changing them can result in breaking changes. In this case, this thing will be internal so breaking changes won't be too big of a deal, but we still typically just stay away from them.
|
|
||
| /// <summary> | ||
| /// Initalizes the CPU monitor. | ||
| /// | ||
| /// This should be called early on in the job transfers. | ||
| /// </summary> | ||
| public CpuMonitor() | ||
| /// | ||
| public ResourceMonitor() { } |
There was a problem hiding this comment.
We should either remove the default constructor or set a default monitoring interval. Right now, as-is the default constructor will result in an instance with a null monitoring interval which we probably want to avoid.
| //}); | ||
|
|
||
| Task.Run(() => MonitorCpuUsage(_cancellationTokenSource.Token)); | ||
| Task.Run(() => MonitorMemoryUsage(_cancellationTokenSource.Token)); |
There was a problem hiding this comment.
Hmm, I think it would be better to have both running in the same thread since there is no reason to start and manage a thread for each. In fact, maybe one function that monitors both at the same time would make sense?
| public void StopMonitoring() | ||
| { | ||
| if (!IsRunning) | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
nit: Maybe we don't need to throw here? If it is already stopped, we are at the state we wanted so we could just return.
sdk/storage/Azure.Storage.DataMovement/src/ConcurrencyTuner/ResourceMonitor.cs
Show resolved
Hide resolved
| await Task.Delay(_monitoringInterval, cancellationToken).ConfigureAwait(false); | ||
| // This is the memory that is shown in the task manager | ||
| //long currentMemoryUsage = CurrentProcess.WorkingSet64; | ||
| long currentMemoryUsage = CurrentProcess.PrivateMemorySize64; |
There was a problem hiding this comment.
Hmm not sure which is better here, working set or private. I never really understood the difference...
| // Use PInvoke here | ||
|
|
||
| #endif | ||
| MemoryUsage = (double)currentMemoryUsage / (double)totalPhysicalMemory; |
There was a problem hiding this comment.
So, I think we should start by keeping this simple and just saving current memory usage of our process. The way to get total memory usage here uses P/Invoke and seems Windows specific. We want to avoid P/Invoke at all costs, so we'd need to rethink this a bit. For now, I vote we just remove all this total memory and P/Invoke stuff.
| //for (var i = 0; i < 1000; i++) | ||
| //{ | ||
| // Assert.That(cpuMonitor.CpuUsage, Is.LessThan(1)); | ||
| //} |
There was a problem hiding this comment.
Remove commented stuff or bring it back if needed.
| public bool IsRunning { get; private set; } | ||
| private double PreviousProcessorTime { get; set; } = 0; | ||
| private double CurrentProcessorTime { get; set; } | ||
| private int CoreCount { get; } = Environment.ProcessorCount; |
There was a problem hiding this comment.
All private members should start with an _ and be camel case. So _previousProcessorTime, _currentProcessorTime , and _coreCount here.
We should also group all public members together followed by all private members.
| if (IsRunning) | ||
| return; | ||
|
|
||
| IsRunning = true; |
There was a problem hiding this comment.
Should this be behind a lock / semaphoreslim ?
There was a problem hiding this comment.
Setting a boolean is atomic: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/variables#96-atomicity-of-variable-references
Also I'm not sure we'll be using this from multiple threads since there will only be one per transfer manager and I think the transfer manager will be the one starting/stopping it.
| // Process.TotalProcessorTime.TotlMilliseconds returns the processing time across all processors | ||
| // Environment.ProcessorCount returns then total number of cores (which includes physical cores plus hyper | ||
| // threaded cores | ||
| return (float)(CurrentProcessorTime - PreviousProcessorTime) / (float)(_monitoringInterval.TotalMilliseconds * CoreCount); |
There was a problem hiding this comment.
Not sure if this is necessary but should we look out for accidentally dividing by 0?
There was a problem hiding this comment.
I just set a default _monitoringInterval to 1 second. But I think you're right; let me add something to ensure that we can't do like Chuck Norris 😏: Divide by 0. 🤣
Pulled main branch from the original repo. Merging updates into my feature branch
- remove interface - change private properties to fields - group private fields and properties together - remove empty constructor and replace with constructor with default monitorInterval - combine cpu and memory into one task; run on one thread - get CPU and memory; then sleep - return Bytes for memory usage - remove totalMemory
