Engine getblobs v2 metrics #31834
Engine getblobs v2 metrics #31834
Conversation
While running kurtosis and spamoor, I noticed occasional `null` entries coming back from `getBlobsV2`. After investigating, I found that spamoor can use the same blob data across multiple transactions, so with larger blob counts, there's an increased chance that a blob is included in a block more than once. As a result, previous indexes in the hash-to-index map in `getBlobsV2` would get overwritten. I changed the map from `map[common.Hash]int` to `map[common.Hash][]int` to handle this case. I decided to go this route because it's the smallest-possible fix, but it could also make sense to update `blobpool.GetBlobs` to de-duplicate the hashes.
SunnysidedJ
requested review from
MariusVanDerWijden,
fjl,
rjl493456442,
lightclient,
jwasinger and
gballet
as code owners
| getBlobsV2BlobsRequestedTotal = metrics.NewRegisteredCounter("get_blobs_requests_blobs_total", nil) | ||
| // Number of blobs requested via getBlobsV2 that are present in the blobpool | ||
| getBlobsV2BlobsInPoolTotal = metrics.NewRegisteredCounter("get_blobs_requests_blobs_in_blobpool_total", nil) | ||
| // Number of times getBlobsV2 responded with “hit” | ||
| getBlobsV2RequestHit = metrics.NewRegisteredCounter("get_blobs_requests_success_total", nil) | ||
| // Number of times getBlobsV2 responded with “miss” | ||
| getBlobsV2RequestMiss = metrics.NewRegisteredCounter("get_blobs_requests_failure_total", nil) |
There was a problem hiding this comment.
What do you think about something like this?
| getBlobsV2BlobsRequestedTotal = metrics.NewRegisteredCounter("get_blobs_requests_blobs_total", nil) | |
| // Number of blobs requested via getBlobsV2 that are present in the blobpool | |
| getBlobsV2BlobsInPoolTotal = metrics.NewRegisteredCounter("get_blobs_requests_blobs_in_blobpool_total", nil) | |
| // Number of times getBlobsV2 responded with “hit” | |
| getBlobsV2RequestHit = metrics.NewRegisteredCounter("get_blobs_requests_success_total", nil) | |
| // Number of times getBlobsV2 responded with “miss” | |
| getBlobsV2RequestMiss = metrics.NewRegisteredCounter("get_blobs_requests_failure_total", nil) | |
| getBlobsRequestedCounter = metrics.NewRegisteredCounter("engine/getblobs/requested", nil) | |
| // Number of blobs requested via getBlobsV2 that are present in the blobpool | |
| getBlobsAvailableCounter = metrics.NewRegisteredCounter("engine/getblobs/available", nil) | |
| // Number of times getBlobsV2 responded with “hit” | |
| getBlobsV2RequestHit = metrics.NewRegisteredCounter("engine/getblobs/hit, nil) | |
| // Number of times getBlobsV2 responded with “miss” | |
| getBlobsV2RequestMiss = metrics.NewRegisteredCounter("engine/getblobs/miss", nil) |
There was a problem hiding this comment.
This naming convention is also reasonable approach. Unfortunately, we are trying to follow the convention recommended by Prometheus, and some clients already have them merged. Would you mind these names? The documentation is here (mentioned in the original PR): https://testinprod.notion.site/Proposal-for-Unified-EL-metrics-for-PeerDAS-1d28fc57f54680f2a3cbfe408d7db4b8?pvs=4
There was a problem hiding this comment.
In the prometheus exporter, we replace / by _. So our names are aligned with the Prometheus convention. We can discuss the names, but I feel that they should at least contain the engine prefix. We have a lot of metrics and they are all categorized nicely. Don't want to break this.
eth/catalyst/api.go
Outdated
|
|
||
| for i, hash := range hashes { | ||
| for i, hash := range hashes { |
There was a problem hiding this comment.
This seems wrong whitespacing imo, please go format -ed your files
There was a problem hiding this comment.
Sorry, it seems like I made typo while merging conflicts on the web.
|
Looks like they want to standardize metric names across clients. We should discuss on triage if we want that |
MariusVanDerWijden
force-pushed
the
peerdas-devnet-7
branch
from
ff6e518 to
c192c9d
Compare
Re-opening from MariusVanDerWijden#66