server: add x-region, x-zone metrics to Node#104585
server: add x-region, x-zone metrics to Node#104585craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
5d22835 to
8181159
Compare
kvoli
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)
pkg/server/node.go line 148 at r1 (raw file):
metaBatchRequestsBytes = metric.Metadata{ Name: "batch_requests.bytes",
Have you considered prefixing with exec.? I see that some of the other node metrics for batch requests have that prefix such as exec.error.
pkg/server/node.go line 1332 at r1 (raw file):
func (n *Node) isCrossRegionCrossZoneBatch( ctx context.Context, ba *kvpb.BatchRequest, ) (bool, bool) {
nit: name the response variables or use an enum. The bools are exclusive now. No need to do in this PR (as it applies to the other x-zone/x-region stuff) but it would be nice to use an enum around the place, with states for "same-region,same-zone" and "bad config" essentially.
pkg/server/node.go line 1422 at r1 (raw file):
n.incrementBatchCounters(args) shouldIncrement := true
nit: a comment here would be helpful for the reader to quickly figure out what the interceptor is for and why shouldInc is true initially.
pkg/server/node_test.go line 790 at r1 (raw file):
// cross-region, cross-zone byte count metrics for batch requests sent and batch // responses received. func TestNodeBatchMetrics(t *testing.T) {
Nice test!
Previously, there were no metrics to observe cross-region, cross-zone traffic in batch requests / responses processed at receiver nodes. To improve this issue, this commit adds four new node metrics - ``` "batch_requests.bytes" "batch_responses.bytes" "batch_requests.cross_region.bytes" "batch_responses.cross_region.bytes" "batch_requests.cross_zone.bytes", "batch_responses.cross_zone.bytes" ``` The first two metrics track the total byte count of batch requests processed and batch responses received at node. Additionally, there are four metrics to track the aggregate counts processed and received across different regions and zones. Note that these metrics only track the receiver node since the node here represents the destination range node but not the gateway node. Part of: cockroachdb#103983 Release note (ops change): Six new metrics - "batch_requests.bytes", "batch_responses.bytes", "batch_requests.cross_region.bytes", "batch_responses.cross_region.bytes", "batch_requests.cross_zone.bytes", "batch_responses.cross_zone.bytes" - are now added to Node metrics. For accurate metrics, follow these assumptions: - Configure region and zone tier keys consistently across nodes. - Within a node locality, ensure unique region and zone tier keys. - Maintain consistent configuration of region and zone tiers across nodes.
|
Previously, kvoli (Austen) wrote…
Discussed this more offline - The metrics with prefix |
|
Previously, kvoli (Austen) wrote…
That sounds like a good idea. I will make another PR to refactor the logic for all relevant PRs. |
|
Previously, kvoli (Austen) wrote…
Done. |
kvoli
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)
|
bors r=kvoli TFTRs! |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
The X-locality log events were added in cockroachdb#104585 to the Node batch receive path, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the node batch path. Part of: cockroachdb#110648 Epic: none Release note: None
111140: roachtest: harmonize GCE and AWS machine types r=erikgrinaker,herkolategan,renatolabs a=srosenberg Previously, same (performance) roachtest executed in GCE and AWS may have used a different memory (per CPU) multiplier and/or cpu family, e.g., cascade lake vs ice lake. In the best case, this resulted in different performance baselines on an otherwise equivalent machine type. In the worst case, this resulted in OOMs due to VMs in AWS having 2x less memory per CPU. This change harmozines GCE and AWS machine types by making them as isomorphic as possible, wrt memory, cpu family and price. The following heuristics are used depending on specified `MemPerCPU`: `Standard` yields 4GB/cpu, `High` yields 8GB/cpu, `Auto` yields 4GB/cpu up to and including 16 vCPUs, then 2GB/cpu. `Low` is supported _only_ in GCE. Consequently, `n2-standard` maps to `m6i`, `n2-highmem` maps to `r6i`, `n2-custom` maps to `c6i`, modulo local SSDs in which case `m6id` is used, etc. Note, we also force `--gce-min-cpu-platform` to `Ice Lake`; isomorphic AWS machine types are exclusively on `Ice Lake`. Roachprod is extended to show cpu family and architecture on `List`. Cost estimation now correctly deals with _custom_ machine types. Finally, we change the default zone allocation in GCE from exclusively `us-east1-b` to ~25% `us-central1-b` and ~75% `us-east1-b`. This is inteded to balance the quotas for local SSDs until we eventually switch to PD-SSDs. Epic: none Fixes: #106570 Release note: None 111442: server,kvcoord: change x-locality log from vevent to vinfo r=arulajmani a=kvoli The X-locality log events were added in #104585 to the Node batch receive path, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the node batch path. The X-locality log events were added in #103963 for the dist sender, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the dist sender path. Resolves: #110648 Epic: none Release note: None 111475: server,settingswatcher: fix the local persisted cache r=stevendanna,aliher1911 a=knz There's two commits here, fixing 2 separate issues. Epic: CRDB-6671 ### server,settingswatcher: properly evict entries from the local persisted cache Fixes #70567. Supersedes #101472. (For context, on each node there is a local persisted cache of cluster setting customizations. This exists to ensure that configured values can be used even before a node has fully started up and can start reading customizations from `system.settings`.) Prior to this patch, entries were never evicted from the local persisted cache: when a cluster setting was reset, any previously saved entry in the cache would remain there. This is a very old bug, which was long hidden and was recently revealed when commit 2f5d717 was merged. In a nutshell, before this recent commit the code responsible to load the entries from the cache didn't fully work and so the stale entries were never restored from the cache. That commit fixed the loader code, and so the stale entries became active, which made the old bug visible. To fix the old bug, this present commit modifies the settings watcher to preserve KV deletion events, and propagates them to the persisted cache. (There is no release note because there is no user-facing release where the bug was visible.) ### settingswatcher: write-through to the persisted cache Fixes #111422. Fixes #111328. Prior to this patch, the rangefeed watcher over `system.settings` was updating the in-RAM value store before it propagated the updates to the persisted local cache. In fact, the update to the persisted local cache was lagging quite a bit behind, because the rangefeed watcher would buffer updates and only flush them after a while. As a result, the following sequence was possible: 1. client updates a cluster setting. 2. server is immediately shut down. The persisted cache has not been updated yet. 3. server is restarted. For a short while (until the settings watcher has caught up), the old version of the setting remains active. This recall of ghost values of a setting was simply a bug. This patch fixes that, by ensuring that the persisted cache is written through before the in-RAM value store. By doing this, we give up on batching updates to the persisted local store. This is deemed acceptable because cluster settings are not updated frequently. Co-authored-by: Stan Rosenberg <stan.rosenberg@gmail.com> Co-authored-by: Austen McClernon <austen@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
The X-locality log events were added in cockroachdb#104585 to the Node batch receive path, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the node batch path. Part of: cockroachdb#110648 Epic: none Release note: None
Previously, there were no metrics to observe cross-region, cross-zone traffic in
batch requests / responses processed at receiver nodes.
To improve this issue, this commit adds four new node metrics -
The first two metrics track the total byte count of batch requests processed and
batch responses received at node. Additionally, there are four metrics to track
the aggregate counts processed and received across different regions and zones.
Note that these metrics only track the receiver node since the node here
represents the destination range node but not the gateway node.
Part of: #103983
Release note (ops change): Six new metrics -
"batch_requests.bytes",
"batch_responses.bytes",
"batch_requests.cross_region.bytes",
"batch_responses.cross_region.bytes",
"batch_requests.cross_zone.bytes",
"batch_responses.cross_zone.bytes" - are now added to Node metrics.
For accurate metrics, follow these assumptions: