Trie storage statistics component #5401

ssd04 · 2023-07-06T11:03:18Z

Reasoning behind the pull request

Added storage statistics components in order to monitor load operations from trie, cache and storage

Testing procedure

Enable state statistics by setting StateStatisticsEnabled = true in [StateTriesConfig] section from config.toml
Checking (providing) logs for trie storage statistics

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

This reverts commit 94db95c.

codecov · 2023-07-17T12:50:27Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (7d118bb) 80.34% compared to head (b5f6e9a) 80.34%.
Report is 1 commits behind head on rc/v1.7.0.

❗ Current head b5f6e9a differs from pull request most recent head d2f3f82. Consider uploading reports for the commit d2f3f82 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           rc/v1.7.0    #5401    +/-   ##
===========================================
  Coverage      80.34%   80.34%            
===========================================
  Files            708      709     +1     
  Lines          93768    93922   +154     
===========================================
+ Hits           75335    75459   +124     
- Misses         13149    13173    +24     
- Partials        5284     5290     +6

Files	Coverage Δ
...uestersContainer/baseRequestersContainerFactory.go	`75.43% <100.00%> (+0.43%)`	⬆️
...uestersContainer/metaRequestersContainerFactory.go	`77.40% <100.00%> (+0.38%)`	⬆️
...estersContainer/shardRequestersContainerFactory.go	`77.61% <100.00%> (+0.33%)`	⬆️
epochStart/bootstrap/common.go	`100.00% <100.00%> (ø)`
epochStart/bootstrap/metaStorageHandler.go	`73.97% <100.00%> (+0.17%)`	⬆️
epochStart/bootstrap/process.go	`84.44% <100.00%> (+0.13%)`	⬆️
epochStart/bootstrap/shardStorageHandler.go	`88.00% <100.00%> (+0.02%)`	⬆️
epochStart/bootstrap/storageProcess.go	`73.97% <100.00%> (+0.17%)`	⬆️
factory/api/apiResolverFactory.go	`83.40% <100.00%> (+0.03%)`	⬆️
factory/bootstrap/bootstrapComponents.go	`91.97% <100.00%> (+0.04%)`	⬆️
... and 19 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BeniaminDrasovean · 2023-08-04T13:15:25Z

trie/patriciaMerkleTrie.go

