logtestutils: generalize structured logging spy

[xinhaoz]

This commit generalizes the structured logging spy previously being used for datadriven telemetry tests so that it can repurposed for other structured logging channels.

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[abarganier]

Nice job identifying a need here. I ended up needing something similar the other day in a PR, so I'm sure others will benefit from this work.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kyle-a-wong, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 106 at r1 (raw file):

}

type StructuredLogSpy[T any] struct {

nit: docstring for the exported type explaining it's use/function.

Code quote:

type StructuredLogSpy[T any] struct {

---

pkg/util/log/logtestutils/structured_log_spy.go line 110 at r1 (raw file):

	// map of channels to pointer
	channels map[logpb.Channel]interface{}

nit: can this just be map[logpb.Channel]struct{}{}?

Code quote:

map[logpb.Channel]interface{}

---

pkg/util/log/logtestutils/structured_log_spy.go line 133 at r1 (raw file):

	format func(entry logpb.Entry) (T, error),
	filters ...func(entry logpb.Entry, formattedEntry T) bool,
) *StructuredLogSpy[T] {

nit: comments/docs explaining how things like format, filters, etc. work

Code quote:

func NewStructuredLogSpy[T any](
	testState *testing.T,
	channels []logpb.Channel,
	eventTypes []string,
	format func(entry logpb.Entry) (T, error),
	filters ...func(entry logpb.Entry, formattedEntry T) bool,
) *StructuredLogSpy[T] {

---

pkg/util/log/logtestutils/structured_log_spy.go line 164 at r1 (raw file):

	defer s.mu.Unlock()
	s.mu.filters = nil
}

Do we have examples of tests needing to modify filters post-construction? If a test needs different filters, should we just create a new StructuredLogSpy?

Code quote:

func (s *StructuredLogSpy[T]) AddFilters(f ...func(entry logpb.Entry, formattedEntry T) bool) {
	s.mu.Lock()
	defer s.mu.Unlock()
	s.mu.filters = append(s.mu.filters, f...)
}

func (s *StructuredLogSpy[T]) ClearFilters() {
	s.mu.Lock()
	defer s.mu.Unlock()
	s.mu.filters = nil
}

---

pkg/util/log/logtestutils/structured_log_spy.go line 193 at r1 (raw file):

	if n > len(s.mu.logs[ch]) {
		n = len(s.mu.logs[ch])
	}

Here and in other functions, should we add some existence checks for the various s.mu.logs[ch] map accesses?

Code quote:

	if n > len(s.mu.logs[ch]) {
		n = len(s.mu.logs[ch])
	}

[dhartunian]

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @kyle-a-wong, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 130 at r1 (raw file):

	testState *testing.T,
	channels []logpb.Channel,
	eventTypes []string,

I don't see usage with multiple channels/events. Is it necessary to support? Same with format + filters, can we get away with a single filter function that combines formatting? Not sure the interface really needs both since in the simplest case someone might just want to manipulate the entry directly.

---

pkg/util/log/logtestutils/structured_log_spy.go line 133 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: comments/docs explaining how things like format, filters, etc. work

+1

---

pkg/util/log/logtestutils/structured_log_spy.go line 164 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

Do we have examples of tests needing to modify filters post-construction? If a test needs different filters, should we just create a new StructuredLogSpy?

298f0dd#diff-2b876a20003068a4ddca879d6c283764b01434281d2015c00037d5e084012436R117-R119

[kyle-a-wong]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 208 at r1 (raw file):

}

func (s *StructuredLogSpy[T]) Intercept(entry []byte) {

Is there a reason why the funcs above have doc strings but this one and the ones below don't?

---

pkg/util/log/logtestutils/structured_log_spy.go line 227 at r1 (raw file):

			}
			return false
		}()

What is the purpose of this being an IFFE?

Code quote:

		found := func() bool {
			for _, re := range s.eventTypeRe {
				if re.MatchString(logEntry.Message) {
					return true
				}
			}
			return false
		}()

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, @kyle-a-wong, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 106 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: docstring for the exported type explaining it's use/function.

