-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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 for count and size of blob files #13614
Conversation
958b967
to
d3c7f4f
Compare
d3c7f4f
to
8b5cb68
Compare
@@ -102,7 +102,9 @@ func (bs *BlobStorage) Save(sidecar blocks.VerifiedROBlob) error { | |||
return nil | |||
} | |||
if bs.pruner != nil { | |||
bs.pruner.notify(sidecar.BlockRoot(), sidecar.Slot()) | |||
if err := bs.pruner.notify(sidecar.BlockRoot(), sidecar.Slot(), sidecar.Index); err != nil { | |||
return errors.Wrapf(err, "problem maintaining pruning cache/metrics for sidecar with root=%#x", sidecar.BlockRoot()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the downstream effect if this returns an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wouldn't write the sidecar. The only error condition is if the index is out of bounds, which would be a panic. I was on the fence about returning an error here, because we should never have a VerifiedROBlob with an index out of bounds, so this should be logically unreachable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I meant to say upstream. I think block processing will fail, but that feels sane to me. If we cant write a blob to disk then we shouldn't be allowed to write a block anyway
beacon-chain/db/filesystem/pruner.go
Outdated
fname = path.Base(fname) | ||
parts := strings.Split(fname, ".") | ||
if len(parts) != 2 { | ||
return 0, errors.New("not a blob ssz file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone could in theory have some other file with 2 parts that isn't an ssz. Wouldn't it be safer to check if it has the extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called downstream of the ssz filter (scFiles := filter(entries, filterSsz)
), but we could double-check it here.
Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Adds 2 new gauge metrics,
blob_disk_count
,blob_disk_bytes
to count the number and size of blob sidecars on disk.Also changes the spelling of
blobs_written
-> singularblobs_written
to be consistent with the other blob metrics.Which issues(s) does this PR fix?
Fixes #13613