Skip to content

drpcpool: add support for ShouldRecord#50

Merged
suj-krishnan merged 6 commits intocockroachdb:mainfrom
suj-krishnan:add_pool_should_record
Apr 6, 2026
Merged

drpcpool: add support for ShouldRecord#50
suj-krishnan merged 6 commits intocockroachdb:mainfrom
suj-krishnan:add_pool_should_record

Conversation

@suj-krishnan
Copy link
Copy Markdown

@suj-krishnan suj-krishnan commented Apr 5, 2026

Add a ShouldRecord func to drpcpool and dprcclient options to enable/disable pool metrics from cockroach.

Comment thread drpcconn/conn.go
Comment thread drpcmetrics/metrics.go
Comment thread drpcpool/pool.go
Comment thread drpcpool/pool.go Outdated
Copy link
Copy Markdown

@cthumuluru-crdb cthumuluru-crdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment. Otherwise looks good.

Comment thread drpcpool/pool_test.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CollectMetrics with ShouldRecord func() bool for client byte metrics recording.
  • Add ShouldRecord func() bool to drpcpool.Options and 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.

Comment thread drpcmetrics/metrics.go
Comment on lines +65 to +68
func ToMeteredTransport(
tr drpc.Transport, bytesSent,
bytesRecv Counter, shouldRecord func() bool,
) drpc.Transport {
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread drpcconn/conn.go Outdated
Comment thread drpcpool/pool.go Outdated
Comment thread drpcclient/dialoptions.go Outdated
Comment thread drpcclient/dialoptions.go
suj-krishnan and others added 2 commits April 6, 2026 19:48
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@suj-krishnan suj-krishnan merged commit 6c77a9e into cockroachdb:main Apr 6, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants