Skip to content

Conversation

makazeu
Copy link
Contributor

@makazeu makazeu commented May 5, 2025

Closes #6371

Implement the following Disk I/O metrics for Linux:

  • system.disk.io
  • system.disk.operations
  • system.disk.io_time

All these values come from Linux Kernel's procfs-diskstats

Tested with Grafana + Prometheus with WSL2 + Ubuntu 24.04
image
image
image

cc @evgenyfedorov2

Microsoft Reviewers: Open in CodeFlow

@makazeu makazeu changed the title Implement disk io metrics for linux [Draft] Implement disk io metrics for linux May 5, 2025
@makazeu makazeu marked this pull request as ready for review May 11, 2025 08:24
@makazeu makazeu requested a review from a team as a code owner May 11, 2025 08:24
@makazeu makazeu changed the title [Draft] Implement disk io metrics for linux Implement disk io metrics for linux May 11, 2025
@amadeuszl
Copy link
Contributor

@amadeuszl
Copy link
Contributor

amadeuszl commented May 26, 2025

Since EnableDiskIoMetrics is experimental we can change the name of the flag to EnableSystemDiskIoMetrics, so in future we can add EnableContainerDiskIoMetrics or something like that, to make both more descriptive of what they add.

EnableDiskIoMetrics did this flag already went through any API review?

@makazeu
Copy link
Contributor Author

makazeu commented May 27, 2025

Can we add those metrics into documentation once merged https://github.com/dotnet/docs/blob/0d95e86e545ce4cfad2da2fe1e7fcee9da2edc8c/docs/core/diagnostics/built-in-metrics-diagnostics.md ?

We already support these metrics on Windows: #6181 , #6338. I'll add these to the document as soon as possible

@makazeu
Copy link
Contributor Author

makazeu commented May 27, 2025

Since EnableDiskIoMetrics is experimental we can change the name of the flag to EnableSystemDiskIoMetrics, so in future we can add EnableContainerDiskIoMetrics or something like that, to make both more descriptive of what they add.

EnableDiskIoMetrics did this flag already went through any API review?

No, EnableDiskIoMetrics has NOT gone through API review, we can change it as it's experimental.

As I commented in this, on Windows, the system.disk.* metrics have already been reflecting the values inside a "container" or VM, if we enable EnableContainerDiskIoMetrics along with EnableSystemDiskIoMetrics on Windows, what should we expect? Or I guess we should flag EnableContainerDiskIoMetrics as Linux-only?

@amadeuszl
Copy link
Contributor

Since EnableDiskIoMetrics is experimental we can change the name of the flag to EnableSystemDiskIoMetrics, so in future we can add EnableContainerDiskIoMetrics or something like that, to make both more descriptive of what they add.
EnableDiskIoMetrics did this flag already went through any API review?

No, EnableDiskIoMetrics has NOT gone through API review, we can change it as it's experimental.

As I commented in this, on Windows, the system.disk.* metrics have already been reflecting the values inside a "container" or VM, if we enable EnableContainerDiskIoMetrics along with EnableSystemDiskIoMetrics on Windows, what should we expect? Or I guess we should flag EnableContainerDiskIoMetrics as Linux-only?

EnableSystemDiskIoMetrics - should enable system.disk* metrics - VM and container are not the same, so reporting VM from system.* metric makes sense to me
EnableContainerDiskIoMetrics - should enable container.disk.* metrics, for Windows we may check if there're other counters reflecting it instead on VMs, I'm not fully into Windows implementation yet

@makazeu
Copy link
Contributor Author

makazeu commented May 27, 2025

Since EnableDiskIoMetrics is experimental we can change the name of the flag to EnableSystemDiskIoMetrics, so in future we can add EnableContainerDiskIoMetrics or something like that, to make both more descriptive of what they add.
EnableDiskIoMetrics did this flag already went through any API review?

No, EnableDiskIoMetrics has NOT gone through API review, we can change it as it's experimental.
As I commented in this, on Windows, the system.disk.* metrics have already been reflecting the values inside a "container" or VM, if we enable EnableContainerDiskIoMetrics along with EnableSystemDiskIoMetrics on Windows, what should we expect? Or I guess we should flag EnableContainerDiskIoMetrics as Linux-only?

EnableSystemDiskIoMetrics - should enable system.disk* metrics - VM and container are not the same, so reporting VM from system.* metric makes sense to me EnableContainerDiskIoMetrics - should enable container.disk.* metrics, for Windows we may check if there're other counters reflecting it instead on VMs, I'm not fully into Windows implementation yet

Makes sense, I have updated renamed the EnableDiskIoMetrics to EnableSystemDiskIoMetrics and I'd like to complete this PR. I will try to implement the container metrics in the future.

@makazeu
Copy link
Contributor Author

makazeu commented May 28, 2025

When I was trying to rename EnableDiskIoMetrics to EnableSystemDiskIoMetrics, I got this error in CI pipeline:

.dotnet/sdk/9.0.106/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ApiCompat.ValidatePackage.targets(39,5): error CP0002: (NETCORE_ENGINEERING_TELEMETRY=Build) Member 'bool Microsoft.Extensions.Diagnostics.ResourceMonitoring.ResourceMonitoringOptions.EnableDiskIoMetrics.get' exists on [Baseline] lib/net462/Microsoft.Extensions.Diagnostics.ResourceMonitoring.dll but not on lib/net462/Microsoft.Extensions.Diagnostics.ResourceMonitoring.dll

Looks like I can not simply rename an existing public API. @amadeuszl What is the correct procedure for making changes to a public API?

@amadeuszl
Copy link
Contributor

I've got recommendation to suppress it since it's experimental. How to suppress exact variable: https://learn.microsoft.com/en-us/dotnet/fundamentals/apicompat/package-validation/baseline-version-validator.

Can we also add a note to the variable

/// <remarks>Previously <c>EnableDiskIoMetrics</c>.</remarks>

@makazeu
Copy link
Contributor Author

makazeu commented May 31, 2025

I've got recommendation to suppress it since it's experimental. How to suppress exact variable: https://learn.microsoft.com/en-us/dotnet/fundamentals/apicompat/package-validation/baseline-version-validator.

Can we also add a note to the variable

/// <remarks>Previously <c>EnableDiskIoMetrics</c>.</remarks>

Please help review again, thanks!

@amadeuszl amadeuszl merged commit af446a2 into dotnet:main Jun 2, 2025
6 checks passed
@makazeu makazeu deleted the ImplementDiskIoMetricsForLinux branch June 2, 2025 15:25
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: Implement Disk IO Metrics for Linux in ResourceMonitoring
2 participants