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

Metrics (Objects): Blocks, transactions, receipts and accounts (state) reads/writes #916

Open
wants to merge 16 commits into
base: release/maindbv4
Choose a base branch
from
+33 −7
Open

Metrics (Objects): Blocks, transactions, receipts and accounts (state) reads/writes #916

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
822eb97
chore: fixed vulnerability
freemanzMrojo Dec 9, 2024
a751385
chore: fixed vulnerability
freemanzMrojo Dec 9, 2024
e5df4aa
first commit
freemanzMrojo Dec 10, 2024
d1ddff0
second commit
freemanzMrojo Dec 10, 2024
f801be2
added receipt
freemanzMrojo Dec 10, 2024
4bb278a
added receipt
freemanzMrojo Dec 10, 2024
8d1488a
state commit
freemanzMrojo Dec 10, 2024
64b9219
fix: Get Devnet ID after the Thor flags are set (#915)
freemanzMrojo Dec 10, 2024
98228a5
added more dashboard
freemanzMrojo Dec 10, 2024
2b7e909
added target
freemanzMrojo Dec 10, 2024
0cca5f3
Merge branch 'feat/db' into miguel/object-write-read2
freemanzMrojo Dec 10, 2024
9025c5f
Merge branch 'feat/db' into miguel/object-write-read2
freemanzMrojo Dec 10, 2024
db0f469
Merge branch 'feat/db' into miguel/object-write-read2
freemanzMrojo Jan 2, 2025
01aff2c
Merge branch 'feat/db' into miguel/object-write-read2
freemanzMrojo Jan 6, 2025
bdedf37
Merge branch 'feat/db' into miguel/object-write-read2
freemanzMrojo Jan 13, 2025
6dd54f7
linter
freemanzMrojo Jan 13, 2025
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
1 change: 1 addition & 0 deletions chain/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func newCache(maxSize int) *cache {

func (c *cache) GetOrLoad(key interface{}, load func() (interface{}, error)) (interface{}, error) {
if value, ok := c.Get(key); ok {
metricBlockRepositoryCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "cache"})
Copy link
Member

Choose a reason for hiding this comment

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

Do we already have similar metrics in PR #910?

Copy link
Member Author

Choose a reason for hiding this comment

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

These metrics are per object, as described in the title. I do not know why #910 was done since cache miss/rate was already covered by this.

But it is true that this other cache looks like a duplicate since it is placed precisely in receipts, transactions and so on with the difference that only applies to reads.

I can align both things. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I would have put this metric here, but I also wanted to record the domain cache/hit, ie blocks, transactions or receipts

Copy link
Member

Choose a reason for hiding this comment

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

After looking into the names, now I understand the purpose of them. Regarding Reads, I would suggest leverage cache.hit + cache.miss. Regarding Writes I would consider they are immutable based on the blocks imported. Is write inevitable from your point of view?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed a duplicate, the thing is I was working on this ticket so as you can imagine I was not expecting someone else doing the same thing.

About Writes, you mean block-wise or also for the other "objects" (tx, receipts...)? Regarding blocks, the metric is only submitted to Prometheus when calling AddBlock. Not sure what you mean by "inevitable", should not we record them?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, I might use wrong word regarding 'inevitable', I was thinking that writing to block/transaction/receipt was deterministic.

return value, nil
}
value, err := load()
Expand Down
2 changes: 2 additions & 0 deletions chain/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ func (c *Chain) GetTransaction(id thor.Bytes32) (*tx.Transaction, *TxMeta, error
if err != nil {
return nil, nil, err
}
metricTransactionRepositoryCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "db"})
return tx, txMeta, nil
}

Expand All @@ -256,6 +257,7 @@ func (c *Chain) GetTransactionReceipt(txID thor.Bytes32) (*tx.Receipt, error) {
if err != nil {
return nil, err
}
metricReceiptRepositoryCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "db"})
return receipt, nil
}

