Publish richer virtual disk statistics to oximeter #746
Conversation
- Use the `virtual_disk:*` timeseries defined in Omicron to publish statistics about the block operations performed on virtual disks. - Add concept of a completion callback to the existing block tracking object, and expose a way to set the callback on a `block::Device`. - Create object for managing all the statistics for one virtual disk backed by Crucible. This is shared between the existing oximeter producer, and the block devices, via the above completion callback mechanism. As I/Os complete, the stats are updated, and oximeter collects them periodically. - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use `virtual_machine:reset` definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the `ServerStats` types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
|
This still needs some testing, but it's a larger chunk than I intended so I'd like to get early eyes on it. I took this opportunity to rework a lot of the metrics initialization, which had become pretty Byzantine. The main point here is to publish richer virtual disk metrics than what we currently get from Crucible. This adds the same counters we had before, and also failure counters and histograms tracking I/O latency and I/O sizes. These use the same block device tracking machinery that was added to handle USDT probes, adding a completion callback to that struct. We now set the completion callback to update the statistics for each virtual disk. Those stats are shared with the oximeter producer registry, so they're published along with everything else. The extras that came along for the ride here are a mishmash of cleanup tasks, mostly listed in the commit message. This PR also relies on a forthcoming PR in Omicron that adds the schema for the new statistics. I'll link that once I post it, and follow up with testing notes as well. |
|
Talked offline with @ahl. He questions the value of putting the sled identifiers on this kind of timeseries at all, which I can understand. I might have been overzealous in my attempt to address oxidecomputer/omicron#5267. |
|
To clarify: I honor the utility (ie to us in support in some situations), but I think it complicated the authz model we've been discussing. |
|
Ok, I've done an initial round of testing. The Omicron side is still blocked on oxidecomputer/omicron#6409 and this WIP branch: https://github.com/oxidecomputer/omicron/tree/new-virtual-disk-timeseries-definition. But incorporating both of those locally and creating a VM does result in the expected timeseries in ClickHouse. Here are all of them: Digging into individual things looks like I expect: So that shows we had 109 total reads, and the I/O size histogram shows that those were broken out into 105 in the range [0, 4KiB), 1 in [4KiB, 8KiB), and 3 in [8KiB, 16KiB). So there are the same number of items in both, meaning we're consistent at least. |
- Use 512B as the lowest I/O size histogram bin - Update Crucible dep so we can test in CI - Fixups for some cfg directives around kstats
|
There's one kind of structural question my testing revealed. These virtual disk stats are updated all the time, but only sampled when I think it largely depends on how densely we want to sample the data. I'd defer to @leftwo and @ahl on that probably. I reduced the collection interval to 10s, which still feels borderline, but I admit that's a WAG. |
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
This currently brings in all non virtual_disk stats/initializer/kstat changes from @bnaecker's #746 since that abstracts where and how we do start tracking kstats. Includes: - Straighten out a bunch of the initialiazation logic for the pre-existing metrics, producer server, and registry. This grew pretty organically, and was getting unwieldy and confusing. - Use virtual_machine:reset definition from TOML - Move kstat sampler creation to machine initialization process, rather than buried in the producer server startup. This lets us use the sampler in-line when initializing the vCPUs, where it should be much easier to re-use when we want to add more kstat-based metrics, such as Viona link stats. - Simplify and cleanup the ServerStats types, and remove the kstat sampler from it, which never should have been there at all. - Bump kstat-rs dep, which includes the new version that is cross-platform. This gets rid of a bunch of cfg-soup in the stats modules.
Yeah, the existing |
- Clarify when we create kstat sampler - Style nits and comment cleanup - Don't return early from blockdev initialization if we fail to track stats, just continue instead
- Remove default impl of `set_completion_callback()` - Cleanup internals of `Tracking` creation
|
Just a note on the sampling interval. I talked in chat with @leftwo, and we decided that the current sampling rate of 10s is sufficient. In this PR, the sampling rate of these statistics is defined by the rate at which oximeter polls -- Propolis only generates sample on a request from oximeter. If we wanted to change this, we have two choices. We could lower that polling rate from oximeter, or we could decouple the rate at which we internally generate samples from the block-dev producer and the rate at which oximeter asks Propolis for samples. That's an architectural change, since we'd need a task that continually generates samples from the block-dev. It's also fairly easy to do, and not necessary right now. We can decide that later without any breaking changes downstream. |
virtual_disk:*timeseries defined in Omicron to publish statistics about the block operations performed on virtual disks.block::Device.virtual_machine:resetdefinition from TOMLServerStatstypes, and remove the kstat sampler from it, which never should have been there at all.