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

Add metrics with correct names for Resource Monitoring #5341

Merged
merged 39 commits into from
Aug 12, 2024

Conversation

evgenyfedorov2
Copy link
Contributor

@evgenyfedorov2 evgenyfedorov2 commented Aug 6, 2024

Fixes #5113

Rather than renaming, I added 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

Previous art: #5309

Microsoft Reviewers: Open in CodeFlow

@evgenyfedorov2 evgenyfedorov2 requested a review from mobratil August 6, 2024 14:33
@evgenyfedorov2 evgenyfedorov2 marked this pull request as ready for review August 6, 2024 14:33
@evgenyfedorov2 evgenyfedorov2 requested a review from RussKie August 6, 2024 15:06
@evgenyfedorov2 evgenyfedorov2 marked this pull request as draft August 8, 2024 16:38
@evgenyfedorov2 evgenyfedorov2 marked this pull request as ready for review August 9, 2024 14:36
@evgenyfedorov2 evgenyfedorov2 requested a review from RussKie August 9, 2024 14:43
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good.
We just need to correct the xml-docs (since the API may become public due to #5345) and fix the code styling.

@evgenyfedorov2
Copy link
Contributor Author

evgenyfedorov2 commented Aug 12, 2024

@joperezr - could you please take a look and give your approval when you have time?

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. I'll let @RussKie take onemore look here.

@RussKie RussKie merged commit cbcef5c into dotnet:main Aug 12, 2024
6 checks passed
joperezr added a commit to joperezr/extensions that referenced this pull request Aug 15, 2024
…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.
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect metric names in ResourceUtilizationInstruments
3 participants