From 18000c4cf9fd1d02355251ec7ecf2b04d68877ca Mon Sep 17 00:00:00 2001 From: galaio <12880651+galaio@users.noreply.github.com> Date: Fri, 10 Jan 2025 21:06:16 +0800 Subject: [PATCH] metric: revert default expensive metrics in blockchain; (#2850) --- cmd/geth/config.go | 3 +- cmd/utils/flags.go | 4 +++ core/blockchain.go | 57 ++++++++++++++++++++------------------ core/state/state_object.go | 11 ++++++-- core/state/statedb.go | 47 +++++++++++++++++++++++-------- metrics/config.go | 2 +- metrics/metrics.go | 15 ++++++++++ 7 files changed, 97 insertions(+), 42 deletions(-) diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 8d441e29dc..d7ffc09e1d 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -316,7 +316,8 @@ func applyMetricConfig(ctx *cli.Context, cfg *gethConfig) { cfg.Metrics.Enabled = ctx.Bool(utils.MetricsEnabledFlag.Name) } if ctx.IsSet(utils.MetricsEnabledExpensiveFlag.Name) { - log.Warn("Expensive metrics are collected by default, please remove this flag", "flag", utils.MetricsEnabledExpensiveFlag.Name) + log.Warn("Expensive metrics will remain in BSC and may be removed in the future", "flag", utils.MetricsEnabledExpensiveFlag.Name) + cfg.Metrics.EnabledExpensive = ctx.Bool(utils.MetricsEnabledExpensiveFlag.Name) } if ctx.IsSet(utils.MetricsHTTPFlag.Name) { cfg.Metrics.HTTP = ctx.String(utils.MetricsHTTPFlag.Name) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index b18c1dfa6f..ded306cdcc 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -2457,6 +2457,10 @@ func SetupMetrics(cfg *metrics.Config, options ...SetupMetricsOption) { } log.Info("Enabling metrics collection") metrics.Enable() + if cfg.EnabledExpensive { + log.Info("Enabling expensive metrics collection") + metrics.EnableExpensive() + } // InfluxDB exporter. var ( diff --git a/core/blockchain.go b/core/blockchain.go index 709faac242..6c047a3e2b 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -76,24 +76,24 @@ var ( chainInfoGauge = metrics.NewRegisteredGaugeInfo("chain/info", nil) - accountReadTimer = metrics.NewRegisteredResettingTimer("chain/account/reads", nil) - accountHashTimer = metrics.NewRegisteredResettingTimer("chain/account/hashes", nil) - accountUpdateTimer = metrics.NewRegisteredResettingTimer("chain/account/updates", nil) - accountCommitTimer = metrics.NewRegisteredResettingTimer("chain/account/commits", nil) + accountReadTimer = metrics.NewRegisteredTimer("chain/account/reads", nil) + accountHashTimer = metrics.NewRegisteredTimer("chain/account/hashes", nil) + accountUpdateTimer = metrics.NewRegisteredTimer("chain/account/updates", nil) + accountCommitTimer = metrics.NewRegisteredTimer("chain/account/commits", nil) - storageReadTimer = metrics.NewRegisteredResettingTimer("chain/storage/reads", nil) - storageUpdateTimer = metrics.NewRegisteredResettingTimer("chain/storage/updates", nil) - storageCommitTimer = metrics.NewRegisteredResettingTimer("chain/storage/commits", nil) + storageReadTimer = metrics.NewRegisteredTimer("chain/storage/reads", nil) + storageUpdateTimer = metrics.NewRegisteredTimer("chain/storage/updates", nil) + storageCommitTimer = metrics.NewRegisteredTimer("chain/storage/commits", nil) - accountReadSingleTimer = metrics.NewRegisteredResettingTimer("chain/account/single/reads", nil) - storageReadSingleTimer = metrics.NewRegisteredResettingTimer("chain/storage/single/reads", nil) + accountReadSingleTimer = metrics.NewRegisteredTimer("chain/account/single/reads", nil) + storageReadSingleTimer = metrics.NewRegisteredTimer("chain/storage/single/reads", nil) - snapshotCommitTimer = metrics.NewRegisteredResettingTimer("chain/snapshot/commits", nil) - triedbCommitTimer = metrics.NewRegisteredResettingTimer("chain/triedb/commits", nil) + snapshotCommitTimer = metrics.NewRegisteredTimer("chain/snapshot/commits", nil) + triedbCommitTimer = metrics.NewRegisteredTimer("chain/triedb/commits", nil) blockInsertTimer = metrics.NewRegisteredTimer("chain/inserts", nil) blockValidationTimer = metrics.NewRegisteredTimer("chain/validation", nil) - blockCrossValidationTimer = metrics.NewRegisteredResettingTimer("chain/crossvalidation", nil) + blockCrossValidationTimer = metrics.NewRegisteredTimer("chain/crossvalidation", nil) blockExecutionTimer = metrics.NewRegisteredTimer("chain/execution", nil) blockWriteTimer = metrics.NewRegisteredTimer("chain/write", nil) @@ -2424,17 +2424,19 @@ func (bc *BlockChain) processBlock(block *types.Block, statedb *state.StateDB, s proctime := time.Since(start) // processing + validation + cross validation // Update the metrics touched during block processing and validation - accountReadTimer.Update(statedb.AccountReads) // Account reads are complete(in processing) - storageReadTimer.Update(statedb.StorageReads) // Storage reads are complete(in processing) - if statedb.AccountLoaded != 0 { - accountReadSingleTimer.Update(statedb.AccountReads / time.Duration(statedb.AccountLoaded)) - } - if statedb.StorageLoaded != 0 { - storageReadSingleTimer.Update(statedb.StorageReads / time.Duration(statedb.StorageLoaded)) + if metrics.EnabledExpensive() { + accountReadTimer.Update(statedb.AccountReads) // Account reads are complete(in processing) + storageReadTimer.Update(statedb.StorageReads) // Storage reads are complete(in processing) + if statedb.AccountLoaded != 0 { + accountReadSingleTimer.Update(statedb.AccountReads / time.Duration(statedb.AccountLoaded)) + } + if statedb.StorageLoaded != 0 { + storageReadSingleTimer.Update(statedb.StorageReads / time.Duration(statedb.StorageLoaded)) + } + accountUpdateTimer.Update(statedb.AccountUpdates) // Account updates are complete(in validation) + storageUpdateTimer.Update(statedb.StorageUpdates) // Storage updates are complete(in validation) + accountHashTimer.Update(statedb.AccountHashes) // Account hashes are complete(in validation) } - accountUpdateTimer.Update(statedb.AccountUpdates) // Account updates are complete(in validation) - storageUpdateTimer.Update(statedb.StorageUpdates) // Storage updates are complete(in validation) - accountHashTimer.Update(statedb.AccountHashes) // Account hashes are complete(in validation) triehash := statedb.AccountHashes // The time spent on tries hashing trieUpdate := statedb.AccountUpdates + statedb.StorageUpdates // The time spent on tries update blockExecutionTimer.Update(ptime - (statedb.AccountReads + statedb.StorageReads)) // The time spent on EVM processing @@ -2456,11 +2458,12 @@ func (bc *BlockChain) processBlock(block *types.Block, statedb *state.StateDB, s return nil, err } // Update the metrics touched during block commit - accountCommitTimer.Update(statedb.AccountCommits) // Account commits are complete, we can mark them - storageCommitTimer.Update(statedb.StorageCommits) // Storage commits are complete, we can mark them - snapshotCommitTimer.Update(statedb.SnapshotCommits) // Snapshot commits are complete, we can mark them - triedbCommitTimer.Update(statedb.TrieDBCommits) // Trie database commits are complete, we can mark them - + if metrics.EnabledExpensive() { + accountCommitTimer.Update(statedb.AccountCommits) // Account commits are complete, we can mark them + storageCommitTimer.Update(statedb.StorageCommits) // Storage commits are complete, we can mark them + snapshotCommitTimer.Update(statedb.SnapshotCommits) // Snapshot commits are complete, we can mark them + triedbCommitTimer.Update(statedb.TrieDBCommits) // Trie database commits are complete, we can mark them + } blockWriteTimer.Update(time.Since(wstart) - max(statedb.AccountCommits, statedb.StorageCommits) /* concurrent */ - statedb.SnapshotCommits - statedb.TrieDBCommits) blockInsertTimer.UpdateSince(start) diff --git a/core/state/state_object.go b/core/state/state_object.go index 56efc5c89b..1cea54dcfb 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -24,6 +24,8 @@ import ( "sync" "time" + "github.com/ethereum/go-ethereum/metrics" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" @@ -224,13 +226,18 @@ func (s *stateObject) GetCommittedState(key common.Hash) common.Hash { } s.db.StorageLoaded++ - start := time.Now() + var start time.Time + if metrics.EnabledExpensive() { + start = time.Now() + } value, err := s.db.reader.Storage(s.address, key) if err != nil { s.db.setError(err) return common.Hash{} } - s.db.StorageReads += time.Since(start) + if metrics.EnabledExpensive() { + s.db.StorageReads += time.Since(start) + } // Schedule the resolved storage slots for prefetching if it's enabled. if s.db.prefetcher != nil && s.data.Root != types.EmptyRootHash { diff --git a/core/state/statedb.go b/core/state/statedb.go index 1b92a8a198..9a717b36c8 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -26,6 +26,8 @@ import ( "sync/atomic" "time" + "github.com/ethereum/go-ethereum/metrics" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/state/snapshot" @@ -699,7 +701,9 @@ func (s *StateDB) getStateObject(addr common.Address) *stateObject { s.setError(fmt.Errorf("getStateObject (%x) error: %w", addr.Bytes(), err)) return nil } - s.AccountReads += time.Since(start) + if metrics.EnabledExpensive() { + s.AccountReads += time.Since(start) + } // Short circuit if the account is not found if acct == nil { @@ -923,9 +927,13 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // method will internally call a blocking trie fetch from the prefetcher, // so there's no need to explicitly wait for the prefetchers to finish. var ( - start = time.Now() + start time.Time workers errgroup.Group ) + + if metrics.EnabledExpensive() { + start = time.Now() + } if s.db.TrieDB().IsVerkle() { // Whilst MPT storage tries are independent, Verkle has one single trie // for all the accounts and all the storage slots merged together. The @@ -988,7 +996,9 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { } } workers.Wait() - s.StorageUpdates += time.Since(start) + if metrics.EnabledExpensive() { + s.StorageUpdates += time.Since(start) + } // Now we're about to start to write changes to the trie. The trie is so far // _untouched_. We can check with the prefetcher, if it can give us a trie @@ -997,7 +1007,10 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // Don't check prefetcher if verkle trie has been used. In the context of verkle, // only a single trie is used for state hashing. Replacing a non-nil verkle tree // here could result in losing uncommitted changes from storage. - start = time.Now() + + if metrics.EnabledExpensive() { + start = time.Now() + } if s.prefetcher != nil { if trie := s.prefetcher.trie(common.Hash{}, s.originalRoot); trie == nil { log.Debug("Failed to retrieve account pre-fetcher trie") @@ -1044,14 +1057,18 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { s.deleteStateObject(deletedAddr) s.AccountDeleted += 1 } - s.AccountUpdates += time.Since(start) + if metrics.EnabledExpensive() { + s.AccountUpdates += time.Since(start) + } if s.prefetcher != nil && len(usedAddrs) > 0 { s.prefetcher.used(common.Hash{}, s.originalRoot, usedAddrs, nil) } - // Track the amount of time wasted on hashing the account trie - defer func(start time.Time) { s.AccountHashes += time.Since(start) }(time.Now()) + if metrics.EnabledExpensive() { + // Track the amount of time wasted on hashing the account trie + defer func(start time.Time) { s.AccountHashes += time.Since(start) }(time.Now()) + } hash := s.trie.Hash() @@ -1357,7 +1374,9 @@ func (s *StateDB) commit(deleteEmptyObjects bool) (*stateUpdate, error) { if err := merge(set); err != nil { return err } - s.AccountCommits = time.Since(start) + if metrics.EnabledExpensive() { + s.AccountCommits = time.Since(start) + } return nil }) // Schedule each of the storage tries that need to be updated, so they can @@ -1388,7 +1407,9 @@ func (s *StateDB) commit(deleteEmptyObjects bool) (*stateUpdate, error) { } lock.Lock() updates[obj.addrHash] = update - s.StorageCommits = time.Since(start) // overwrite with the longest storage commit runtime + if metrics.EnabledExpensive() { + s.StorageCommits = time.Since(start) // overwrite with the longest storage commit runtime + } lock.Unlock() return nil }) @@ -1468,7 +1489,9 @@ func (s *StateDB) commitAndFlush(block uint64, deleteEmptyObjects bool) (*stateU log.Warn("Failed to cap snapshot tree", "root", ret.root, "layers", TriesInMemory, "err", err) } }() - s.SnapshotCommits += time.Since(start) + if metrics.EnabledExpensive() { + s.SnapshotCommits += time.Since(start) + } } // If trie database is enabled, commit the state update as a new layer if db := s.db.TrieDB(); db != nil && !s.noTrie { @@ -1476,7 +1499,9 @@ func (s *StateDB) commitAndFlush(block uint64, deleteEmptyObjects bool) (*stateU if err := db.Update(ret.root, ret.originRoot, block, ret.nodes, ret.stateSet()); err != nil { return nil, err } - s.TrieDBCommits += time.Since(start) + if metrics.EnabledExpensive() { + s.TrieDBCommits += time.Since(start) + } } } s.reader, _ = s.db.Reader(s.originalRoot) diff --git a/metrics/config.go b/metrics/config.go index 72f94dd194..2eb09fb48a 100644 --- a/metrics/config.go +++ b/metrics/config.go @@ -19,7 +19,7 @@ package metrics // Config contains the configuration for the metric collection. type Config struct { Enabled bool `toml:",omitempty"` - EnabledExpensive bool `toml:"-"` + EnabledExpensive bool `toml:",omitempty"` HTTP string `toml:",omitempty"` Port int `toml:",omitempty"` EnableInfluxDB bool `toml:",omitempty"` diff --git a/metrics/metrics.go b/metrics/metrics.go index 3602e88d5e..2839df8e95 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -13,6 +13,11 @@ import ( var ( metricsEnabled = false + + // metricsExpensiveEnabled is a soft-flag meant for external packages to check if costly + // metrics gathering is allowed or not. The goal is to separate standard metrics + // for health monitoring and debug metrics that might impact runtime performance. + metricsExpensiveEnabled = false ) // Enabled is checked by functions that are deemed 'expensive', e.g. if a @@ -31,6 +36,16 @@ func Enable() { metricsEnabled = true } +// EnabledExpensive is checked by functions that are deemed 'expensive'. +func EnabledExpensive() bool { + return metricsExpensiveEnabled +} + +// EnableExpensive enables the expensive metrics. +func EnableExpensive() { + metricsExpensiveEnabled = true +} + var threadCreateProfile = pprof.Lookup("threadcreate") type runtimeStats struct {