+func (tr *patriciaMerkleTrie) GetStats() string {
+	dbWithStats, ok := tr.trieStorage.(storageManagerWithStats)
+	if !ok {
+		log.Warn("invalid trie storage type %T", tr.trieStorage)


This will print invalid trie storage type %T.
fmt.Sprintf("invalid trie storage type %T", tr.trieStorage) will print the storage type

done, thanks

BeniaminDrasovean · 2023-08-04T13:16:40Z

trie/patriciaMerkleTrie.go

@@ -269,6 +269,17 @@ func (tr *patriciaMerkleTrie) Commit() error {
 	return nil
 }

+// GetStats will returns trie storage operations statistics as string
+func (tr *patriciaMerkleTrie) GetStats() string {


Add tests for this func

BeniaminDrasovean · 2023-08-04T13:28:15Z

common/interface.go

@@ -116,10 +117,17 @@ type StorageManager interface {
 	IsInterfaceNil() bool
 }

+// StorageManagerWithStats defines the behaviour of a storage manager component with stats collector
+type StorageManagerWithStats interface {


I think this can be removed.

BeniaminDrasovean · 2023-08-04T13:31:40Z

common/interface.go

@@ -56,6 +56,7 @@ type Trie interface {
 	VerifyProof(rootHash []byte, key []byte, proof [][]byte) (bool, error)
 	GetStorageManager() StorageManager
 	IsMigratedToLatestVersion() (bool, error)
+	GetStats() string


GetStorageStats() ?

BeniaminDrasovean · 2023-08-04T13:42:38Z

storage/interface.go

@@ -91,6 +91,12 @@ type StorerWithPutInEpoch interface {
 	SetEpochForPutOperation(epoch uint32)
 }

+// StorerWithPutInEpoch is an extended storer with the ability to set the epoch which will be used for put operations


StorerWithStats ....

updated comment

BeniaminDrasovean · 2023-08-04T13:45:04Z

storage/pruning/pruningStorer.go

 	v, ok := ps.cacher.Get(key)
 	if ok {
-		return v.([]byte), nil
+		return v.([]byte), true, nil


On L447 replace with if errors.Is(err, storage.ErrDBIsClosed) please

BeniaminDrasovean · 2023-08-07T05:52:18Z

state/accountsDB.go

@@ -877,6 +877,7 @@ func (adb *AccountsDB) CommitInEpoch(currentEpoch uint32, epochToCommit uint32)
 	adb.mutOp.Lock()
 	defer func() {
 		adb.mainTrie.GetStorageManager().SetEpochForPutOperation(currentEpoch)
+		adb.printTrieStorageStatistics()


This needs to also be called on Commit().

The stats need to be reset after each commit if we want to know the stats for each block without adding extra operations.

Maybe move it inside func (adb *AccountsDB) commit() ([]byte, error) . This will remove duplicated code.

BeniaminDrasovean · 2023-08-07T07:42:52Z

state/accountsDB.go

+	if stats != "" {
+		log.Debug("trie storage statistics",
+			"storage manager", adb.mainTrie.GetStorageManager().GetIdentifier(),
+			"stats", adb.mainTrie.GetStats(),


use stats instead of adb.mainTrie.GetStats()

BeniaminDrasovean · 2023-08-07T07:49:37Z

trie/trieStorageManagerWithoutCheckpoints.go

@@ -41,3 +50,8 @@ func (tsm *trieStorageManagerWithoutCheckpoints) SetCheckpoint(
 func (tsm *trieStorageManagerWithoutCheckpoints) AddDirtyCheckpointHashes(_ []byte, _ common.ModifiedHashes) bool {
 	return false
 }
+
+// GetStatsCollector will return the stats collector component
+func (tsm *trieStorageManagerWithoutCheckpoints) GetStatsCollector() common.StateStatisticsHandler {


I think this can be removed, as this func is exported on common.StorageManager interface. By removing this, there is no need to have storerWithStats.

raduchis · 2023-09-05T14:14:55Z

epochStart/bootstrap/disabled/disabledChainStorer.go

@@ -59,7 +60,7 @@ func (c *chainStorer) GetStorer(unitType dataRetriever.UnitType) (storage.Storer
 	_, ok := c.mapStorages[unitType]
 	if !ok {
 		log.Debug("created new mem storer", "key", unitType)
-		c.mapStorages[unitType] = CreateMemUnit()
+		c.mapStorages[unitType] = testscommon.CreateMemStorerWithStats(CreateMemUnit())


don't we have the MemStorerWithStats somewhere else, not in the testscommon? please create it somewhere else, so that we do not import testscommon in the production code.

raduchis · 2023-09-05T14:23:33Z

epochStart/metachain/systemSCs_test.go

@@ -97,59 +97,59 @@ func TestNewSystemSCProcessor(t *testing.T) {
 	cfg := config.EnableEpochs{
 		StakingV2EnableEpoch: 100,
 	}
-	args, _ := createFullArgumentsForSystemSCProcessing(cfg, createMemUnit())
+	args, _ := createFullArgumentsForSystemSCProcessing(cfg, testscommon.CreateStorerWithStats())


this creates a memory-StorerWithStats and from the name it is not obvious any more. Could it be renamed to something like: CreateDefaultMemStorerWithStats?

raduchis · 2023-09-06T08:10:07Z

trie/patriciaMerkleTrie.go

@@ -269,6 +269,17 @@ func (tr *patriciaMerkleTrie) Commit() error {
 	return nil
 }

+// GetStorageStats will returns trie storage operations statistics as string


will return, not will returnS

raduchis · 2023-09-06T08:12:47Z

trie/patriciaMerkleTrie.go

@@ -269,6 +269,17 @@ func (tr *patriciaMerkleTrie) Commit() error {
 	return nil
 }

+// GetStorageStats will returns trie storage operations statistics as string
+func (tr *patriciaMerkleTrie) GetStorageStats() string {
+	dbWithStats, ok := tr.trieStorage.(storageManagerWithStats)


will this be called many times and fail? there will be a lot of warns in the log.
could all StorageManagers have a GetStatsCollector() that returns "" on ToString()?
if not, could you also add a unit test for the branch !ok?

raduchis · 2023-09-06T08:17:45Z

trie/trieStorageManager.go

+		return nil, storage.ErrNilStatsCollector
+	}
+
+	storerWithStats, ok := args.MainStorer.(common.StorerWithStats)


could the args.MainStorer be of type common.StorerWithStats? This way this check is not needed anymore, if the MainStorer has to be always StorerWithStats

raduchis · 2023-09-06T08:22:16Z

trie/trieStorageManager.go

@@ -184,17 +197,28 @@ func (tsm *trieStorageManager) Get(key []byte) ([]byte, error) {
 		return nil, core.ErrContextClosing
 	}

-	val, err := tsm.mainStorer.Get(key)
+	val, foundInCache, err := tsm.mainStorer.GetWithStats(key)


I don't think the trieStorageManager should know about the caches... could the statsCollector be injected one level lower in a PruningStorerWithStats for example?

raduchis · 2023-09-06T08:29:58Z

trie/node.go

 	}

-	return nil
+	return n.resolveCollapsed(pos, db)


the number of resolveCollapsed can be computed from the trieOps and cacheOps?

common/statistics/disabled/stateStatistics_test.go

dataRetriever/factory/storageRequestersContainer/shardRequestersContainerFactory.go

BeniaminDrasovean · 2023-09-06T13:39:25Z

epochStart/bootstrap/disabled/disabledStorer.go

+	return v, false, err
+}
+
+// CreateMemStorerWithStats -


Add proper comments, this is productive code

BeniaminDrasovean · 2023-09-06T13:52:46Z

trie/patriciaMerkleTrie.go

@@ -40,6 +40,7 @@ type patriciaMerkleTrie struct {
 	hasher                  hashing.Hasher
 	enableEpochsHandler     common.EnableEpochsHandler
 	trieNodeVersionVerifier core.TrieNodeVersionVerifier
+	stateStatsHandler       common.StateStatisticsHandler


This is not used anywhere

The base branch was changed.

gabi-vuls

Normal allin test: v1.5.13-dev-config-8c685c8e00 -> trie-storage-statistics-co-4c31571a2d

--- Specific errors ---

block hash does not match 6173
wrong nonce in block 2582
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 1

/------/

--- Statistics ---

Nr. of all ERRORS: 1
Nr. of all WARNS: 1442
Nr. of new ERRORS: 1
Nr. of new WARNS: 8
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

ssd04 added 4 commits July 5, 2023 12:14

storage statistics component in storage package

b509c81

addapt storage statistics component in statusCoreComponents

94db95c

Revert "addapt storage statistics component in statusCoreComponents"

bdcb008

This reverts commit 94db95c.

use stats component in trie storage manager

4e02971

ssd04 self-assigned this Jul 6, 2023

ssd04 added 7 commits July 6, 2023 16:46

refactor trie statistics component

5ba1393

update storer with stats interface

1be0bd0

fix trie package tests

1a7db42

refactor to cast storagemanager in constructor

2471b82

fix epoch start unit tests

261c492

Merge branch 'rc/v1.6.0' into trie-storage-statistics-component

088a9ac

fix test in epochstart and integration tests

1b5b5de

ssd04 added 4 commits July 18, 2023 08:49

add missing comments

68cba11

fix benchmarks integration test

a352567

fix linter issue

c990e1d

Merge branch 'rc/v1.6.0' into trie-storage-statistics-component

8aeac06

ssd04 marked this pull request as ready for review August 4, 2023 07:19

BeniaminDrasovean requested changes Aug 7, 2023

View reviewed changes

ssd04 added 2 commits August 7, 2023 17:26

fixes after review

c0afd2a

update trie stub

8703e00

BeniaminDrasovean previously approved these changes Aug 11, 2023

View reviewed changes

ssd04 added 2 commits September 4, 2023 15:48

Merge branch 'rc/v1.6.0' into trie-storage-statistics-component

239929d

fix unit tests after merge

7a8fbb4

ssd04 dismissed BeniaminDrasovean’s stale review via 7a8fbb4 September 4, 2023 13:57

raduchis self-requested a review September 5, 2023 14:09

raduchis requested changes Sep 6, 2023

View reviewed changes

move state stats collector in common

c646309

BeniaminDrasovean reviewed Sep 6, 2023

View reviewed changes

Merge branch 'rc/v1.6.0' into trie-storage-statistics-component

dc3b665

BeniaminDrasovean previously approved these changes Oct 25, 2023

View reviewed changes

raduchis previously approved these changes Oct 26, 2023

View reviewed changes

Merge branch 'rc/v1.6.0' into trie-storage-statistics-component

d45437b

ssd04 changed the base branch from rc/v1.6.0 to rc/v1.7.0 October 26, 2023 13:33

ssd04 added 7 commits October 26, 2023 16:45

Merge branch 'rc/v1.7.0' into trie-storage-statistics-component

9593d18

revert changes from d45437b

ee9c8b6

fix unit test

a444c20

Merge branch 'rc/v1.7.0' into trie-storage-statistics-component

343e7c8

update stats return format

2b19dd3

revert additional changes from 343e7c8

97ad360

Merge branch 'rc/v1.7.0' into trie-storage-statistics-component

63b2a82

gabi-vuls previously approved these changes Nov 3, 2023

View reviewed changes

BeniaminDrasovean previously approved these changes Nov 3, 2023

View reviewed changes

raduchis approved these changes Nov 3, 2023

View reviewed changes

raduchis previously approved these changes Nov 3, 2023

View reviewed changes

fix integration tests

6a4be57

ssd04 dismissed stale reviews from raduchis, BeniaminDrasovean, and gabi-vuls via 6a4be57 November 3, 2023 15:01

fix hardfork integration test

c192ba4

BeniaminDrasovean previously approved these changes Nov 6, 2023

View reviewed changes

fix create prunning storer maps epoch

d2f3f82

ssd04 dismissed BeniaminDrasovean’s stale review via d2f3f82 November 6, 2023 09:25

BeniaminDrasovean approved these changes Nov 6, 2023

View reviewed changes

raduchis approved these changes Nov 6, 2023

View reviewed changes

gabi-vuls approved these changes Nov 6, 2023

View reviewed changes

ssd04 merged commit e6e79c4 into rc/v1.7.0 Nov 6, 2023
6 checks passed

ssd04 deleted the trie-storage-statistics-component branch November 6, 2023 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trie storage statistics component #5401

Trie storage statistics component #5401

ssd04 commented Jul 6, 2023 •

edited

codecov bot commented Jul 17, 2023 •

edited

BeniaminDrasovean Aug 4, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 4, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 4, 2023

BeniaminDrasovean Aug 4, 2023

BeniaminDrasovean Aug 4, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Aug 4, 2023

BeniaminDrasovean Aug 7, 2023

ssd04 Aug 7, 2023

BeniaminDrasovean Oct 23, 2023

BeniaminDrasovean Aug 7, 2023

BeniaminDrasovean Aug 7, 2023

ssd04 Aug 7, 2023

raduchis Sep 5, 2023

raduchis Sep 5, 2023

raduchis Sep 6, 2023

raduchis Sep 6, 2023

raduchis Sep 6, 2023

raduchis Sep 6, 2023

raduchis Sep 6, 2023

BeniaminDrasovean Sep 6, 2023

BeniaminDrasovean Sep 6, 2023

gabi-vuls left a comment

Trie storage statistics component #5401

Trie storage statistics component #5401

Conversation

ssd04 commented Jul 6, 2023 • edited

Reasoning behind the pull request

Testing procedure

Pre-requisites

codecov bot commented Jul 17, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

ssd04 commented Jul 6, 2023 •

edited

codecov bot commented Jul 17, 2023 •

edited