kv: include locking strength and durability in Get/Scan/RevScan SafeFormat

[lyang24]

kv: include locking strength and durability in Get/Scan/RevScan SafeFormat

The goal of this pr is improving observability. We are adding lock strength
and lock durability with Get/Scan/RevScan request if the request is locking.
This is implemented by introducing an optional extension interface to Request
Interface called SafeFormatterRequest. We are also refactoring inside
BatchRequest.SafeFormat with the added interface. Please note the subtle
changed introduced here: if the EndKey is not present we print only Key with
square brackets, and this applies to all the request types.

Fixes: #114475
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?

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

• @nvanbenschoten (author of kv: include locking strength and durability in Get/Scan/RevScan SafeFormat #114475)

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

[cockroach-teamcity]

This change is 

[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?

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[nvanbenschoten]

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

---

-- commits line 2 at r3:
Same two comments about the commit message as in #114636.

---

pkg/kv/kvpb/batch.go line 813 at r3 (raw file):

		req := arg.GetInner()
		if et, ok := req.(*EndTxnRequest); ok {

I was thinking that this new SafeFormatterRequest interface could be used for all of these special cases. So instead, we'd just have some logic that looked like

if safeFormatterReq, ok := req.(SafeFormatterRequest); ok {
    safeFormatterReq.SafeFormat(s, verb)
} else {
    s.Print(req.Method())
}
if len(h.EndKey) > 0 {
    s.Printf(" [%s,%s)", h.Key, h.EndKey)
} else {
    s.Printf(" [%s]", h.Key)
}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[lyang24]

Thanks for the feedback, i have addressed the comments. Please note the subtle change that we are applying this logic at the end of request types now.

if len(h.EndKey) > 0 {
	s.Printf(" [%s,%s)", h.Key, h.EndKey)
} else {
	s.Printf(" [%s]", h.Key)

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

---

pkg/kv/kvpb/batch.go line 813 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I was thinking that this new SafeFormatterRequest interface could be used for all of these special cases. So instead, we'd just have some logic that looked like

if safeFormatterReq, ok := req.(SafeFormatterRequest); ok {
    safeFormatterReq.SafeFormat(s, verb)
} else {
    s.Print(req.Method())
}
if len(h.EndKey) > 0 {
    s.Printf(" [%s,%s)", h.Key, h.EndKey)
} else {
    s.Printf(" [%s]", h.Key)
}

Thanks it is so much cleaner after the refactor.

[nvanbenschoten]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lyang24)

---

pkg/kv/kvpb/api.go line 197 at r4 (raw file):

}

// SafeFormatterRequest is an optional extension interface used to allow request to do custom formatting.

It would be nice to add some request-specific tests for these requests which are implementing the SafeFormatterRequest interface. We could add those small unit tests in pkg/kv/kvpb/string_test.go, right below TestBatchRequestString.

When doing so, we can test the safe-string and redacted-string forms. TestReplicaStringAndSafeFormat is an example of how to do that.

---

pkg/kv/kvpb/api.go line 206 at r4 (raw file):

// SafeFormat implements the redact.SafeFormatter interface.
func (gr GetRequest) SafeFormat(s redact.SafePrinter, _ rune) {

These methods should all use pointer receivers.

---

pkg/kv/kvpb/api.go line 211 at r4 (raw file):

		return
	}
	s.Printf("[lockStrength=%s,lockDurability=%s]", gr.KeyLockingStrength, gr.KeyLockingDurability)

Here and below: we can make this less verbose. How about something like:

s.Printf("(%s,%s)", gr.KeyLockingStrength, gr.KeyLockingDurability)

---

pkg/kv/kvpb/batch.go line 813 at r3 (raw file):

Previously, lyang24 (Lanqing Yang) wrote…

Thanks it is so much cleaner after the refactor.

Agreed, this did turn out nicely!

[lyang24]

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

---

pkg/kv/kvpb/api.go line 197 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It would be nice to add some request-specific tests for these requests which are implementing the SafeFormatterRequest interface. We could add those small unit tests in pkg/kv/kvpb/string_test.go, right below TestBatchRequestString.

When doing so, we can test the safe-string and redacted-string forms. TestReplicaStringAndSafeFormat is an example of how to do that.

yep added the TestRequestSafeFormat the keys are printed in the batch request SafeFormat method so it wont display here just fyi.

---

pkg/kv/kvpb/api.go line 206 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

These methods should all use pointer receivers.

great catch!

[nvanbenschoten]

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

---

pkg/kv/kvpb/string_test.go line 103 at r5 (raw file):

// Unit test the requests that implemented SafeFormatterRequest interface.
func TestRequestSafeFormat(t *testing.T) {
	gr := &kvpb.GetRequest{

Can we turn this into a table-driven test to make it easier to add new request types? Something like:

testCases := []struct {
	req        Request
	redactable string
	redacted   string
}{
	{
		req: &kvpb.GetRequest{
			RequestHeader: kvpb.RequestHeader{
				Key: roachpb.Key("a"),
			},
			KeyLockingStrength:   lock.Shared,
			KeyLockingDurability: lock.Unreplicated,
		},
		redactable: "Get(Shared,Unreplicated)",	
		redacted: "Get(Shared,Unreplicated)",	
	},
	// ...
}
for _, c := range testCases {
	t.Run(c.Method(), func(t *testing.T) {
		// ...
	})
}

---

pkg/kv/kvpb/string_test.go line 170 at r5 (raw file):

		PusheeTxn: pusheeTxn.TxnMeta,
	}
	require.EqualValues(t, "PushTxn(‹PUSH_TIMESTAMP›,00fbff58->00fbff59)", redact.Sprint(ptr))

The redaction of PUSH_TIMESTAMP is interesting here.

We should add a new commit which makes PushTxnType implement redact.SafeValue.

To do that, we'll want a line like the following to api.go:

// SafeValue implements the redact.SafeValue interface.
func (PushTxnType) SafeValue() {}

And then we'll have to add an entry to pkg/testutils/lint/passes/redactcheck/redactcheck.go.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

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

[lyang24]

Thank you for reviewing.

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

---

pkg/kv/kvpb/string_test.go line 103 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we turn this into a table-driven test to make it easier to add new request types? Something like:

testCases := []struct {
	req        Request
	redactable string
	redacted   string
}{
	{
		req: &kvpb.GetRequest{
			RequestHeader: kvpb.RequestHeader{
				Key: roachpb.Key("a"),
			},
			KeyLockingStrength:   lock.Shared,
			KeyLockingDurability: lock.Unreplicated,
		},
		redactable: "Get(Shared,Unreplicated)",	
		redacted: "Get(Shared,Unreplicated)",	
	},
	// ...
}
for _, c := range testCases {
	t.Run(c.Method(), func(t *testing.T) {
		// ...
	})
}

Thank you this is a nice pattern looking forward to incorporate in my future developments.

---

pkg/kv/kvpb/string_test.go line 170 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The redaction of PUSH_TIMESTAMP is interesting here.

We should add a new commit which makes PushTxnType implement redact.SafeValue.

To do that, we'll want a line like the following to api.go:

// SafeValue implements the redact.SafeValue interface.
func (PushTxnType) SafeValue() {}

And then we'll have to add an entry to pkg/testutils/lint/passes/redactcheck/redactcheck.go.

Thank you for catching this. I fixed it.

[nvanbenschoten]

bors r=nvanbenschoten

Reviewed 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lyang24)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[nvanbenschoten]

bors r-

[nvanbenschoten]

@lyang24 this PR skewed with e53cc10. Could you rebase it on master?

[lyang24]

Hi Nathan, This pr is rebased on master now.

[nvanbenschoten]

bors r=nvanbenschoten

[nvanbenschoten]

bors r=nvanbenschoten

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[michae2]

This is really helpful! Thank you!

[michae2]

@lyang24 @nvanbenschoten I'm going to backport this to 23.2. I think it would be really helpful to have in v23.2.1+.

blathers backport release-23.2