admission: include more work queue info in tracing and errors

[aadityasondhi]

See individual commits.

---

admission: inject queue kind into work queue tracing

This patch plumbs down a queue type identifier (QueueKind) into the
WorkQueue to help track down the origin of items in the queue.

Informs: #113990

Release note: None

---

admission: include work priority in trace and errors

This patch now includes work priority in the WorkQueue traces and
error messages to better help track where work items originate from.

Informs #113990.

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 

[sumeerbhola]

Reviewed 1 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)

---

pkg/util/admission/admission.go line 553 at r1 (raw file):

const (
	KVAdmissionQueue QueueKind = iota

• Should we just dotype QueueKind string, since all we care about is the string representation. The indirection just seems to add code.
• We can leave it as the empty string for WorkKinds SQLKVResponseWork and SQLSQLResponseWork since there is only one queue each for those. Then work %s deadline expired while waiting in queue: %s would use the QueueKind if non-empty, else fallback to using the WorkKind.String().
• The strings we need are "kv-regular-cpu-queue", "kv-elastic-cpu-queue", "kv-regular-store-queue", "kv-elastic-store-queue".

[aadityasondhi]

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

---

pkg/util/admission/admission.go line 553 at r1 (raw file):

Previously, sumeerbhola wrote…

• Should we just dotype QueueKind string, since all we care about is the string representation. The indirection just seems to add code.
• We can leave it as the empty string for WorkKinds SQLKVResponseWork and SQLSQLResponseWork since there is only one queue each for those. Then work %s deadline expired while waiting in queue: %s would use the QueueKind if non-empty, else fallback to using the WorkKind.String().
• The strings we need are "kv-regular-cpu-queue", "kv-elastic-cpu-queue", "kv-regular-store-queue", "kv-elastic-store-queue".

Done. PTAL

[sumeerbhola]

Reviewed 4 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)

---

pkg/util/admission/admission.go line 555 at r3 (raw file):

//
// It is left empty for SQL types of WorkKind.
type QueueKind string

do we need to mark this by implementing SafeValue so that it plays well with redaction?

---

pkg/util/admission/work_queue.go line 380 at r3 (raw file):

	ambientCtx log.AmbientContext,
	workKind WorkKind,
	queueKind QueueKind,

in initWorkQueue if the queueKind is empty, let's initialize q.queueKind to workKind.String(). Then we don't need the if-else block when doing the logging and tracing.

---

pkg/util/admission/work_queue.go line 815 at r3 (raw file):

		recordAdmissionWorkQueueStats(span, waitDur, q.workKind, q.queueKind, true)
		log.Eventf(ctx, "deadline expired, waited in %s queue for %v",
			q.workKind, waitDur)

The log.Eventf should also use the queueKind.

---

pkg/util/admission/admissionpb/admission_stats.proto line 23 at r3 (raw file):

  int64 wait_duration_nanos = 1 [(gogoproto.casttype) = "time.Duration"];
  // String representation of admission work kind.
  string work_kind = 2;

we could just rename this as queue_kind and use the same tag number. The queue_kind subsumes the information provided by work_kind.

[aadityasondhi]

Made the changes PTAL.

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

---

pkg/util/admission/admission.go line 555 at r3 (raw file):

Previously, sumeerbhola wrote…

do we need to mark this by implementing SafeValue so that it plays well with redaction?

Done.

---

pkg/util/admission/work_queue.go line 380 at r3 (raw file):

Previously, sumeerbhola wrote…

in initWorkQueue if the queueKind is empty, let's initialize q.queueKind to workKind.String(). Then we don't need the if-else block when doing the logging and tracing.

Done.

---

pkg/util/admission/work_queue.go line 815 at r3 (raw file):

Previously, sumeerbhola wrote…

The log.Eventf should also use the queueKind.

Done.

---

pkg/util/admission/admissionpb/admission_stats.proto line 23 at r3 (raw file):

Previously, sumeerbhola wrote…

we could just rename this as queue_kind and use the same tag number. The queue_kind subsumes the information provided by work_kind.

Done.

[sumeerbhola]

Reviewed 2 of 3 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)

---

pkg/util/admission/work_queue.go line 815 at r3 (raw file):

Previously, aadityasondhi (Aaditya Sondhi) wrote…

Done.

I was suggesting the same string as prior to this PR: "deadline expired, waited in %s queue for %v" with the queueKind. The workKind is always inferrable from the queue kind.

---

pkg/util/admission/work_queue.go line 850 at r5 (raw file):

	}
	if queueKind == "" {
		queueKind = QueueKind(workKind.String())

why do we need this if WorkQueue.queueKind is always being passed here and it is already initialized?

[aadityasondhi]

Made the changes requested, PTAL again.

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

---

pkg/util/admission/work_queue.go line 815 at r3 (raw file):

Previously, sumeerbhola wrote…

I was suggesting the same string as prior to this PR: "deadline expired, waited in %s queue for %v" with the queueKind. The workKind is always inferrable from the queue kind.

Ah I miss understood, yeah makes sense.

---

pkg/util/admission/work_queue.go line 850 at r5 (raw file):

Previously, sumeerbhola wrote…

why do we need this if WorkQueue.queueKind is always being passed here and it is already initialized?

Sorry, that was me just being hasty. You are right, we always set this in initWorkQueue.

[sumeerbhola]

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

[aadityasondhi]

TFTR!

bors r=sumeerbhola

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from ba253a2 to blathers/backport-release-23.2-114664: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

---

error creating merge commit from ba253a2 to blathers/backport-release-23.2.0-rc-114664: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.0-rc failed. See errors above.

---

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