Skip to content

Commit 8fe32d3

Browse files
suj-krishnansuj-krishnan
committed
fixup! *: plug in no-op metric implementations for nil fields
1 parent 6f017ef commit 8fe32d3

6 files changed

Lines changed: 80 additions & 54 deletions

File tree

‎drpcclient/dialoptions.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,10 @@ func DialContext(ctx context.Context, address string, opts ...DialOption) (conn
134134
}
135135
}
136136

137+
if options.metrics == nil {
138+
options.metrics = &drpcmetrics.ClientMetrics{}
139+
}
140+
137141
return drpcconn.NewWithOptions(netConn, drpcconn.Options{
138142
Manager: drpcmanager.Options{
139143
Reader: drpcwire.ReaderOptions{
@@ -144,6 +148,6 @@ func DialContext(ctx context.Context, address string, opts ...DialOption) (conn
144148
},
145149
SoftCancel: false,
146150
},
147-
Metrics: options.metrics,
151+
Metrics: *options.metrics,
148152
}), nil
149153
}

‎drpcconn/conn.go‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,16 @@ type Options struct {
2525
// Manager controls the options we pass to the manager of this conn.
2626
Manager drpcmanager.Options
2727

28+
// TODO: (server): deprecate this
2829
// CollectStats controls whether the client should collect stats on the
2930
// rpcs it creates.
3031
CollectStats bool
3132

32-
// Metrics holds optional metrics the client will populate. If nil, no
33-
// metrics are recorded.
34-
Metrics *drpcmetrics.ClientMetrics
33+
// CollectMetrics controls whether the client should collect metrics
34+
CollectMetrics bool
35+
36+
// Metrics holds optional metrics the client will populate.
37+
Metrics drpcmetrics.ClientMetrics
3538
}
3639

3740
// Conn is a drpc client connection.
@@ -41,8 +44,7 @@ type Conn struct {
4144
mu sync.Mutex
4245
wbuf []byte
4346

44-
stats map[string]*drpcstats.Stats // TODO (server): deprecate stats
45-
metrics drpcmetrics.ClientMetrics
47+
stats map[string]*drpcstats.Stats // TODO (server): deprecate
4648
}
4749

4850
var _ drpc.Conn = (*Conn)(nil)
@@ -57,8 +59,13 @@ func NewWithOptions(tr drpc.Transport, opts Options) *Conn {
5759
tr: tr,
5860
}
5961

60-
c.initClientMetrics(opts.Metrics, tr)
62+
if opts.CollectMetrics {
63+
mt := drpcmetrics.NewMeteredTransport(tr, opts.Metrics.BytesSent,
64+
opts.Metrics.BytesRecv)
65+
c.tr = mt
66+
}
6167