Expand Down
3 changes: 3 additions & 0 deletions chain/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ import "github.com/vechain/thor/v2/metrics"

var (
metricCacheHitMiss = metrics.LazyLoadCounterVec("repo_cache_hit_miss_count", []string{"type", "event"})
metricBlockRepositoryCounter = metrics.LazyLoadCounterVec("block_repository_count", []string{"type", "target"})
metricTransactionRepositoryCounter = metrics.LazyLoadCounterVec("transaction_repository_count", []string{"type", "target"})
metricReceiptRepositoryCounter = metrics.LazyLoadCounterVec("receipt_repository_count", []string{"type", "target"})
)
1 change: 1 addition & 0 deletions chain/persist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,6 @@ func loadBlockSummary(r kv.Getter, id thor.Bytes32) (*BlockSummary, error) {
if err := loadRLP(r, id[:], &summary); err != nil {
return nil, err
}
metricBlockRepositoryCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "db"})
return &summary, nil
}
5 changes: 5 additions & 0 deletions chain/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ func (r *Repository) saveBlock(block *block.Block, receipts tx.Receipts, conflic
}
r.caches.txs.Add(string(keyBuf), tx)
}
metricTransactionRepositoryCounter().AddWithLabel(int64(len(txs)), map[string]string{"type": "write", "target": "db"})

// save receipts
for i, receipt := range receipts {
Expand All @@ -186,6 +187,7 @@ func (r *Repository) saveBlock(block *block.Block, receipts tx.Receipts, conflic
}
r.caches.receipts.Add(string(keyBuf), receipt)
}
metricReceiptRepositoryCounter().AddWithLabel(int64(len(receipts)), map[string]string{"type": "write", "target": "db"})
}
if err := indexChainHead(headPutter, header); err != nil {
return nil, err
Expand Down Expand Up @@ -229,6 +231,7 @@ func (r *Repository) AddBlock(newBlock *block.Block, receipts tx.Receipts, confl
if _, err := r.saveBlock(newBlock, receipts, conflicts, asBest); err != nil {
return err
}
metricBlockRepositoryCounter().AddWithLabel(1, map[string]string{"type": "write", "target": "db"})
return nil
}

Expand Down Expand Up @@ -334,6 +337,7 @@ func (r *Repository) GetBlockTransactions(id thor.Bytes32) (tx.Transactions, err
return nil, err
}
}
metricTransactionRepositoryCounter().AddWithLabel(int64(n), map[string]string{"type": "read"})
return txs, nil
}
return nil, nil
Expand Down Expand Up @@ -390,6 +394,7 @@ func (r *Repository) GetBlockReceipts(id thor.Bytes32) (tx.Receipts, error) {
return nil, err
}
}
metricReceiptRepositoryCounter().AddWithLabel(int64(n), map[string]string{"type": "read", "target": "db"})
return receipts, nil
}
return nil, nil
Expand Down
12 changes: 6 additions & 6 deletions metrics/prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (o *prometheusMetrics) newHistogramMeter(name string, buckets []int64) Hist

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}

