Rename metrics for Resource Monitoring #5309
Conversation
There was a problem hiding this comment.
This change looks like it potentially may be a breaking change. Metrics are kind part of the public surface - i.e., documentations, dashboards and other monitoring systems depend on existing/published metrics.
Unfortunately the original issue doesn't provide great level of detailed information on the underlying issue(s), scenarios where and how it manifests, how it affects customers, etc.
Could you please provide more information and assessment of the change in the following form:
- Customer Imparct - how would this change affect existing customers?
- Is this a regression? If yes, when was it introduced?
- How risky is this change?
Thank you.
I'm blocking the merge to understand the risks and impacts and stop an accidental merge.
dotnet-policy-service
bot
added
the
waiting-author-feedback 📭
|
@RussKie, here you are:
|
dotnet-policy-service
bot
removed
the
waiting-author-feedback 📭
It does not affect customers as such, they can live with it forever and be happy :) However, it is just semantically incorrect to have |
|
It's definitely a breaking change. It needs to be documented as a breaking change and the mitigations required to work around it be in release notes. You could require people to update whatever consumes the metrics to make a fix. If there are people who can't do that then you could add an app context switch that someone can set in their app to keep old behavior. |
|
@evgenyfedorov2 can you please go into a bit more detail as to what is the motivation behind this change? Is it mostly just that the naming is inaccurate so we want to fix it? We should get the full motivation so we can evaluate if this is worth the breaking change. |
| /// Gets the CPU consumption share of the running process in range <c>[0, 1]</c>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The type of an instrument is <see cref="System.Diagnostics.Metrics.ObservableGauge{T}"/>. | ||
| /// </remarks> | ||
| public const string ProcessCpuUtilization = "process.cpu.utilization"; | ||
|
|
||
| /// <summary> | ||
| /// Gets the memory consumption share of the running process in range <c>[0, 1]</c>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The type of an instrument is <see cref="System.Diagnostics.Metrics.ObservableGauge{T}"/>. | ||
| /// </remarks> | ||
| public const string ProcessMemoryUtilization = "dotnet.process.memory.virtual.utilization"; |
There was a problem hiding this comment.
Should we decorate these as [Obsolete] to indicate they shouldn't be used?
This reverts commit 5d54584.
Fixes #5113 Previous art: #5309 Add new metrics with correct names. Old metrics will continue to be enabled by default. ### Existing metric setup **Windows Snapshot provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` **Windows Container Snapshot provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` **Linix Utilization Provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` ### New metric setup **Windows Snapshot provider class** `process.cpu.utilization` - no changes `dotnet.process.memory.virtual.utilization` - no changes **Windows Container Snapshot provider class** `process.cpu.utilization` - no changes `dotnet.process.memory.virtual.utilization` - calculates memory for the dotnet process only (instead of all processes) `container.cpu.limit.utilization` - new metric, same value as `process.cpu.utilization` `container.memory.limit.utilization` - new metric, calculates memory for all processes in the container **Linux Utilization Provider class** `process.cpu.utilization` - fixed incorrect scale calculation, instead of `host CPUs / CPU limit / CPU request`, it is now `host CPUs / CPU request` `dotnet.process.memory.virtual.utilization` - no changes `container.cpu.limit.utilization` - new metric, value is relative to CPU resource limit (aka maximum CPU units) `container.memory.limit.utilization` - new metric, calculates memory for all processes in the container `container.cpu.request.utilization` - new metric, same value as `process.cpu.utilization`
…ng (dotnet#5341) Add metrics with correct names for Resource Monitoring (dotnet#5341) Fixes dotnet#5113 Previous art: dotnet#5309 Add new metrics with correct names. Old metrics will continue to be enabled by default. ### Existing metric setup **Windows Snapshot provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` **Windows Container Snapshot provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` **Linix Utilization Provider class** `process.cpu.utilization` `dotnet.process.memory.virtual.utilization` ### New metric setup **Windows Snapshot provider class** `process.cpu.utilization` - no changes `dotnet.process.memory.virtual.utilization` - no changes **Windows Container Snapshot provider class** `process.cpu.utilization` - no changes `dotnet.process.memory.virtual.utilization` - calculates memory for the dotnet process only (instead of all processes) `container.cpu.limit.utilization` - new metric, same value as `process.cpu.utilization` `container.memory.limit.utilization` - new metric, calculates memory for all processes in the container **Linux Utilization Provider class** `process.cpu.utilization` - fixed incorrect scale calculation, instead of `host CPUs / CPU limit / CPU request`, it is now `host CPUs / CPU request` `dotnet.process.memory.virtual.utilization` - no changes `container.cpu.limit.utilization` - new metric, value is relative to CPU resource limit (aka maximum CPU units) `container.memory.limit.utilization` - new metric, calculates memory for all processes in the container `container.cpu.request.utilization` - new metric, same value as `process.cpu.utilization` ---- #### AI description (iteration 1) #### PR Classification New feature: Added metrics with correct names for resource monitoring. #### PR Summary This pull request introduces new metrics for resource monitoring with correct naming conventions and updates the related tests and implementation. - `LinuxUtilizationProvider.cs`: Added new metrics for container CPU and memory utilization, and updated existing metrics. - `AcceptanceTest.cs`: Added new tests for verifying the new metrics and updated existing tests for better coverage. - `ResourceUtilizationInstruments.cs`: Defined new constants for the new metrics. - Removed `WindowsCounters.cs` as it is no longer needed.
Fixes #5113
Microsoft Reviewers: Open in CodeFlow