68+
// TODO: (server): deprecate
6269
if opts.CollectStats {
6370
drpcopts.SetManagerStatsCB(&opts.Manager.Internal, c.getStats)
6471
c.stats = make(map[string]*drpcstats.Stats)
@@ -69,17 +76,6 @@ func NewWithOptions(tr drpc.Transport, opts Options) *Conn {
6976
return c
7077
}
7178

72-
// initClientMetrics copies the caller-supplied metrics into the conn and
73-
// wraps the transport with a metered transport if metrics are provided.
74-
func (c *Conn) initClientMetrics(metrics *drpcmetrics.ClientMetrics, tr drpc.Transport) {
75-
if metrics != nil {
76-
c.metrics = *metrics
77-
mt := drpcmetrics.NewMeteredTransport(tr, c.metrics.BytesSent,
78-
c.metrics.BytesRecv)
79-
c.tr = mt
80-
}
81-
}
82-
8379
// Stats returns the collected stats grouped by rpc.
8480
func (c *Conn) Stats() map[string]drpcstats.Stats {
8581
c.mu.Lock()

‎drpcpool/pool.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ type Options struct {
3838

3939
// Metrics holds optional metrics the pool will populate. If nil,
4040
// no metrics are recorded.
41-
Metrics *PoolMetrics
41+
Metrics PoolMetrics
4242

4343
// Labels holds optional labels to be attached to all metrics.
4444
Labels map[string]string
@@ -53,7 +53,6 @@ type Pool[K comparable, V Conn] struct {
5353
mu sync.Mutex
5454
entries map[K]*list[K, V]
5555
order list[K, V]
56-
metrics PoolMetrics
5756
}
5857

5958
// New constructs a new Pool with the provided Options.
@@ -62,7 +61,8 @@ func New[K comparable, V Conn](opts Options) *Pool[K, V] {
6261
opts: opts,
6362
entries: make(map[K]*list[K, V]),
6463
}
65-
pool.initPoolMetrics(opts.Metrics)
64+
65+
pool.initPoolMetrics()
6666

6767
// emit the metric (0 value) so it shows up as soon as the pool is created
6868
pool.updatePoolSize()
@@ -71,31 +71,29 @@ func New[K comparable, V Conn](opts Options) *Pool[K, V] {
7171

7272
// initPoolMetrics copies the caller-supplied metrics into the pool,
7373
// substituting no-op implementations for any nil fields.
74-
func (p *Pool[K, V]) initPoolMetrics(metrics *PoolMetrics) {
75-
if metrics != nil {
76-
p.metrics = *metrics
77-
}
78-
if p.metrics.PoolSize == nil {
79-
p.metrics.PoolSize = drpcmetrics.NoOpGauge{}
74+
func (p *Pool[K, V]) initPoolMetrics() {
75+
metrics := &p.opts.Metrics
76+
if metrics.PoolSize == nil {
77+
metrics.PoolSize = drpcmetrics.NoOpGauge{}
8078
}
81-
if p.metrics.ConnectionHitsTotal == nil {
82-
p.metrics.ConnectionHitsTotal = drpcmetrics.NoOpLabeledCounter{}
79+
if metrics.ConnectionHitsTotal == nil {
80+
metrics.ConnectionHitsTotal = drpcmetrics.NoOpLabeledCounter{}
8381
}
84-
if p.metrics.ConnectionMissesTotal == nil {
85-
p.metrics.ConnectionMissesTotal = drpcmetrics.NoOpLabeledCounter{}
82+
if metrics.ConnectionMissesTotal == nil {
83+
metrics.ConnectionMissesTotal = drpcmetrics.NoOpLabeledCounter{}
8684
}
8785
}
8886

8987
func (p *Pool[K, V]) recordHit() {
90-
p.metrics.ConnectionHitsTotal.Inc(p.opts.Labels, 1)
88+
p.opts.Metrics.ConnectionHitsTotal.Inc(p.opts.Labels, 1)
9189
}
9290

9391
func (p *Pool[K, V]) recordMiss() {
94-
p.metrics.ConnectionMissesTotal.Inc(p.opts.Labels, 1)
92+
p.opts.Metrics.ConnectionMissesTotal.Inc(p.opts.Labels, 1)
9593
}
9694

9795
func (p *Pool[K, V]) updatePoolSize() {
98-
p.metrics.PoolSize.Update(p.opts.Labels, int64(p.order.count))
96+
p.opts.Metrics.PoolSize.Update(p.opts.Labels, int64(p.order.count))
9997
}
10098

10199
func (p *Pool[K, V]) log(what string, cb func() string) {

‎drpcpool/pool_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ func TestPoolMetrics_PutTakeClose(t *testing.T) {
448448

449449
pool := New[string, Conn](Options{
450450
Capacity: 10,
451-
Metrics: &PoolMetrics{
451+
Metrics: PoolMetrics{
452452
PoolSize: poolSize,
453453
ConnectionHitsTotal: hits,
454454
ConnectionMissesTotal: misses,
@@ -501,7 +501,7 @@ func TestPoolMetrics_Eviction(t *testing.T) {
501501
pool := New[string, Conn](Options{
502502
Capacity: 1,
503503
KeyCapacity: 1,
504-
Metrics: &PoolMetrics{
504+
Metrics: PoolMetrics{
505505
PoolSize: poolSize,
506506
ConnectionMissesTotal: misses,
507507
},
@@ -529,7 +529,7 @@ func TestPoolMetrics_Eviction(t *testing.T) {
529529
func TestPoolMetrics_NilFields(t *testing.T) {
530530
// All PoolMetrics fields are nil — should not panic.
531531
pool := New[string, Conn](Options{
532-
Metrics: &PoolMetrics{},
532+
Metrics: PoolMetrics{},
533533
})
534534

535535
conn := &callbackConn{

‎drpcserver/server.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ type Options struct {
4848

4949
// Metrics holds optional metrics the server will populate. If nil, no
5050
// metrics are recorded.
51-
Metrics *ServerMetrics
51+
Metrics ServerMetrics
5252
}
5353

5454
// ServerMetrics holds optional metrics that the server will populate during
@@ -61,17 +61,16 @@ type ServerMetrics struct {
6161

6262
// addTLSHandshakeError increments the TLS handshake error counter.
6363
func (s *Server) addTLSHandshakeError() {
64-
s.metrics.TLSHandshakeErrors.Inc(1)
64+
s.opts.Metrics.TLSHandshakeErrors.Inc(1)
6565
}
6666

6767
// Server is an implementation of drpc.Server to serve drpc connections.
6868
type Server struct {
6969
opts Options
7070
handler drpc.Handler
7171

72-
mu sync.Mutex
73-
stats map[string]*drpcstats.Stats
74-
metrics ServerMetrics
72+
mu sync.Mutex
73+
stats map[string]*drpcstats.Stats
7574
}
7675

7776
// New constructs a new Server.
@@ -92,16 +91,13 @@ func NewWithOptions(handler drpc.Handler, opts Options) *Server {
9291
opts: opts,
9392
handler: handler,
9493
}
95-
9694
if s.opts.CollectStats {
95+
// TODO: (server): deprecate stats
9796
drpcopts.SetManagerStatsCB(&s.opts.Manager.Internal, s.getStats)
9897
s.stats = make(map[string]*drpcstats.Stats)
9998
}
100-
if s.opts.Metrics != nil {
101-
s.metrics = *s.opts.Metrics
102-
if s.metrics.TLSHandshakeErrors == nil {
103-
s.metrics.TLSHandshakeErrors = drpcmetrics.NoOpCounter{}
104-
}
99+
if s.opts.Metrics.TLSHandshakeErrors == nil {
100+
s.opts.Metrics.TLSHandshakeErrors = drpcmetrics.NoOpCounter{}
105101
}
106102
return s
107103
}

‎internal/integration/metrics_test.go‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (c *testCounter) count() int {
5656
//
5757

5858
func createMeteredClientConnection(
59-
t testing.TB, server DRPCServiceServer, metrics *drpcmetrics.ClientMetrics,
59+
t testing.TB, server DRPCServiceServer, metrics drpcmetrics.ClientMetrics,
6060
) (DRPCServiceClient, func()) {
6161
ctx := drpctest.NewTracker(t)
6262
c1, c2 := net.Pipe()
@@ -65,8 +65,9 @@ func createMeteredClientConnection(
6565
srv := drpcserver.New(mux)
6666
ctx.Run(func(ctx context.Context) { _ = srv.ServeOne(ctx, c1) })
6767
conn := drpcconn.NewWithOptions(c2, drpcconn.Options{
68-
Manager: drpcmanager.Options{},
69-
Metrics: metrics,
68+
Manager: drpcmanager.Options{},
69+
Metrics: metrics,
70+
CollectMetrics: true,
7071
})
7172
return NewDRPCServiceClient(conn), func() {
7273
_ = conn.Close()
@@ -84,7 +85,7 @@ func TestClientByteMetrics(t *testing.T) {
8485

8586
sent := &testCounter{}
8687
recv := &testCounter{}
87-
cli, close := createMeteredClientConnection(t, standardImpl, &drpcmetrics.ClientMetrics{
88+
cli, close := createMeteredClientConnection(t, standardImpl, drpcmetrics.ClientMetrics{
8889
BytesSent: sent,
8990
BytesRecv: recv,
9091
})
@@ -121,7 +122,7 @@ func TestClientByteMetricsPartialNil(t *testing.T) {
121122
defer ctx.Close()
122123

123124
sent := &testCounter{}
124-
cli, close := createMeteredClientConnection(t, standardImpl, &drpcmetrics.ClientMetrics{
125+
cli, close := createMeteredClientConnection(t, standardImpl, drpcmetrics.ClientMetrics{
125126
BytesSent: sent,
126127
// BytesRecv intentionally nil.
127128
})
@@ -133,13 +134,44 @@ func TestClientByteMetricsPartialNil(t *testing.T) {
133134
assert.That(t, sent.total() > 0)
134135
}
135136

137+
func TestClientByteMetricsNotCollected(t *testing.T) {
138+
ctx := drpctest.NewTracker(t)
139+
defer ctx.Close()
140+
141+
sent := &testCounter{}
142+
recv := &testCounter{}
143+
144+
c1, c2 := net.Pipe()
145+
mux := drpcmux.New()
146+
assert.NoError(t, DRPCRegisterService(mux, standardImpl))
147+
srv := drpcserver.New(mux)
148+
ctx.Run(func(ctx2 context.Context) { _ = srv.ServeOne(ctx2, c1) })
149+
conn := drpcconn.NewWithOptions(c2, drpcconn.Options{
150+
Metrics: drpcmetrics.ClientMetrics{
151+
BytesSent: sent,
152+
BytesRecv: recv,
153+
},
154+
})
155+
cli := NewDRPCServiceClient(conn)
156+
157+
out, err := cli.Method1(ctx, in(1))
158+
assert.NoError(t, err)
159+
assert.True(t, Equal(out, &Out{Out: 1}))
160+
161+
// CollectMetrics is false, so no metrics should be collected.
162+
assert.Equal(t, sent.total(), 0.0)
163+
assert.Equal(t, recv.total(), 0.0)
164+
165+
_ = conn.Close()
166+
}
167+
136168
func TestServerTLSHandshakeErrorMetric(t *testing.T) {
137169
tlsErrors := &testCounter{}
138170

139171
mux := drpcmux.New()
140172
assert.NoError(t, DRPCRegisterService(mux, standardImpl))
141173
srv := drpcserver.NewWithOptions(mux, drpcserver.Options{
142-
Metrics: &drpcserver.ServerMetrics{
174+
Metrics: drpcserver.ServerMetrics{
143175
TLSHandshakeErrors: tlsErrors,
144176
},
145177
})

0 commit comments

Comments
 (0)