Skip to content

Commit ddabde5

Browse files
author
Larry Li
committed
address PR comments
1 parent 3bc3510 commit ddabde5

4 files changed

Lines changed: 27 additions & 53 deletions

File tree

‎metrics/client.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,29 @@ var (
3434
const rpcCallLatencyBeholder = "rpc_call_latency"
3535

3636
// RPCClientMetrics records RPC call latency to Prometheus and Beholder (failures: success="false"; same pattern as multinode metrics).
37-
// Construct once per chain (or process) with ChainFamily and ChainID; pass rpcUrl and isSendOnly on each call
38-
// when they vary by node or request.
37+
// Construct once per client with the full stable label set and record requests by call name.
3938
type RPCClientMetrics interface {
4039
// RecordRequest records latency for an RPC call (observed in nanoseconds for Prometheus and Beholder).
4140
// Failures use success="false"; derive error rate from rpc_call_latency_count{success="false"} (or equivalent).
42-
RecordRequest(ctx context.Context, rpcURL string, isSendOnly bool, callName string, latency time.Duration, err error)
41+
RecordRequest(ctx context.Context, callName string, latency time.Duration, err error)
4342
}
4443

4544
var _ RPCClientMetrics = (*rpcClientMetrics)(nil)
4645

4746
type rpcClientMetrics struct {
4847
chainFamily string
4948
chainID string
49+
rpcURL string
50+
isSendOnly bool
5051
latencyHis metric.Float64Histogram
5152
}
5253

53-
// RPCClientMetricsConfig holds labels that are fixed for the lifetime of the metrics handle (e.g. one per chain).
54+
// RPCClientMetricsConfig holds labels that are fixed for the lifetime of the metrics handle (e.g. one per client).
5455
type RPCClientMetricsConfig struct {
5556
ChainFamily string
5657
ChainID string
58+
RPCURL string
59+
IsSendOnly bool
5760
}
5861

5962
// NewRPCClientMetrics creates RPC client metrics that publish to Prometheus and Beholder.
@@ -65,24 +68,26 @@ func NewRPCClientMetrics(cfg RPCClientMetricsConfig) (RPCClientMetrics, error) {
6568
return &rpcClientMetrics{
6669
chainFamily: cfg.ChainFamily,
6770
chainID: cfg.ChainID,
71+
rpcURL: cfg.RPCURL,
72+
isSendOnly: cfg.IsSendOnly,
6873
latencyHis: latency,
6974
}, nil
7075
}
7176

72-
func (m *rpcClientMetrics) RecordRequest(ctx context.Context, rpcURL string, isSendOnly bool, callName string, latency time.Duration, err error) {
77+
func (m *rpcClientMetrics) RecordRequest(ctx context.Context, callName string, latency time.Duration, err error) {
7378
successStr := "true"
7479
if err != nil {
7580
successStr = "false"
7681
}
77-
sendStr := strconv.FormatBool(isSendOnly)
82+
sendStr := strconv.FormatBool(m.isSendOnly)
7883
latencyNs := float64(latency)
7984

80-
RPCCallLatency.WithLabelValues(m.chainFamily, m.chainID, rpcURL, sendStr, successStr, callName).Observe(latencyNs)
85+
RPCCallLatency.WithLabelValues(m.chainFamily, m.chainID, m.rpcURL, sendStr, successStr, callName).Observe(latencyNs)
8186

8287
latAttrs := metric.WithAttributes(
8388
attribute.String("chainFamily", m.chainFamily),
8489
attribute.String("chainID", m.chainID),
85-
attribute.String("rpcUrl", rpcURL),
90+
attribute.String("rpcUrl", m.rpcURL),
8691
attribute.String("isSendOnly", sendStr),
8792
attribute.String("success", successStr),
8893
attribute.String("rpcCallName", callName),
@@ -93,7 +98,7 @@ func (m *rpcClientMetrics) RecordRequest(ctx context.Context, rpcURL string, isS
9398
// NoopRPCClientMetrics is a no-op implementation for when metrics are disabled.
9499
type NoopRPCClientMetrics struct{}
95100

96-
func (NoopRPCClientMetrics) RecordRequest(context.Context, string, bool, string, time.Duration, error) {
101+
func (NoopRPCClientMetrics) RecordRequest(context.Context, string, time.Duration, error) {
97102
}
98103

99104
var _ RPCClientMetrics = NoopRPCClientMetrics{}

‎metrics/client_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,20 @@ func TestNewRPCClientMetrics(t *testing.T) {
1313
m, err := NewRPCClientMetrics(RPCClientMetricsConfig{
1414
ChainFamily: "evm",
1515
ChainID: "1",
16+
RPCURL: "http://localhost:8545",
17+
IsSendOnly: false,
1618
})
1719
require.NoError(t, err)
1820
require.NotNil(t, m)
1921

2022
ctx := context.Background()
21-
const url = "http://localhost:8545"
22-
m.RecordRequest(ctx, url, false, "latest_block", 100*time.Millisecond, nil)
23-
m.RecordRequest(ctx, url, true, "latest_block", 50*time.Millisecond, errors.New("rpc error"))
23+
m.RecordRequest(ctx, "latest_block", 100*time.Millisecond, nil)
24+
m.RecordRequest(ctx, "latest_block", 50*time.Millisecond, errors.New("rpc error"))
2425
}
2526

2627
func TestNoopRPCClientMetrics_RecordRequest(t *testing.T) {
2728
var m NoopRPCClientMetrics
2829
ctx := context.Background()
29-
m.RecordRequest(ctx, "http://localhost:8545", false, "latest_block", 100*time.Millisecond, nil)
30-
m.RecordRequest(ctx, "http://localhost:8545", false, "latest_block", 50*time.Millisecond, errors.New("rpc error"))
30+
m.RecordRequest(ctx, "latest_block", 100*time.Millisecond, nil)
31+
m.RecordRequest(ctx, "latest_block", 50*time.Millisecond, errors.New("rpc error"))
3132
}

‎multinode/rpc_client_base.go‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,6 @@ type RPCClientBaseConfig interface {
2424
FinalizedBlockPollInterval() time.Duration
2525
}
2626

27-
type RPCClientBaseMetricsConfig struct {
28-
RPCClientMetrics frameworkmetrics.RPCClientMetrics
29-
RPCURL string
30-
IsSendOnly bool
31-
}
32-
3327
// RPCClientBase is used to integrate multinode into chain-specific clients.
3428
// For new MultiNode integrations, we wrap the RPC client and inherit from the RPCClientBase
3529
// to get the required RPCClient methods and enable the use of MultiNode.
@@ -62,15 +56,13 @@ type RPCClientBase[HEAD Head] struct {
6256
latestChainInfo ChainInfo
6357

6458
rpcMetrics frameworkmetrics.RPCClientMetrics
65-
rpcURL string
66-
isSendOnly bool
6759
}
6860

6961
func NewRPCClientBase[HEAD Head](
7062
cfg RPCClientBaseConfig, ctxTimeout time.Duration, log logger.Logger,
7163
latestBlock func(ctx context.Context) (HEAD, error),
7264
latestFinalizedBlock func(ctx context.Context) (HEAD, error),
73-
rpcMetrics *RPCClientBaseMetricsConfig,
65+
rpcMetrics frameworkmetrics.RPCClientMetrics,
7466
) *RPCClientBase[HEAD] {
7567
base := &RPCClientBase[HEAD]{
7668
cfg: cfg,
@@ -80,11 +72,7 @@ func NewRPCClientBase[HEAD Head](
8072
latestFinalizedBlock: latestFinalizedBlock,
8173
subs: make(map[Subscription]struct{}),
8274
lifeCycleCh: make(chan struct{}),
83-
}
84-
if rpcMetrics != nil {
85-
base.rpcMetrics = rpcMetrics.RPCClientMetrics
86-
base.rpcURL = rpcMetrics.RPCURL
87-
base.isSendOnly = rpcMetrics.IsSendOnly
75+
rpcMetrics: rpcMetrics,
8876
}
8977
return base
9078
}
@@ -224,7 +212,7 @@ func (m *RPCClientBase[HEAD]) recordRPCRequest(ctx context.Context, callName str
224212
return
225213
}
226214

227-
m.rpcMetrics.RecordRequest(ctx, m.rpcURL, m.isSendOnly, callName, time.Since(startedAt), err)
215+
m.rpcMetrics.RecordRequest(ctx, callName, time.Since(startedAt), err)
228216
}
229217

230218
func (m *RPCClientBase[HEAD]) OnNewHead(ctx context.Context, requestCh <-chan struct{}, head HEAD) {

‎multinode/rpc_client_base_test.go‎

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,6 @@ func newTestRPC(t *testing.T) *testRPC {
7474
}
7575

7676
type recordedRPCRequest struct {
77-
rpcURL string
78-
isSendOnly bool
7977
callName string
8078
latency time.Duration
8179
err error
@@ -87,10 +85,8 @@ type spyRPCClientMetrics struct {
8785

8886
var _ frameworkmetrics.RPCClientMetrics = (*spyRPCClientMetrics)(nil)
8987

90-
func (s *spyRPCClientMetrics) RecordRequest(_ context.Context, rpcURL string, isSendOnly bool, callName string, latency time.Duration, err error) {
88+
func (s *spyRPCClientMetrics) RecordRequest(_ context.Context, callName string, latency time.Duration, err error) {
9189
s.requests = append(s.requests, recordedRPCRequest{
92-
rpcURL: rpcURL,
93-
isSendOnly: isSendOnly,
9490
callName: callName,
9591
latency: latency,
9692
err: err,
@@ -161,19 +157,13 @@ func TestRPCClientBase_RecordsRPCMetrics(t *testing.T) {
161157
func(context.Context) (*testHead, error) {
162158
return &testHead{blockNumber: 8}, nil
163159
},
164-
&RPCClientBaseMetricsConfig{
165-
RPCClientMetrics: spy,
166-
RPCURL: "http://primary.test",
167-
IsSendOnly: false,
168-
},
160+
spy,
169161
)
170162

171163
head, err := rpc.LatestBlock(t.Context())
172164
require.NoError(t, err)
173165
require.Equal(t, int64(7), head.BlockNumber())
174166
require.Len(t, spy.requests, 1)
175-
require.Equal(t, "http://primary.test", spy.requests[0].rpcURL)
176-
require.False(t, spy.requests[0].isSendOnly)
177167
require.Equal(t, rpcCallNameLatestBlock, spy.requests[0].callName)
178168
require.NoError(t, spy.requests[0].err)
179169
require.Positive(t, spy.requests[0].latency)
@@ -192,18 +182,12 @@ func TestRPCClientBase_RecordsRPCMetrics(t *testing.T) {
192182
func(context.Context) (*testHead, error) {
193183
return nil, expectedErr
194184
},
195-
&RPCClientBaseMetricsConfig{
196-
RPCClientMetrics: spy,
197-
RPCURL: "http://sendonly.test",
198-
IsSendOnly: true,
199-
},
185+
spy,
200186
)
201187

202188
_, err := rpc.LatestFinalizedBlock(t.Context())
203189
require.ErrorIs(t, err, expectedErr)
204190
require.Len(t, spy.requests, 1)
205-
require.Equal(t, "http://sendonly.test", spy.requests[0].rpcURL)
206-
require.True(t, spy.requests[0].isSendOnly)
207191
require.Equal(t, rpcCallNameLatestFinalizedBlock, spy.requests[0].callName)
208192
require.ErrorIs(t, spy.requests[0].err, expectedErr)
209193
require.Positive(t, spy.requests[0].latency)
@@ -221,11 +205,7 @@ func TestRPCClientBase_RecordsRPCMetrics(t *testing.T) {
221205
func(context.Context) (*testHead, error) {
222206
return &testHead{blockNumber: 8}, nil
223207
},
224-
&RPCClientBaseMetricsConfig{
225-
RPCClientMetrics: spy,
226-
RPCURL: "http://invalid.test",
227-
IsSendOnly: false,
228-
},
208+
spy,
229209
)
230210

231211
_, err := rpc.LatestBlock(t.Context())

0 commit comments

Comments
 (0)