-
Notifications
You must be signed in to change notification settings - Fork 691
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 physical_disk collector #1251
Add physical_disk collector #1251
Conversation
Signed-off-by: Rob Scheepens <rob.scheepens@nutanix.com> Signed-off-by: Brantley West <brantley@nutanix.com>
Signed-off-by: Rob Scheepens <rob.scheepens@nutanix.com> Signed-off-by: Brantley West <brantley@nutanix.com>
Signed-off-by: Rob Scheepens <rob.scheepens@nutanix.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this! Is this intended to replace #803?
I've left feedback for a few items. Let me know if any clarification is required.
@@ -49,7 +49,7 @@ type prometheusVersion struct { | |||
} | |||
|
|||
const ( | |||
defaultCollectors = "cpu,cs,logical_disk,net,os,service,system,textfile" | |||
defaultCollectors = "cpu,cs,logical_disk,physical_disk,net,os,service,system,textfile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have both the logical_disk
and physical_disk
collectors enabled by default? That is, do the metrics exposed by both collectors overlap in scope?
`requests_queued` | Number of requests outstanding on the disk at the time the performance data is collected | gauge | `disk` | ||
`read_bytes_total` | Rate at which bytes are transferred from the disk during read operations | counter | `disk` | ||
`reads_total` | Rate of read operations on the disk | counter | `disk` | ||
`write_bytes_total` | Rate at which bytes are transferred to the disk during write operations | counter | `disk` | ||
`writes_total` | Rate of write operations on the disk | counter | `disk` | ||
`read_seconds_total` | Seconds the disk was busy servicing read requests | counter | `disk` | ||
`write_seconds_total` | Seconds the disk was busy servicing write requests | counter | `disk` | ||
`free_bytes` | Unused space of the disk in bytes (not real time, updates every 10-15 min) | gauge | `disk` | ||
`size_bytes` | Total size of the disk in bytes (not real time, updates every 10-15 min) | gauge | `disk` | ||
`idle_seconds_total` | Seconds the disk was idle (not servicing read/write requests) | counter | `disk` | ||
`split_ios_total` | Number of I/Os to the disk split into multiple I/Os | counter | `disk` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the prefix to the documented metrics? E.G. windows_physical_disk_requests_queued
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
|
||
func init() { | ||
registerCollector("physical_disk", NewPhysicalDiskCollector, "PhysicalDisk") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init()
function will need to be removed, with collector registration details moved to init.go. I'm happy to make this change if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ben, yes please make this change for me if you could.
"collector.physical_disk.disk-whitelist", | ||
"Regexp of disks to whitelist. Disk name must both match whitelist and not match blacklist to be included.", | ||
).Default(".+").String() | ||
diskBlacklist = kingpin.Flag( | ||
"collector.physical_disk.disk-blacklist", | ||
"Regexp of disks to blacklist. Disk name must both match whitelist and not match blacklist to be included.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the flag names from whitelist
and blacklist
to include
and exclude
, respectively?
See #679 for context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing in favor of #803. |
This PR implements the physical_disk collector, based on the logical_disk collector. This is a result from the request in #795 (comment).