separated rpc call metrics into a collector (#20)

* separated rpc call metrics into a collector to remove circular dependencies and make it cleaner

* fix eigen metrics example that was breaking lint

* removed unneeded methods from noop metrics

* added TODO comment in clients constructor

* updated constructor comment following madhur's advise

chainio/constructor/constructor.go

@@ -28,6 +28,11 @@ type Config struct {
 	PromMetricsIpPortAddress      string `yaml:"prometheus_metrics_ip_port_address"`
 }
 
+// TODO: this is confusing right now because clients are not instrumented clients, but
+// we return metrics and prometheus reg, so user has to build instrumented clients at the call
+// site if they need them. We should probably separate into two separate constructors, one
+// for non-instrumented clients that doesn't return metrics/reg, and another instrumented-constructor
+// that returns instrumented clients and the metrics/reg.
 type Clients struct {
 	AvsRegistryChainReader avsregistry.AvsRegistryReader
 	AvsRegistryChainWriter avsregistry.AvsRegistryWriter

metrics/collectors/economic.go → metrics/collectors/economic/economic.go

@@ -1,5 +1,5 @@
 // Package collectors contains custom prometheus collectors that are not just simple instrumented metrics
-package collectors
+package economic
 
 import (
 	"context"
@@ -13,15 +13,15 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-// EconomicCollector to export the economic metrics listed at
+// Collector exports the economic metrics listed at
 //
 //	https://eigen.nethermind.io/docs/spec/metrics/metrics-examples#economics-metrics
 //
 // these are metrics that are exported not via instrumentation, but instead by proxying
 // a call to the relevant eigenlayer contracts
-// EconomicCollector should be registered in the same prometheus registry that is passed to metrics.NewEigenMetrics
+// Collector should be registered in the same prometheus registry that is passed to metrics.NewEigenMetrics
 // so that they are exported on the same port
-type EconomicCollector struct {
+type Collector struct {
 	// TODO(samlaf): we use a chain as the backend for now, but should eventually move to a subgraph
 	elReader          elcontracts.ELReader
 	avsRegistryReader avsregistry.AvsRegistryReader
@@ -48,7 +48,7 @@ type EconomicCollector struct {
 	// then the operator's registeredStake will remain 600 until updateStakes() is called, at which point it will
 	// drop to the correct value of 300.
 	registeredStake *prometheus.Desc
-	
+
 	// TODO(samlaf): Removing this as not part of avs node spec anymore.
 	// delegatedShares is the total shares delegated to the operator in each strategy
 	// strategies are currently token specific, and we have one for cbETH, rETH, and stETH,
@@ -57,18 +57,18 @@ type EconomicCollector struct {
 	// delegatedShares *prometheus.Desc
 }
 
-var _ prometheus.Collector = (*EconomicCollector)(nil)
+var _ prometheus.Collector = (*Collector)(nil)
 
-func NewEconomicCollector(
+func NewCollector(
 	elReader elcontracts.ELReader, avsRegistryReader avsregistry.AvsRegistryReader,
 	avsName string, logger logging.Logger,
 	operatorAddr common.Address, quorumNames map[types.QuorumNum]string,
-) *EconomicCollector {
+) *Collector {
 	operatorId, err := avsRegistryReader.GetOperatorId(context.Background(), operatorAddr)
 	if err != nil {
 		logger.Error("Failed to get operator id", "err", err)
 	}
-	return &EconomicCollector{
+	return &Collector{
 		elReader:          elReader,
 		avsRegistryReader: avsRegistryReader,
 		logger:            logger,
@@ -104,7 +104,7 @@ func NewEconomicCollector(
 // Describe is implemented with DescribeByCollect. That's possible because the
 // Collect method will always return the same two metrics with the same two
 // descriptors.
-func (ec *EconomicCollector) Describe(ch chan<- *prometheus.Desc) {
+func (ec *Collector) Describe(ch chan<- *prometheus.Desc) {
 	ch <- ec.slashingStatus
 	ch <- ec.registeredStake
 	// ch <- ec.delegatedShares
@@ -114,7 +114,7 @@ func (ec *EconomicCollector) Describe(ch chan<- *prometheus.Desc) {
 // see https://github.com/prometheus/client_golang/blob/v1.16.0/prometheus/collector.go#L27
 // collect just makes jsonrpc calls to the slasher and delegationManager and then creates
 // constant metrics with the results
-func (ec *EconomicCollector) Collect(ch chan<- prometheus.Metric) {
+func (ec *Collector) Collect(ch chan<- prometheus.Metric) {
 	// collect slashingStatus metric
 	// TODO(samlaf): note that this call is not avs specific, so every avs will have the same value if the operator has been slashed
 	// if we want instead to only output 1 if the operator has been slashed for a specific avs, we have 2 choices:

metrics/collectors/economic_test.go → metrics/collectors/economic/economic_test.go

@@ -1,4 +1,4 @@
-package collectors
+package economic
 
 import (
 	"math/big"
@@ -53,7 +53,7 @@ func TestEconomicCollector(t *testing.T) {
 	avsRegistryReader.EXPECT().GetOperatorId(gomock.Any(), operatorAddr).Return(operatorId, nil)
 
 	logger := logging.NewNoopLogger()
-	economicCollector := NewEconomicCollector(ethReader, avsRegistryReader, "testavs", logger, operatorAddr, quorumNames)
+	economicCollector := NewCollector(ethReader, avsRegistryReader, "testavs", logger, operatorAddr, quorumNames)
 
 	count := testutil.CollectAndCount(economicCollector, "eigen_slashing_status", "eigen_registered_stakes")
 	// 1 for eigen_slashing_status, and 2 for eigen_registered_stakes (1 per quorum)

metrics/collectors/rpc_calls/rpc_calls.go

@@ -0,0 +1,58 @@
+package rpccalls
+
+import (
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promauto"
+)
+
+// Collector contains instrumented metrics that should be incremented by the avs node using the methods below
+// it is used by the eigensdk's instrumented_client, but can also be used by avs teams to instrument their own client
+// if it differs from ours.
+type Collector struct {
+	rpcRequestDurationSeconds *prometheus.HistogramVec
+	rpcRequestTotal           *prometheus.CounterVec
+}
+
+// NewCollector returns an rpccalls Collector that collects metrics for json-rpc calls
+func NewCollector(eigenNamespace, avsName string, reg prometheus.Registerer) *Collector {
+	return &Collector{
+		rpcRequestDurationSeconds: promauto.With(reg).NewHistogramVec(
+			prometheus.HistogramOpts{
+				Namespace:   eigenNamespace,
+				Name:        "rpc_request_duration_seconds",
+				Help:        "Duration of json-rpc <method> in seconds",
+				ConstLabels: prometheus.Labels{"avs_name": avsName},
+			},
+			// client_version is the client name and its current version. We don't separate them because
+			// this string is returned as a single string by the web3_clientVersion jsonrpc call
+			// which doesn't follow any standardized format so not possible to parse it...
+			// https://ethereum.org/en/developers/docs/apis/json-rpc/#web3_clientversion
+			[]string{"method", "client_version"},
+		),
+		rpcRequestTotal: promauto.With(reg).NewCounterVec(
+			prometheus.CounterOpts{
+				Namespace:   eigenNamespace,
+				Name:        "rpc_request_total",
+				Help:        "Total number of json-rpc <method> requests",
+				ConstLabels: prometheus.Labels{"avs_name": avsName},
+			},
+			[]string{"method", "client_version"},
+		),
+	}
+}
+
+// ObserveRPCRequestDurationSeconds observes the duration of a json-rpc request
+func (c *Collector) ObserveRPCRequestDurationSeconds(duration float64, method, clientVersion string) {
+	c.rpcRequestDurationSeconds.With(prometheus.Labels{
+		"method":         method,
+		"client_version": clientVersion,
+	}).Observe(duration)
+}
+
+// AddRPCRequestTotal adds a json-rpc request to the total number of requests
+func (c *Collector) AddRPCRequestTotal(method, clientVersion string) {
+	c.rpcRequestTotal.With(prometheus.Labels{
+		"method":         method,
+		"client_version": clientVersion,
+	}).Inc()
+}

metrics/collectors/rpc_calls/rpc_calls_test.go

@@ -0,0 +1,22 @@
+package rpccalls
+
+import (
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRpcCallsCollector(t *testing.T) {
+	reg := prometheus.NewRegistry()
+	rpcCallsCollector := NewCollector("testavs", "testname", reg)
+
+	rpcCallsCollector.ObserveRPCRequestDurationSeconds(1, "testmethod", "testclient/testversion")
+	// TODO(samlaf): not sure how to test histogram values.. there's no mention of histograms in
+	//               https://pkg.go.dev/github.com/prometheus/client_golang/prometheus/testutil
+	// assert.Equal(t, 1.0, testutil.ToFloat64(suite.metrics.rpcRequestDurationSeconds))
+
+	rpcCallsCollector.AddRPCRequestTotal("testmethod", "testclient/testversion")
+	assert.Equal(t, 1.0, testutil.ToFloat64(rpcCallsCollector.rpcRequestTotal.WithLabelValues("testmethod", "testclient/testversion")))
+}

metrics/eigenmetrics.go

@@ -20,8 +20,6 @@ type EigenMetrics struct {
 	// fees are not yet turned on, so these should just be 0 for the time being
 	feeEarnedTotal            *prometheus.CounterVec
 	performanceScore          prometheus.Gauge
-	rpcRequestDurationSeconds *prometheus.HistogramVec
-	rpcRequestTotal           *prometheus.CounterVec
 }
 
 var _ Metrics = (*EigenMetrics)(nil)
@@ -51,24 +49,6 @@ func NewEigenMetrics(avsName, ipPortAddress string, reg prometheus.Registerer, l
 				ConstLabels: prometheus.Labels{"avs_name": avsName},
 			},
 		),
-		rpcRequestDurationSeconds: promauto.With(reg).NewHistogramVec(
-			prometheus.HistogramOpts{
-				Namespace:   eigenNamespace,
-				Name:        "rpc_request_duration_seconds",
-				Help:        "Duration of json-rpc <method> in seconds",
-				ConstLabels: prometheus.Labels{"avs_name": avsName},
-			},
-			[]string{"method", "client", "version"},
-		),
-		rpcRequestTotal: promauto.With(reg).NewCounterVec(
-			prometheus.CounterOpts{
-				Namespace:   eigenNamespace,
-				Name:        "rpc_request_total",
-				Help:        "Total number of json-rpc <method> requests",
-				ConstLabels: prometheus.Labels{"avs_name": avsName},
-			},
-			[]string{"method", "client", "version"},
-		),
 		ipPortAddress: ipPortAddress,
 		logger:        logger,
 	}
@@ -95,38 +75,6 @@ func (m *EigenMetrics) SetPerformanceScore(score float64) {
 	m.performanceScore.Set(score)
 }
 
-// ObserveRPCRequestDurationSeconds observes the duration of a json-rpc request
-func (m *EigenMetrics) ObserveRPCRequestDurationSeconds(duration float64, method, client, version string) {
-	// TODO(samlaf): client and version are separate because we're following the current avs-node-spec
-	// https://eigen.nethermind.io/docs/metrics/metrics-prom-spec
-	// but the web3_clientVersion jsonrpc call returns a single string that has both
-	// in a non-standardized format so not possible to parse...
-	// https://ethereum.org/en/developers/docs/apis/json-rpc/#web3_clientversion
-	// for now we'll just duplicate the clientVersion string in both client and version,
-	// but we should eventually have a single clientVersion label
-	m.rpcRequestDurationSeconds.With(prometheus.Labels{
-		"method":  method,
-		"client":  client,
-		"version": version,
-	}).Observe(duration)
-}
-
-// AddRPCRequestTotal adds a json-rpc request to the total number of requests
-func (m *EigenMetrics) AddRPCRequestTotal(method, client, version string) {
-	// TODO(samlaf): client and version are separate because we're following the current avs-node-spec
-	// https://eigen.nethermind.io/docs/metrics/metrics-prom-spec
-	// but the web3_clientVersion jsonrpc call returns a single string that has both
-	// in a non-standardized format so not possible to parse...
-	// https://ethereum.org/en/developers/docs/apis/json-rpc/#web3_clientversion
-	// for now we'll just duplicate the clientVersion string in both client and version,
-	// but we should eventually have a single clientVersion label
-	m.rpcRequestTotal.With(prometheus.Labels{
-		"method":  method,
-		"client":  client,
-		"version": version,
-	}).Inc()
-}
-
 // Start creates an http handler for reg and starts the prometheus server in a goroutine, listening at m.ipPortAddress.
 // reg needs to be the prometheus registry that was passed in the NewEigenMetrics constructor
 func (m *EigenMetrics) Start(ctx context.Context, reg prometheus.Gatherer) <-chan error {

metrics/eigenmetrics_example_test.go

@@ -9,11 +9,12 @@ import (
 
 	"github.com/Layr-Labs/eigensdk-go/chainio/avsregistry"
 	sdkclients "github.com/Layr-Labs/eigensdk-go/chainio/clients"
-	ethclients "github.com/Layr-Labs/eigensdk-go/chainio/clients/eth"
+	"github.com/Layr-Labs/eigensdk-go/chainio/clients/eth"
 	"github.com/Layr-Labs/eigensdk-go/chainio/elcontracts"
 	"github.com/Layr-Labs/eigensdk-go/logging"
 	"github.com/Layr-Labs/eigensdk-go/metrics"
-	"github.com/Layr-Labs/eigensdk-go/metrics/collectors"
+	"github.com/Layr-Labs/eigensdk-go/metrics/collectors/economic"
+	rpccalls "github.com/Layr-Labs/eigensdk-go/metrics/collectors/rpc_calls"
 	"github.com/Layr-Labs/eigensdk-go/types"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/prometheus/client_golang/prometheus"
@@ -35,11 +36,11 @@ func ExampleEigenMetrics() {
 
 	slasherAddr := common.HexToAddress("0x0")
 	blsPubKeyCompendiumAddr := common.HexToAddress("0x0")
-	ethHttpClient, err := ethclients.NewClient("http://localhost:8545")
+	ethHttpClient, err := eth.NewClient("http://localhost:8545")
 	if err != nil {
 		panic(err)
 	}
-	ethWsClient, err := ethclients.NewClient("ws://localhost:8545")
+	ethWsClient, err := eth.NewClient("ws://localhost:8545")
 	if err != nil {
 		panic(err)
 	}
@@ -74,8 +75,17 @@ func ExampleEigenMetrics() {
 	}
 	// We must register the economic metrics separately because they are exported metrics (from jsonrpc or subgraph calls)
 	// and not instrumented metrics: see https://prometheus.io/docs/instrumenting/writing_clientlibs/#overall-structure
-	economicMetricsCollector := collectors.NewEconomicCollector(eigenlayerReader, avsRegistryReader, "exampleAvs", logger, operatorAddr, quorumNames)
+	economicMetricsCollector := economic.NewCollector(eigenlayerReader, avsRegistryReader, "exampleAvs", logger, operatorAddr, quorumNames)
 	reg.MustRegister(economicMetricsCollector)
 
+	rpcCallsCollector := rpccalls.NewCollector("eigen", "exampleAvs", reg)
+	instrumentedEthClient, err := eth.NewInstrumentedClient("http://localhost:8545", rpcCallsCollector)
+	if err != nil {
+		panic(err)
+	}
+
 	eigenMetrics.Start(context.Background(), reg)
+
+	// use instrumentedEthClient as you would a normal ethClient
+	_ = instrumentedEthClient
 }

metrics/eigenmetrics_test.go

@@ -44,8 +44,6 @@ func (suite *MetricsTestSuite) TestEigenMetricsServerIntegration() {
 
 	suite.metrics.AddFeeEarnedTotal(1, "testtoken")
 	suite.metrics.SetPerformanceScore(1)
-	suite.metrics.ObserveRPCRequestDurationSeconds(1, "testmethod", "testclient", "testversion")
-	suite.metrics.AddRPCRequestTotal("testmethod", "testclient", "testversion")
 
 	resp, err := http.Get("http://localhost:9090/metrics")
 	assert.NoError(suite.T(), err)
@@ -57,8 +55,6 @@ func (suite *MetricsTestSuite) TestEigenMetricsServerIntegration() {
 	// the other metrics have labels (they are vectors) so they don't appear in the output unless we use them once at least
 	assert.Contains(suite.T(), string(body), "eigen_fees_earned_total")
 	assert.Contains(suite.T(), string(body), "eigen_performance_score")
-	assert.Contains(suite.T(), string(body), "eigen_rpc_request_duration_seconds")
-	assert.Contains(suite.T(), string(body), "eigen_rpc_request_total")
 }
 
 // The below tests are very pedantic but at least show how avs teams can make use of
@@ -75,18 +71,6 @@ func (suite *MetricsTestSuite) TestSetEigenPerformanceScore() {
 	assert.Equal(suite.T(), 1.0, testutil.ToFloat64(suite.metrics.performanceScore))
 }
 
-func (suite *MetricsTestSuite) TestObserveEigenRPCRequestDurationSeconds() {
-	suite.metrics.ObserveRPCRequestDurationSeconds(1, "testmethod", "testclient", "testversion")
-	// TODO(samlaf): not sure how to test histogram values.. there's no mention of histograms in
-	//               https://pkg.go.dev/github.com/prometheus/client_golang/prometheus/testutil
-	// assert.Equal(t, 1.0, testutil.ToFloat64(suite.metrics.rpcRequestDurationSeconds))
-}
-
-func (suite *MetricsTestSuite) TestAddEigenRPCRequestTotal() {
-	suite.metrics.AddRPCRequestTotal("testmethod", "testclient", "testversion")
-	assert.Equal(suite.T(), 1.0, testutil.ToFloat64(suite.metrics.rpcRequestTotal.WithLabelValues("testmethod", "testclient", "testversion")))
-}
-
 // In order for 'go test' to run this suite, we need to create
 // a normal test function and pass our suite to suite.Run
 func TestMetricsTestSuite(t *testing.T) {

metrics/metrics.go

@@ -6,10 +6,12 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
+// Metrics is the interface for the EigenMetrics server
+// it only wraps 2 of the 6 methods required by the spec (https://eigen.nethermind.io/docs/spec/metrics/metrics-prom-spec)
+// the others are implemented by the economics and rpc_calls collectors,
+// and need to be registered with the metrics server prometheus registry
 type Metrics interface {
 	AddFeeEarnedTotal(amount float64, token string)
 	SetPerformanceScore(score float64)
-	ObserveRPCRequestDurationSeconds(duration float64, method, client, version string)
-	AddRPCRequestTotal(method, client, version string)
 	Start(ctx context.Context, reg prometheus.Gatherer) <-chan error
 }

metrics/noop_metrics.go

@@ -17,9 +17,6 @@ func NewNoopMetrics() *NoopMetrics {
 
 func (m *NoopMetrics) AddFeeEarnedTotal(amount float64, token string) {}
 func (m *NoopMetrics) SetPerformanceScore(score float64)              {}
-func (m *NoopMetrics) ObserveRPCRequestDurationSeconds(duration float64, method, client, version string) {
-}
-func (m *NoopMetrics) AddRPCRequestTotal(method, client, version string) {}
 func (m *NoopMetrics) Start(ctx context.Context, reg prometheus.Gatherer) <-chan error {
 	return nil
 }