refactor(core): Better metrics for exchange and add metric to fetcher #4651
Conversation
renaynay
added
area:core_and_app
renaynay
requested review from
Wondertan,
vgonkivs and
walldiss
as code owners
core/fetcher.go
Outdated
| isLast = resp.IsLast | ||
| } | ||
|
|
||
| f.metrics.observeReceiveBlock(ctx, time.Since(start), len(parts)) |
There was a problem hiding this comment.
BTW - the majority of download time (exchange metric) is actually spent here so I'm not 100% sure we need both exchange block download time and fetcher receive block time.
There was a problem hiding this comment.
it would be good to keep it actually to observe departure in case it happens.
core/fetcher_metrics.go
Outdated
| // numPartsAttribute creates an attribute for the number of parts in a block. | ||
| // This can be used to approximate the block size (with each part being roughly | ||
| // 64KB). | ||
| func numPartsAttribute(numParts int) attribute.KeyValue { | ||
| return attribute.Int("num_parts", numParts) | ||
| } |
There was a problem hiding this comment.
Seems a bit too much to have a function for that. It adds extra nesting
There was a problem hiding this comment.
Oh it's a relic where i was actually trying to do some math there to reduce cardinality but it's fine to just do numParts as int attribute i think.
renaynay
force-pushed
the
rene/core-metrics
branch
from
d73b058 to
3e2a1da
Compare
walldiss
left a comment
walldiss
left a comment
There was a problem hiding this comment.
Given that otel does no support proper buckets setup, those might not bring as much value as expected. Like all time events might get recorded as same bucket. I would suggest to try to add traces for same events and compare avarages, to verify that metrics are accurate here
|
@walldiss to view metrics by |
|
@walldiss new dashboard for total # blocks/sec for BN syncing 
and one grouped by eds_size (looks bad rn bc validator i'm connected to got stuck, but you get the picture) |
|
When referring to buckets I was talking about usage of Histograms to track time. 3/4 of new metrics try to track time and my point is that it will most likely not do this. However as you pointed out tracking rate is possible through counter of events. |
Adds to exchange:
Adds to fetcher: