storage: support per-store IO metrics with fine granularity #119885
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
CheranMahalingam
force-pushed
the
granular_per_store_metrics
branch
14 times, most recently
from
e7dd5eb
to
3a0538f
Compare
|
These changes were verified e2e by running a 3-node cluster, each with additional ebs volumes to run 2 stores. 
The figure compares the individual store write metrics with the existing node-wide disk write metrics for node 3 in the cluster and shows that the store metrics almost add up to the system metrics.
|
CheranMahalingam
force-pushed
the
granular_per_store_metrics
branch
from
3a0538f
to
930890a
Compare
CheranMahalingam
requested review from
itsbilal,
abarganier,
jbowens and
aadityasondhi
and removed request for
a team
CheranMahalingam
force-pushed
the
granular_per_store_metrics
branch
from
053daaf
to
cd39477
Compare
jbowens
approved these changes
Mar 14, 2024
There was a problem hiding this comment.
Reviewed 1 of 4 files at r4, 1 of 12 files at r5.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi, @abarganier, and @itsbilal)
|
bors r=jbowens,abarganier |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors r- @CheranMahalingam I think you need to rebase over master. |
|
Canceled. |
Currently, timeseries metrics are collected on a 10s interval which hides momentary spikes in IO operations. This commit introduces a disk monitoring system that allows callers to subscribe to stats for a block device sampled every 100ms. Epic: None. Release note: None.
Currently, admission control consumes disk bandwidth stats aggregated across all block devices, including noise introduced by reads/writes from unrelated processes. This commit adds support for aggregating disk bandwidth stats across only monitored block devices which are used by the cockroach process. Epic: None. Release note: None.
CheranMahalingam
force-pushed
the
granular_per_store_metrics
branch
from
cd39477
to
3d72b2c
Compare
Currently, disk metrics are computed and aggregated at a node level. However, a cockroach node can run multiple stores. This commit adds new timeseries disk metrics computed per-store. Epic: None. Release note (general change): The following metrics were added for observability of per-store disk events: - storage.disk.read.count - storage.disk.read.bytes - storage.disk.read.time - storage.disk.write.count - storage.disk.write.bytes - storage.disk.write.time - storage.disk.io.time - storage.disk.weightedio.time - storage.disk.iopsinprogress The metrics match the definitions of the sys.host.disk.* system metrics.
CheranMahalingam
force-pushed
the
granular_per_store_metrics
branch
from
3d72b2c
to
f33445b
Compare
|
bors r=jbowens,abarganier |
CheranMahalingam
added a commit
to CheranMahalingam/cockroach
that referenced
this pull request
Mar 25, 2024
This commit cleans up changes from cockroachdb#119885. There is no longer a need for users to specify a disk name when specifying the provisioned bandwidth since we can now automatically infer disk names from the `StoreSpec.Path` and the underlying block device. Informs: cockroachdb#86857. Epic: None. Release note (ops change): The provisioned-rate field, if specified, should no longer accept a disk-name or an optional bandwidth field. To use the disk bandwidth constraint the store-spec must contain provisioned-rate=bandwidth=<bandwidth-bytes/s>, otherwise the cluster setting kv.store.admission.provisioned_bandwidth will be used.
craig bot
pushed a commit
that referenced
this pull request
Mar 26, 2024
120895: admission: remove `DiskName` from `StoreSpec.ProvisionedRateSpec` r=sumeerbhola a=CheranMahalingam This commit cleans up changes from #119885. There is no longer a need for users to specify a disk name when specifying the provisioned bandwidth since we can now automatically infer disk names from the `StoreSpec.Path` and the underlying block device. Informs: #86857. Epic: None. Release note (ops change): The provisioned-rate field, if specified, should no longer accept a disk-name or an optional bandwidth field. To use the disk bandwidth constraint the store-spec must contain provisioned-rate=bandwidth=<bandwidth-bytes/s>, otherwise the cluster setting kv.store.admission.provisioned_bandwidth will be used. Co-authored-by: Cheran Mahalingam <cheran.mahalingam@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, timeseries metrics are collected on a 10s interval which hides momentary spikes in IO. This commit introduces a central disk monitoring system that polls for disk stats at a 100ms interval. Additionally, the current system accumulates disk metrics across all block devices which includes noise from unrelated processes. This commit also adds support for exporting per-store IO metrics (i.e. IO stats for block devices that map to stores used by Cockroach).
These changes will be followed up by a PR to remove the need for customers to specify disk names when setting the provisioned bandwidth for each store as described in #109350.
Fixes: #104114, #112898.
Informs: #89786.
Epic: None.
Release note: None.