windows_exporter
windows_exporter copied to clipboard
prometheus-community
First implementation of physical_disk collector
This PR is to implement a first incarnation of the physical_disk collector, as derived from the logical_disk collector. This as a result from the request in https://github.com/prometheus-community/windows_exporter/issues/795#issuecomment-860029644.
Nice! Some feedback below. Also the CI is failing because you added it as a default collector, which means the end-to-end tests need to be updated (see tools/e2e-output.txt)
What has your preference: remove it from the defaults, or shall I update the end-to-end tests?
@carlpett : I've modified e2e-output and included it in this PR. Can you check if all is ok now?
Thanks @rob-scheepens, apart from clarification on if one of the old review comments has been addressed, this looks good! Can you make sure to sign the DCO, though?
Thanks! The e2e test appear to fail now though, although it's not clear to me why. Do they work locally for you?
Thanks! The e2e test appear to fail now though, although it's not clear to me why. Do they work locally for you?
When I execute "C:\git\rob-scheepens\windows_exporter>powershell -NonInteractive -ExecutionPolicy Bypass -File .\tools\end-to-end-test.ps1" that works fine, but when I trigger the script through make, it fails:
C:\git\rob-scheepens\windows_exporter>make e2e-test
powershell -NonInteractive -ExecutionPolicy Bypass -File .\tools\end-to-end-test.ps1
InputObject SideIndicator
----------- -------------
windows_exporter_collector_success{collector="physical_disk"} 1 <=
windows_exporter_collector_timeout{collector="physical_disk"} 0 <=
# HELP windows_physical_disk_idle_seconds_total Seconds that the disk was idle (PhysicalDisk.PercentIdleTime) <=
# TYPE windows_physical_disk_idle_seconds_total counter <=
# HELP windows_physical_disk_read_bytes_total The number of bytes transferred from the disk during read operations (PhysicalDisk.DiskReadBytesPerSec) <=
# TYPE windows_physical_disk_read_bytes_total counter <=
# HELP windows_physical_disk_read_latency_seconds_total Shows the average time, in seconds, of a read operation from the disk (PhysicalDisk.AvgDiskSecPerRead) <=
# TYPE windows_physical_disk_read_latency_seconds_total counter <=
# HELP windows_physical_disk_read_seconds_total Seconds that the disk was busy servicing read requests (PhysicalDisk.PercentDiskReadTime) <=
# TYPE windows_physical_disk_read_seconds_total counter <=
# HELP windows_physical_disk_read_write_latency_seconds_total Shows the time, in seconds, of the average disk transfer (PhysicalDisk.AvgDiskSecPerTransfer) <=
# TYPE windows_physical_disk_read_write_latency_seconds_total counter <=
# HELP windows_physical_disk_reads_total The number of read operations on the disk (PhysicalDisk.DiskReadsPerSec) <=
# TYPE windows_physical_disk_reads_total counter <=
# HELP windows_physical_disk_requests_queued The number of requests queued to the disk (PhysicalDisk.CurrentDiskQueueLength) <=
# TYPE windows_physical_disk_requests_queued gauge <=
# HELP windows_physical_disk_split_ios_total The number of I/Os to the disk were split into multiple I/Os (PhysicalDisk.SplitIOPerSec) <=
# TYPE windows_physical_disk_split_ios_total counter <=
# HELP windows_physical_disk_write_bytes_total The number of bytes transferred to the disk during write operations (PhysicalDisk.DiskWriteBytesPerSec) <=
# TYPE windows_physical_disk_write_bytes_total counter <=
# HELP windows_physical_disk_write_latency_seconds_total Shows the average time, in seconds, of a write operation to the disk (PhysicalDisk.AvgDiskSecPerWrite) <=
# TYPE windows_physical_disk_write_latency_seconds_total counter <=
# HELP windows_physical_disk_write_seconds_total Seconds that the disk was busy servicing write requests (PhysicalDisk.PercentDiskWriteTime) <=
# TYPE windows_physical_disk_write_seconds_total counter <=
# HELP windows_physical_disk_writes_total The number of write operations on the disk (PhysicalDisk.DiskWritesPerSec) <=
# TYPE windows_physical_disk_writes_total counter <=
make: *** [Makefile:19: e2e-test] Error 1
I've attached a make -d log but that didn't show anything obvious to me. Also running ProcMon during make did not show an obvious problem, log also attached.
Is the exit code a standard Windows code? If so it means "file not found", but as said, I didn't see that yet in the ProcMon file or make debug log. Thoughts @carlpett ?
make -d log.txt make_e2e-test.zip
@carlpett Looking at the various logs generated I noticed that the physical_disk collector is not started during the e2e-test. Looks like I missed a few places where it should be included, working on that.
Could that be the reason of the error code 2? if so, then why would things work ok when running the PS script directly, without make?
@carlpett : looks like the three additional changes and a rebuild fixed it - can you verify? I am now able to run make e2e-test:
Putting child 025bcc08 (e2e-test) PID 40258792 on the chain.
Live child 025bcc08 (e2e-test) PID 40258792
Main thread handle = 00000154
Reaping winning child 025bcc08 PID 40258792
Removing child 025bcc08 PID 40258792 from chain.
Successfully remade target file 'e2e-test'.```
Sorry for the delay @carlpett, for some reason I was sure I had already replied... :(
Regarding the labeling: I suggest we use the PhysicalDisk number in combination with a mountpoint if appropriate. Technically speaking a disk (PhysicalDisk) is never mounted, only partitions (LogicalDisks) are. An example label for C: would then look like:
0 C:
An example label for a raw disk without any partitions would be:
1
This seems to match your third possible resolution mentioned above.
Would you agree?
Sorry for the delay @carlpett, for some reason I was sure I had already replied... :(
Regarding the labeling: I suggest we use the PhysicalDisk number in combination with a mountpoint if appropriate. Technically speaking a disk (PhysicalDisk) is never mounted, only partitions (LogicalDisks) are. An example label for C: would then look like:
0 C:An example label for a raw disk without any partitions would be:
1This seems to match your third possible resolution mentioned above.
Would you agree?
@carlpett : friendly nudge. ;)
@carlpett Just had another look at the labels on a test machine, and to me this looks good. Are you ok with this too?
@rob-scheepens I don't think Calle's available at the moment. I could step in and review based on the existing comments.
Could you rebase the feature branch on master so the CI passes? There's also a merge conflict with the Go module files.
@rob-scheepens I don't think Calle's available at the moment. I could step in and review based on the existing comments.
Could you rebase the feature branch on
masterso the CI passes? There's also a merge conflict with the Go module files.
Hi @breed808 , I cleaned up the git commit history and backed out the unused import, so CI should be able to finish ok now.
Closing this since we now have a working version of physical_disk collector using PDH instead of Perflib, as mentioned in https://github.com/prometheus-community/windows_exporter/issues/799#issuecomment-1406568524. A PR for that will follow shortly.
Eventually I could reopen this PR, due to GitHub issues. I closed the duplicate I created of this, PR 1211.
@breed808 & @carlpett, this MR is tagged for milestone v0.21.0, however v0.22.0 is already released. Is it possible for this PR to go into the next release? tia!
Looks like the CI isn't happy due to the logging dependency in the collector. Have a look at #1192 for context on the logging change.
Apologies for the delay in getting back to this. I've re-run the CI and resolved the issues it's found.
Are we happy with using the disk number in the disk flag for the physical_disk collector metrics?
@breed808 thanks for checking, and yes, we're happy with the disk number.