*: plug in no-op metric implementations for nil fields#49
*: plug in no-op metric implementations for nil fields#49suj-krishnan merged 1 commit intocockroachdb:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes drpc metrics safer to use by ensuring metric interfaces default to no-op implementations when callers omit individual metric fields, and by funneling metered transport wrapping through a constructor that handles nil counters.
Changes:
- Initialize nil metric fields with no-op implementations for server and pool metrics at construction time.
- Replace direct use of
drpcmetrics.MeteredTransportwithdrpcmetrics.NewMeteredTransport, which substitutes no-op counters when needed. - Restructure server/client/pool code paths to rely on initialized metrics rather than per-call nil guards.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| drpcserver/server.go | Adds server-side metrics initialization and switches metering/TLS error counting to use initialized metrics. |
| drpcpool/pool.go | Adds pool-side metrics initialization so pool operations can emit metrics without nil checks. |
| drpcmetrics/metrics.go | Introduces NewMeteredTransport and makes the metered transport implementation unexported. |
| drpcconn/conn.go | Wraps client transports via NewMeteredTransport when metrics are provided. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
drpcconn/conn.go
Outdated
| tr = mt | ||
| c.tr = tr | ||
| } | ||
| c.initClientMetrics(opts.Metrics, tr) |
There was a problem hiding this comment.
Why can't opts take value and not a pointer?
There was a problem hiding this comment.
I was using this field (!= nil) to decide whether the code should collect metrics or not. Without it, we would unnecessarily add the indirection of the wrapped transport, even if metrics are not being collected. I could use the existing bool CollectStats instead and change this to value (and that way, avoid the value in the Conn struct).
There was a problem hiding this comment.
Can rename CollectStats to CollectMetrics later (when we deprecate stats).
suj-krishnan
force-pushed
the
drpc_metrics_followup
branch
from
319fd52 to
6f017ef
Compare
suj-krishnan
force-pushed
the
drpc_metrics_followup
branch
from
124d486 to
7e3a93b
Compare
This is a follow-up to PR cockroachdb#33 with the following fixes: - Initialize nil metric fields with no-op implementations at construction time (server, client, pool) so that call sites don't need nil guards. - Unexport MeteredTransport behind a NewMeteredTransport constructor that handles nil counters.
suj-krishnan
force-pushed
the
drpc_metrics_followup
branch
from
17573c7 to
c4b5d22
Compare
This is a follow-up to PR #33 with the following fixes: