drpcpool: add support for ShouldRecord#50
Conversation
cthumuluru-crdb
left a comment
There was a problem hiding this comment.
One minor comment. Otherwise looks good.
There was a problem hiding this comment.
Pull request overview
Adds a ShouldRecord callback to client connection and pool configuration to allow runtime control over whether metrics are recorded (intended for CockroachDB integration).
Changes:
- Replace
CollectMetricswithShouldRecord func() boolfor client byte metrics recording. - Add
ShouldRecord func() booltodrpcpool.Optionsand gate pool metric updates behind it. - Update and extend tests to reflect the new metric-recording controls.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/integration/metrics_test.go |
Updates integration tests to use ShouldRecord and validate the “not recorded when nil” behavior. |
drpcpool/pool.go |
Adds Options.ShouldRecord and gates pool metric initialization and updates behind it. |
drpcpool/pool_test.go |
Updates existing tests to set ShouldRecord, and adds coverage for ShouldRecord == false. |
drpcmetrics/metrics.go |
Extends metered transport wrapper to consult shouldRecord() per Read/Write. |
drpcconn/conn.go |
Replaces CollectMetrics with ShouldRecord and wires it into metered transport creation. |
drpcclient/dialoptions.go |
Adds WithShouldRecordFunc and passes the callback into drpcconn.Options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ToMeteredTransport( | ||
| tr drpc.Transport, bytesSent, | ||
| bytesRecv Counter, shouldRecord func() bool, | ||
| ) drpc.Transport { |
There was a problem hiding this comment.
ToMeteredTransport stores shouldRecord and Read/Write call it unconditionally; if a caller passes a nil shouldRecord this will panic at runtime. Consider defaulting shouldRecord to a func that always returns true (or false) when nil, and/or document/enforce that it must be non-nil.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add a
ShouldRecordfunc to drpcpool and dprcclient options to enable/disable pool metrics from cockroach.