server: don't log for missing locality

[andrewbaptist]

Previously we would always log a message that the locality was unknown for requests from sql gateways. We should remove unnecessary logs from traces.

Epic: none

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[andrewbaptist]

This logging was quite annoying and useless on requests from sql pods.

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist)

---

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

	// We can't lookup the locality of SQL gateway nodes for cross locality
	// metrics, ignore them.
	if gatewayNodeID == 0 {

When do we expect this to be 0? The comment at:

cockroach/pkg/kv/kvpb/api.proto

Lines 2876 to 2879 in 5cb2264

	// gateway_node_id is the ID of the gateway node where the request originated.
	// For requests from tenants, this is set to the NodeID of the KV node handling
	// the BatchRequest.
	int32 gateway_node_id = 11 [(gogoproto.customname) = "GatewayNodeID",

would indicate it's set for all requests, including ones from SQL tenants. But evidently that's not the case -- so is this case unexpected to you?

---

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

	if err != nil {
		log.VInfof(ctx, 2,
			"failed to perform look up for node descriptor %v", err)

Should we be just deleting this log line instead of adding that extra if condition above?

[andrewbaptist]

Interesting, I hadn't noticed the comment on gateway_node_id in api.proto. The behavior is described here: c5d847f

and as that commit suggests it is only available in shared process multi-tenancy not in separate-process
multi-tenant. Looping in @yuzefovich for thoughts on how we should handle this case for SQL Pods.

[yuzefovich]

Let's just not log anything for separate process pods - we know that we are in such setup when GatewayNodeID is 0 and roachpb.ClientTenantFromContext returns ok=true.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @arulajmani)

[andrewbaptist]

Let's just not log anything for separate process pods - we know that we are in such setup when GatewayNodeID is 0 and roachpb.ClientTenantFromContext returns ok=true.

I updated the comment in api.proto as well to make this consistent. I agree it is worth logging in the case of a request that should have set it as it may help debugging. Can you validate my change to the comment as well as the check that is now in place.

I'm not 100% sure about whether a CLI will cause logging, but either way it is not overly important to suppress it in that case.

[yuzefovich]

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

[andrewbaptist]

bors r=yuzefovich

TFTR!

[arulajmani]

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

---

pkg/kv/kvpb/api.proto line 2879 at r2 (raw file):

  // originated. For requests from a shared-process cluster, this is set to the
  // NodeID of the KV node which created the BatchRequest. For requests from a
  // separate-process SQL Pod or a CLI, this is set to 0.

Thanks for capturing this here.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• local_roachtest_fips
• local_roachtest
• docker_image_amd64
• windows_build
• examples_orms
• check_generated_code
• macos_arm64_build
• linux_amd64_build
• acceptance
• lint
• macos_amd64_build
• linux_arm64_build
• linux_amd64_fips_build