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 container.cpu.time metric #5806

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evgenyfedorov2
Copy link
Contributor

@evgenyfedorov2 evgenyfedorov2 commented Jan 20, 2025

Fixes #5800

Microsoft Reviewers: Open in CodeFlow

@evgenyfedorov2 evgenyfedorov2 requested a review from a team as a code owner January 20, 2025 11:31
@evgenyfedorov2 evgenyfedorov2 self-assigned this Jan 20, 2025
@evgenyfedorov2 evgenyfedorov2 force-pushed the users/evgenyfedorov/add_cputime branch from c56acb3 to 795d513 Compare January 20, 2025 12:47
@evgenyfedorov2 evgenyfedorov2 force-pushed the users/evgenyfedorov/add_cputime branch from 795d513 to d946512 Compare January 20, 2025 12:48
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 77.82 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.AI 88 89
Microsoft.Extensions.AI.OpenAI 77 78

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=922984&view=codecoverage-tab

@evgenyfedorov2
Copy link
Contributor Author

@andrey-noskov FYI

@@ -15,6 +16,7 @@ namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Windows;
internal sealed class WindowsContainerSnapshotProvider : ISnapshotProvider
{
private const double Hundred = 100.0d;
private const double TicksPerSecoundDouble = TimeSpan.TicksPerSecond;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private const double TicksPerSecoundDouble = TimeSpan.TicksPerSecond;
private const double TicksPerSecondDouble = TimeSpan.TicksPerSecond;

using var jobHandle = _createJobHandleObject();
var basicAccountingInfo = jobHandle.GetBasicAccountingInfo();

yield return new(basicAccountingInfo.TotalUserTime / TicksPerSecoundDouble, [new KeyValuePair<string, object?>("cpu.mode", "user")]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between UserTime and KernelTime from perspective of container running in Cosmic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I got the question right, but I don't think it is different from what you have without containers/k8s. See also https://unix.stackexchange.com/questions/87625/what-is-difference-between-user-space-and-kernel-space

Copy link
Contributor

Choose a reason for hiding this comment

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

For Linux (cgroupv2) I see that we're going to report (correct me if I'm wrong):
cpu.mode=user is /sys/fs/cgroup/cpu.stat so CPU time from container related cgroup
cpu.mode=system is /proc/stat) so host CPU time, which is whole machine, which in fact can host hundreds of containers.

For Windows based on JOBOBJECT:
cpu.mode=user - TotalUserTime - The total amount of user-mode execution time for all active processes associated with the job, what's the job in this case? And what's user-mode?
cpu.mode=system - TotalKernelTime - The total amount of kernel-mode execution time for all active processes associated with the job, what's the job in this case? And what's kernel-mode?
Basically, I'm trying to double check what those values are representing, so tags make sense, and that they represent same thing in both OSes.

@danmoseley danmoseley requested a review from Copilot January 27, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new metric to Resource Monitoring
3 participants