Done.

---

pkg/util/log/logtestutils/structured_log_spy.go line 110 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: can this just be map[logpb.Channel]struct{}{}?

Done.

---

pkg/util/log/logtestutils/structured_log_spy.go line 130 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I don't see usage with multiple channels/events. Is it necessary to support? Same with format + filters, can we get away with a single filter function that combines formatting? Not sure the interface really needs both since in the simplest case someone might just want to manipulate the entry directly.

I don't see any harm with extending it to multiple channels. One motivation for this many-to-one case: I was hoping to use this for some datadriven testing for sql exec / perf channels similar to what we have for telemetry and it would be nice to have one log spy for all of them, since they'd all use the same string format function. Appending a channel name to the log spy in that case will save us from having to create and manage separate log spies for all of them.

Similarly for the format / filter functions, I've found some utility in separating those two ops for reuse. Esp if we might have different tests being setup with their own log spies for the same events, it's nice not having to write the same format function multiple times, but allow those spies to have different filtering behaviours. LMK though and I can convert it to a single channel / function if desired.

---

pkg/util/log/logtestutils/structured_log_spy.go line 133 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

+1

Done.

---

pkg/util/log/logtestutils/structured_log_spy.go line 164 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

298f0dd#diff-2b876a20003068a4ddca879d6c283764b01434281d2015c00037d5e084012436R117-R119

Yes, we do always have the option of creating a separate log spy. I find these functions just add some shortcuts so that we don't always need to.

---

pkg/util/log/logtestutils/structured_log_spy.go line 193 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

Here and in other functions, should we add some existence checks for the various s.mu.logs[ch] map accesses?

Working with a nil slice is actually pretty ergonomic in golang. The bottom will just append no entries if we get a nil entry.

---

pkg/util/log/logtestutils/structured_log_spy.go line 208 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

Is there a reason why the funcs above have doc strings but this one and the ones below don't?

Added!

---

pkg/util/log/logtestutils/structured_log_spy.go line 227 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

What is the purpose of this being an IFFE?

Was just trying to avoid having to do a loop with a break to accomplish array.some. Can convert it to that or something else if you have suggestions to make it more readable.

[kyle-a-wong]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 208 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Added!

lgtm

---

pkg/util/log/logtestutils/structured_log_spy.go line 227 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Was just trying to avoid having to do a loop with a break to accomplish array.some. Can convert it to that or something else if you have suggestions to make it more readable.

Ahhh, thanks for the info. I don't have a particular opinion or suggestion about what is most readable. I was mostly just wondering if this was an idiosyncrasy of golang or just an implementation decision.

[abarganier]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @nkodali, and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 164 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Yes, we do always have the option of creating a separate log spy. I find these functions just add some shortcuts so that we don't always need to.

Ack, thanks for the example usage.

[dhartunian]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @nkodali and @xinhaoz)

---

pkg/util/log/logtestutils/structured_log_spy.go line 130 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I don't see any harm with extending it to multiple channels. One motivation for this many-to-one case: I was hoping to use this for some datadriven testing for sql exec / perf channels similar to what we have for telemetry and it would be nice to have one log spy for all of them, since they'd all use the same string format function. Appending a channel name to the log spy in that case will save us from having to create and manage separate log spies for all of them.

Similarly for the format / filter functions, I've found some utility in separating those two ops for reuse. Esp if we might have different tests being setup with their own log spies for the same events, it's nice not having to write the same format function multiple times, but allow those spies to have different filtering behaviours. LMK though and I can convert it to a single channel / function if desired.

👍 thanks for the note

[xinhaoz]

TFTR!
bors r+

[craig]

Build failed (retrying...):

• docker_image_amd64

[craig]

Build succeeded:

• acceptance
• macos_amd64_build
• windows_build
• examples_orms
• check_generated_code
• macos_arm64_build
• docker_image_amd64
• unit_tests
• linux_arm64_build
• local_roachtest
• linux_amd64_fips_build
• local_roachtest_fips
• lint
• linux_amd64_build