return &promHistogramMeter{
Expand Down Expand Up @@ -237,7 +237,7 @@ func (o *prometheusMetrics) newHistogramVecMeter(name string, labels []string, b

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}

return &promHistogramVecMeter{
Expand All @@ -263,7 +263,7 @@ func (o *prometheusMetrics) newCountMeter(name string) CountMeter {

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}
return &promCountMeter{
counter: meter,
Expand All @@ -281,7 +281,7 @@ func (o *prometheusMetrics) newCountVecMeter(name string, labels []string) Count

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}
return &promCountVecMeter{
counter: meter,
Expand All @@ -298,7 +298,7 @@ func (o *prometheusMetrics) newGaugeMeter(name string) GaugeMeter {

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}
return &promGaugeMeter{
gauge: meter,
Expand All @@ -316,7 +316,7 @@ func (o *prometheusMetrics) newGaugeVecMeter(name string, labels []string) Gauge

err := prometheus.Register(meter)
if err != nil {
logger.Warn("unable to register metric", "err", err)
logger.Warn(fmt.Sprintf("unable to register metric %s", name), "err", err)
}
return &promGaugeVecMeter{
gauge: meter,
Expand Down
10 changes: 10 additions & 0 deletions state/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright (c) 2024 The VeChainThor developers
//
// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying
// file LICENSE or <https://www.gnu.org/licenses/lgpl-3.0.html>

package state

import "github.com/vechain/thor/v2/metrics"

var metricAccountCounter = metrics.LazyLoadCounterVec("account_state_count", []string{"type", "target"})
4 changes: 4 additions & 0 deletions state/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func (s *State) getCachedObject(addr thor.Address) (*cachedObject, error) {
if err != nil {
return nil, err
}
metricAccountCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "trie"})
Copy link
Member

Choose a reason for hiding this comment

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

State.GetAccount -> stackedMap.Get -> state.getCachedObject -> state.LoadAccount -> trie.Get

This might not be reflecting the real calling path, but just for demonstrating the path from state to the trie package. I would suggest consider in the low-level packages other than this one.

Copy link
Member Author

@freemanzMrojo freemanzMrojo Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The problem of this approach is that we miss the concept of object, so at the trie level we do not know whether it is an account or something else. That method (trie.Get) is actually used also by the chain package.

Copy link
Member

Choose a reason for hiding this comment

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

Let's solve this offline.

co := newCachedObject(s.db, addr, a, am)
s.cache[addr] = co
return co, nil
Expand All @@ -131,6 +132,7 @@ func (s *State) getAccount(addr thor.Address) (*Account, error) {
if err != nil {
return nil, err
}
metricAccountCounter().AddWithLabel(1, map[string]string{"type": "read", "target": "stackedmap"})
Copy link
Member

Choose a reason for hiding this comment

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

The stackedmap is an in-memory cache stuff with stack functionality, I don't see a necessity of adding metrics for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will get rid of the stackedmap metrics then, thanks.

return v.(*Account), nil
}

Expand Down Expand Up @@ -538,6 +540,8 @@ func (s *State) Stage(newVer trie.Version) (*Stage, error) {
return err
}
}
// Just once for the account trie.
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest avoid including metrics in state, as it's the primitive operation and all the metrics will be fallen into trie node cache.

Copy link
Member Author

@freemanzMrojo freemanzMrojo Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(Copying this conversation here so others could have this context too)

let me elaborate a bit more over the code: https://github.com/vechain/thor/blob/miguel/object-write-read2/state/state.go

  1. We save the account here https://github.com/vechain/thor/blob/miguel/object-write-read2/state/state.go#L487 in a loop with all the changes . If successful basically we save account changes times (so if changes is 9, 9 times).
  2. The account gets saved in the trieCpy and then the trieCpy is appended to the tries here https://github.com/vechain/thor/blob/miguel/object-write-read2/state/state.go#L522
  3. In the function associated to commit we commit all the elements of tries , being trieCpy the last one (hence the comment just once in there)
  4. By placing the metric in there, I am storing the number of times we save an account in a single place, and the number we call it is changes, hence that line

hope I managed to explain it properly. or if you see I am missing something in my argument please let me know

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation, I was proposed avoiding including metrics in the state package, reason for that is avoiding potential huge overhead and all state operations are trie operations. Since we already got metrics in the trie package, why don't leave state package without metrics.

Secondly, commit function usually being called for every block processing, I would suggest remove this metric.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea of having these metrics here is because of the object, in this case accounts. At trie we do not have that visibility, hence we cannot figure this out.

metricAccountCounter().AddWithLabel(int64(len(changes)), map[string]string{"type": "write", "target": "trie"})
return nil
},
}, nil
Expand Down
Loading