Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add telemetry for beacon and eth version #2176

Merged
merged 7 commits into from
Nov 29, 2024
Merged

Add telemetry for beacon and eth version #2176

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64a27ae
Add telemetry for beacon and eth version
fridrik01 Nov 24, 2024
7259475
Multiple improvements to the reporting service
fridrik01 Nov 25, 2024
b7a3fcc
Merge branch 'main' into add-telemetry-for-beacon-and-eth-version
calbera Nov 28, 2024
7443f52
Merge branch 'main' into add-telemetry-for-beacon-and-eth-version
calbera Nov 28, 2024
c34e582
Address review comments
fridrik01 Nov 29, 2024
7c99de1
Add HasCapability wrapper
fridrik01 Nov 29, 2024
a35ad38
Merge branch 'main' into add-telemetry-for-beacon-and-eth-version
calbera Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion beacond/cmd/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ func DefaultComponents() []any {
*BeaconBlockHeader, *BeaconState, *BeaconStateMarshallable,
*ExecutionPayload, *ExecutionPayloadHeader, *KVStore, *Logger,
],
components.ProvideReportingService[*Logger],
components.ProvideReportingService[
*ExecutionPayload, *PayloadAttributes, *Logger,
],
components.ProvideCometBFTService[*Logger],
components.ProvideServiceRegistry[
*AvailabilityStore,
Expand Down
5 changes: 4 additions & 1 deletion beacond/cmd/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,10 @@ type (
NodeAPIServer = server.Server[NodeAPIContext]

// ReportingService is a type alias for the reporting service.
ReportingService = version.ReportingService
ReportingService = version.ReportingService[
*ExecutionPayload,
*PayloadAttributes,
]
fridrik01 marked this conversation as resolved.
Show resolved Hide resolved

// SidecarFactory is a type alias for the sidecar factory.
SidecarFactory = dablob.SidecarFactory[
Expand Down
21 changes: 18 additions & 3 deletions mod/execution/pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"context"
"math/big"
"strings"
"sync"
"time"

"github.com/berachain/beacon-kit/mod/errors"
Expand All @@ -49,8 +50,12 @@ type EngineClient[
eth1ChainID *big.Int
// clientMetrics is the metrics for the engine client.
metrics *clientMetrics
// capabilities is a map of capabilities that the execution client has.
capabilities map[string]struct{}
// Capabilities is a map of capabilities that the execution client has.
Capabilities map[string]struct{}
fridrik01 marked this conversation as resolved.
Show resolved Hide resolved
// connected will be set to true when we have successfully connected
// to the execution client.
mutex sync.RWMutex
fridrik01 marked this conversation as resolved.
Show resolved Hide resolved
connected bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider improving field organization and documentation.

  1. The field documentation could be more comprehensive:
-// Capabilities is a map of capabilities that the execution client has.
+// Capabilities is a map of capabilities that the execution client has.
+// This map is populated during connection initialization and used for feature detection
+// and telemetry reporting. It is made public to allow external components to query
+// supported features.

-// connected will be set to true when we have successfully connected
-// to the execution client.
+// mutex protects concurrent access to the connected field
+// connected indicates whether we have successfully established and verified
+// a connection to the execution client, including chain ID verification
+// and capability exchange.
  1. Consider grouping related fields together:
type EngineClient[...] struct {
    *ethclient.Client[ExecutionPayloadT]
    cfg         *Config
    logger      log.Logger
    eth1ChainID *big.Int
    metrics     *clientMetrics
+   // Connection state
+   mutex       sync.RWMutex
+   connected   bool
    // Client capabilities
    Capabilities map[string]struct{}
}

Committable suggestion skipped: line range outside the PR's diff.

}

// New creates a new engine client EngineClient.
Expand Down Expand Up @@ -79,9 +84,10 @@ func New[
cfg.RPCJWTRefreshInterval,
),
)),
capabilities: make(map[string]struct{}),
Capabilities: make(map[string]struct{}),
eth1ChainID: eth1ChainID,
metrics: newClientMetrics(telemetrySink, logger),
connected: false,
}
}

