Metrics (Objects): Blocks, transactions, receipts and accounts (state) reads/writes #916
Conversation
* first commit * using bytes32 method
| @@ -538,6 +540,8 @@ func (s *State) Stage(newVer trie.Version) (*Stage, error) { | |||
| return err | |||
| } | |||
| } | |||
| // Just once for the account trie. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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
- 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
changestimes (so if changes is 9, 9 times). - The account gets saved in the
trieCpyand then thetrieCpyis appended to thetrieshere https://github.com/vechain/thor/blob/miguel/object-write-read2/state/state.go#L522 - In the function associated to
commitwe commit all the elements oftries, beingtrieCpythe last one (hence the commentjust oncein there) - 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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"}) | |||
There was a problem hiding this comment.
Do we already have similar metrics in PR #910?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I would have put this metric here, but I also wanted to record the domain cache/hit, ie blocks, transactions or receipts
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Nah, I might use wrong word regarding 'inevitable', I was thinking that writing to block/transaction/receipt was deterministic.
| @@ -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"}) | |||
There was a problem hiding this comment.
The stackedmap is an in-memory cache stuff with stack functionality, I don't see a necessity of adding metrics for it.
There was a problem hiding this comment.
Ok, will get rid of the stackedmap metrics then, thanks.
| @@ -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"}) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/db #916 +/- ##
===========================================
+ Coverage 61.54% 61.56% +0.02%
===========================================
Files 217 217
Lines 22635 22649 +14
===========================================
+ Hits 13930 13944 +14
Misses 7583 7583
Partials 1122 1122 ☔ View full report in Codecov by Sentry. |
Description
This PR adds metrics for the following elements in the context of the
repository:Havent added metrics (yet) for stackedmap writes (just in case you guys think it is not important a metric on this guy). This is how compares with trie (green is read, yellow is write, again by rate):