insights: call ingester methods by reference

[matthewtodd]

Fixes the data race observed in #85988. Passing this data structure by
value meant that the fullHandler was working with a different object
than the external callers.

Release justification: Category 3: Fixes for high-priority or
high-severity bugs in existing functionality

Release note: None

[cockroach-teamcity]

This change is 

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)

---

-- commits line 4 at r1:
nit: can you add a little more explanation about what this change does?

[maryliag]

Just a small nit, otherwise

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)

[j82w]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w, @maryliag, and @matthewtodd)

---

pkg/sql/sqlstats/insights/ingester.go line 58 at r1 (raw file):

}

func (i *concurrentBufferIngester) Start(ctx context.Context, stopper *stop.Stopper) {

Should a test be added to avoid possible regressions?

[matthewtodd]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w and @maryliag)

---

-- commits line 4 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

nit: can you add a little more explanation about what this change does?

Done.

---

pkg/sql/sqlstats/insights/ingester.go line 58 at r1 (raw file):

Previously, j82w wrote…

Should a test be added to avoid possible regressions?

Good q. I'm wondering what that test would look like. I think we could come up with something, but I also think it's unlikely we'll revert to passing by value, so 🤷 Perhaps the data race detector is our friend here. WDYT?

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w and @maryliag)

---

pkg/sql/sqlstats/insights/ingester.go line 58 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Good q. I'm wondering what that test would look like. I think we could come up with something, but I also think it's unlikely we'll revert to passing by value, so 🤷 Perhaps the data race detector is our friend here. WDYT?

The previous behaviour was never part of a release, so this change won't affect any "old" behaviour

[xinhaoz]

pkg/sql/sqlstats/insights/insights.go line 141 at r2 (raw file):

// New builds a new Registry.
func New(st *cluster.Settings, metrics Metrics) Registry {
	return newConcurrentBufferIngester(newRegistry(st, metrics))

Just wondering for learning's sake, are there cases when we want to return an interface in the constructor over the exported struct directly? In the past I recall this was discouraged.

[matthewtodd]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w, @maryliag, and @xinhaoz)

---

pkg/sql/sqlstats/insights/insights.go line 141 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Just wondering for learning's sake, are there cases when we want to return an interface in the constructor over the exported struct directly? In the past I recall this was discouraged.

Oh, that's interesting! Now that you mention it, I kind of recall those conversations, but I don't really remember the reasons. I'll ask in #go-learners.

[matthewtodd]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w, @maryliag, and @xinhaoz)

---

pkg/sql/sqlstats/insights/insights.go line 141 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Oh, that's interesting! Now that you mention it, I kind of recall those conversations, but I don't really remember the reasons. I'll ask in #go-learners.

Googling first, this result doesn't seem particularly Go-specific; you could just as easily balance the same forces the other way, as is done in, say, Java, where returning interfaces is the norm. I wonder if the difference is purely cultural, or if there's some technical benefit?

[matthewtodd]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w, @maryliag, and @xinhaoz)

---

pkg/sql/sqlstats/insights/insights.go line 141 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Googling first, this result doesn't seem particularly Go-specific; you could just as easily balance the same forces the other way, as is done in, say, Java, where returning interfaces is the norm. I wonder if the difference is purely cultural, or if there's some technical benefit?

From what I've digested, I think it probably does make sense to return a struct here. (Though the interface was maybe slightly helpful tactically in the move to wrap the ingester around the registry.)

In general, I've been feeling like the Registry interface is too big: the needs of the ingester (Start) are completely separate from the needs of the registry (enabled), and there's a reader/writer distinction just waiting to be made. I'll take a little time to play with it on Friday to see if something simpler shakes out.

[matthewtodd]

bors r+

[ajwerner]

A great rule of thumb for Go is accept interfaces, return structs.

https://medium.com/@cep21/preemptive-interface-anti-pattern-in-go-54c18ac0668a

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)