Expand Down Expand Up @@ -130,11 +136,20 @@ func (s *EngineClient[
}
continue
}
s.mutex.Lock()
s.connected = true
s.mutex.Unlock()
return nil
}
}
}

func (s *EngineClient[_, _]) IsConnected() bool {
s.mutex.RLock()
defer s.mutex.RUnlock()
return s.connected
}

/* -------------------------------------------------------------------------- */
/* Helpers */
/* -------------------------------------------------------------------------- */
Expand Down
4 changes: 2 additions & 2 deletions mod/execution/pkg/client/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,12 @@ func (s *EngineClient[
// Capture and log the capabilities that the execution client has.
for _, capability := range result {
s.logger.Info("Exchanged capability", "capability", capability)
s.capabilities[capability] = struct{}{}
s.Capabilities[capability] = struct{}{}
fridrik01 marked this conversation as resolved.
Show resolved Hide resolved
}

// Log the capabilities that the execution client does not have.
for _, capability := range ethclient.BeaconKitSupportedCapabilities() {
if _, exists := s.capabilities[capability]; !exists {
if _, exists := s.Capabilities[capability]; !exists {
s.logger.Warn(
"Your execution client may require an update 🚸",
"unsupported_capability", capability,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance version telemetry during capabilities exchange.

Since this PR aims to create a Grafana dashboard for version tracking, consider capturing the execution client version information during capabilities exchange.

 // Capture and log the capabilities that the execution client has.
 for _, capability := range result {
     s.logger.Info("Exchanged capability", "capability", capability)
     s.Capabilities[capability] = struct{}{}
+    // Extract and set version information if the capability contains version details
+    if version := extractVersionFromCapability(capability); version != "" {
+        s.metrics.SetGauge("execution_client_version", 1, map[string]string{
+            "version": version,
+            "client": s.Client.Name(),
+        })
+    }
 }

This enhancement will help populate the Grafana dashboard with version information from connected execution clients.

Committable suggestion skipped: line range outside the PR's diff.

Expand Down
7 changes: 6 additions & 1 deletion mod/execution/pkg/client/ethclient/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
"github.com/berachain/beacon-kit/mod/primitives/pkg/eip4844"
"github.com/berachain/beacon-kit/mod/primitives/pkg/version"
"github.com/ethereum/go-ethereum/beacon/engine"
)

/* -------------------------------------------------------------------------- */
Expand Down Expand Up @@ -180,8 +181,12 @@ func (s *Client[ExecutionPayloadT]) GetClientVersionV1(
ctx context.Context,
) ([]engineprimitives.ClientVersionV1, error) {
result := make([]engineprimitives.ClientVersionV1, 0)

// NOTE: although the ethereum spec does not require us passing a
// clientversion as param, it seems some clients require it and even
// enforce a valid Code.
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fix comment style and typo

The comment should:

  1. Follow Go comment style (start with the function/variable name)
  2. Fix the typo "enforce"
-	// NOTE: although the ethereum spec does not require us passing a
-	// clientversion as param, it seems some clients require it and even
-	// enforce a valid Code.
+	// GetClientVersionV1: Although the Ethereum spec does not require passing a
+	// client version as param, some clients require it and even enforce
+	// a valid Code.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// NOTE: although the ethereum spec does not require us passing a
// clientversion as param, it seems some clients require it and even
// enforce a valid Code.
// GetClientVersionV1: Although the Ethereum spec does not require passing a
// client version as param, some clients require it and even enforce
// a valid Code.

if err := s.Call(
ctx, &result, GetClientVersionV1, nil,
ctx, &result, GetClientVersionV1, engine.ClientVersionV1{Code: "GE"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider making the client code configurable

The hardcoded "GE" client code might need to be configurable for different execution clients. Consider making this a configurable parameter or documenting why "GE" is used as the default.

-		ctx, &result, GetClientVersionV1, engine.ClientVersionV1{Code: "GE"},
+		ctx, &result, GetClientVersionV1, engine.ClientVersionV1{Code: s.clientCode},

Committable suggestion skipped: line range outside the PR's diff.

); err != nil {
return nil, err
}
Expand Down
19 changes: 16 additions & 3 deletions mod/node-core/pkg/components/reporting_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,41 @@ package components

import (
"cosmossdk.io/depinject"
"github.com/berachain/beacon-kit/mod/execution/pkg/client"
"github.com/berachain/beacon-kit/mod/log"
"github.com/berachain/beacon-kit/mod/node-core/pkg/components/metrics"
"github.com/berachain/beacon-kit/mod/node-core/pkg/services/version"
"github.com/berachain/beacon-kit/mod/primitives/pkg/constraints"
sdkversion "github.com/cosmos/cosmos-sdk/version"
)

type ReportingServiceInput[
ExecutionPayloadT constraints.EngineType[ExecutionPayloadT],
PayloadAttributesT client.PayloadAttributes,
LoggerT log.AdvancedLogger[LoggerT],
] struct {
depinject.In
Logger LoggerT
TelemetrySink *metrics.TelemetrySink
EngineClient *client.EngineClient[
ExecutionPayloadT,
PayloadAttributesT,
]
}

func ProvideReportingService[
ExecutionPayloadT constraints.EngineType[ExecutionPayloadT],
PayloadAttributesT client.PayloadAttributes,
LoggerT log.AdvancedLogger[LoggerT],
](
in ReportingServiceInput[LoggerT],
) *ReportingService {
return version.NewReportingService(
in ReportingServiceInput[ExecutionPayloadT, PayloadAttributesT, LoggerT],
) *version.ReportingService[ExecutionPayloadT, PayloadAttributesT] {
return version.NewReportingService[
ExecutionPayloadT, PayloadAttributesT,
](
in.Logger.With("service", "reporting"),
in.TelemetrySink,
sdkversion.Version,
in.EngineClient,
)
}
6 changes: 5 additions & 1 deletion mod/node-core/pkg/components/service_registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"github.com/berachain/beacon-kit/mod/node-api/server"
"github.com/berachain/beacon-kit/mod/node-core/pkg/components/metrics"
service "github.com/berachain/beacon-kit/mod/node-core/pkg/services/registry"
"github.com/berachain/beacon-kit/mod/node-core/pkg/services/version"
"github.com/berachain/beacon-kit/mod/observability/pkg/telemetry"
)

Expand Down Expand Up @@ -101,7 +102,10 @@ type ServiceRegistryInput[
]
Logger LoggerT
NodeAPIServer *server.Server[NodeAPIContextT]
ReportingService *ReportingService
ReportingService *version.ReportingService[
ExecutionPayloadT,
*engineprimitives.PayloadAttributes[WithdrawalT],
]
TelemetrySink *metrics.TelemetrySink
TelemetryService *telemetry.Service
ValidatorService *validator.Service[
Expand Down
4 changes: 0 additions & 4 deletions mod/node-core/pkg/components/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
consruntimetypes "github.com/berachain/beacon-kit/mod/consensus/pkg/types"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/node-core/pkg/components/signer"
"github.com/berachain/beacon-kit/mod/node-core/pkg/services/version"
"github.com/berachain/beacon-kit/mod/primitives/pkg/async"
"github.com/berachain/beacon-kit/mod/primitives/pkg/transition"
"github.com/berachain/beacon-kit/mod/storage/pkg/manager"
Expand All @@ -40,9 +39,6 @@ import (
type (
// DBManager is a type alias for the database manager.
DBManager = manager.DBManager

// ReportingService is a type alias for the reporting service.
ReportingService = version.ReportingService
)

/* -------------------------------------------------------------------------- */
Expand Down
73 changes: 0 additions & 73 deletions mod/node-core/pkg/services/version/metrics.go
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions mod/node-core/pkg/services/version/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,7 @@ type TelemetrySink interface {
// IncrementCounter increments a counter metric identified by the provided
// keys.
IncrementCounter(key string, args ...string)
// SetGauge sets a gauge metric to the specified value, identified by the
// provided keys.
SetGauge(key string, value int64, args ...string)
fridrik01 marked this conversation as resolved.
Show resolved Hide resolved
}
Loading
Loading