Skip to content

server: add x-region, x-zone metrics to Node#104585

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
wenyihu6:xlocality-node
Jun 14, 2023
Merged

server: add x-region, x-zone metrics to Node#104585
craig[bot] merged 1 commit intocockroachdb:masterfrom
wenyihu6:xlocality-node

Conversation

@wenyihu6
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 commented Jun 8, 2023

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: #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.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the xlocality-node branch 6 times, most recently from 5d22835 to 8181159 Compare June 12, 2023 04:41
@wenyihu6 wenyihu6 marked this pull request as ready for review June 12, 2023 13:40
@wenyihu6 wenyihu6 requested review from a team as code owners June 12, 2023 13:40
@wenyihu6 wenyihu6 requested a review from kvoli June 12, 2023 13:40
Copy link
Copy Markdown
Contributor

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: 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.
@wenyihu6
Copy link
Copy Markdown
Contributor Author

pkg/server/node.go line 148 at r1 (raw file):

Previously, kvoli (Austen) wrote…

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.

Discussed this more offline -

The metrics with prefix exec are incremented at the end of batchInternal and are for batches that are guaranteed to have been executed without an early return. At the moment, we think these metrics are designed to measure byte count of processed batch requests (rather than executed) and get a sense of proportion of cross-region and cross-zone batches among them.

@wenyihu6
Copy link
Copy Markdown
Contributor Author

pkg/server/node.go line 1332 at r1 (raw file):

Previously, kvoli (Austen) wrote…

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.

That sounds like a good idea. I will make another PR to refactor the logic for all relevant PRs.

@wenyihu6
Copy link
Copy Markdown
Contributor Author

pkg/server/node.go line 1422 at r1 (raw file):

Previously, kvoli (Austen) wrote…

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.

Done.

@wenyihu6 wenyihu6 requested a review from kvoli June 13, 2023 13:49
Copy link
Copy Markdown
Contributor

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

@wenyihu6
Copy link
Copy Markdown
Contributor Author

bors r=kvoli

TFTRs!

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 13, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 14, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 14, 2023

Build succeeded:

kvoli added a commit to kvoli/cockroach that referenced this pull request Sep 29, 2023
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
craig bot pushed a commit that referenced this pull request Sep 29, 2023
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>
THardy98 pushed a commit to THardy98/cockroach that referenced this pull request Oct 6, 2023
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
@wenyihu6 wenyihu6 deleted the xlocality-node branch October 30, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants