*: plug in no-op metric implementations for nil fields#49
*: plug in no-op metric implementations for nil fields#49suj-krishnan wants to merge 1 commit intocockroachdb:mainfrom
Conversation
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.
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.
| // initServerMetrics copies the caller-supplied metrics into the server, | ||
| // substituting no-op implementations for any nil fields. | ||
| func (s *Server) initServerMetrics(metrics *ServerMetrics) { | ||
| if metrics != nil { | ||
| s.metrics = *metrics | ||
| } | ||
| if s.metrics.TLSHandshakeErrors == nil { | ||
| s.metrics.TLSHandshakeErrors = drpcmetrics.NoOpCounter{} | ||
| } |
There was a problem hiding this comment.
initServerMetrics claims to substitute no-op implementations for “any nil fields”, but it currently only initializes TLSHandshakeErrors. Either also set BytesSent/BytesRecv to drpcmetrics.NoOpCounter{} here, or adjust the comment to reflect that byte counters are handled later by NewMeteredTransport.
| // MeteredTransport wraps a Transport and increments byte counters on each | ||
| // Read and Write call. | ||
| type MeteredTransport struct { | ||
| type meteredTransport struct { | ||
| drpc.Transport | ||
| BytesSent Counter | ||
| BytesRecv Counter | ||
| bytesSent Counter | ||
| bytesRecv Counter | ||
| } | ||
|
|
||
| func NewMeteredTransport(tr drpc.Transport, bytesSent, | ||
| bytesRecv Counter) drpc.Transport { | ||
| if bytesSent == nil { | ||
| bytesSent = NoOpCounter{} | ||
| } | ||
| if bytesRecv == nil { | ||
| bytesRecv = NoOpCounter{} | ||
| } | ||
| return &meteredTransport{Transport: tr, bytesSent: bytesSent, | ||
| bytesRecv: bytesRecv} | ||
| } |
There was a problem hiding this comment.
This change removes the exported MeteredTransport type (now meteredTransport), which is a breaking API change for any downstream users that constructed/typed-asserted drpcmetrics.MeteredTransport directly. If backward compatibility matters, consider keeping MeteredTransport as a deprecated wrapper/alias and introducing NewMeteredTransport as the preferred construction path.
| // MeteredTransport wraps a Transport and increments byte counters on each | ||
| // Read and Write call. | ||
| type MeteredTransport struct { | ||
| type meteredTransport struct { | ||
| drpc.Transport |
There was a problem hiding this comment.
The doc comment says “MeteredTransport wraps…” but the exported symbol is now NewMeteredTransport and the concrete type is unexported. Consider updating the comment to describe the constructor/wrapper returned by NewMeteredTransport to avoid misleading GoDoc users.
This is a follow-up to PR #33 with the following fixes: