Skip to content

Add BlobMetrics#7622

Merged
fab-10 merged 6 commits intobesu-eth:mainfrom
7suyash7:TransactionPoolMetrics
Sep 19, 2024
Merged

Add BlobMetrics#7622
fab-10 merged 6 commits intobesu-eth:mainfrom
7suyash7:TransactionPoolMetrics

Conversation

@7suyash7
Copy link
Copy Markdown
Contributor

Add support for metrics, besu_blob_storage_cache_size: TransactionPool.cacheForBlobsOfTransactionsAddedToABlock and besu_blob_storage_blob_map_size: TransactionPool.mapOfBlobsInTransactionPool

image

#7578

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
Copy link
Copy Markdown
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Since these metrics are related to the txpool, you can add them to the existing TransactionPoolMetrics class, that should also make the whole PR much smaller, since you do not need to pass around the metrics system.

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
@7suyash7
Copy link
Copy Markdown
Contributor Author

Since these metrics are related to the txpool, you can add them to the existing TransactionPoolMetrics class, that should also make the whole PR much smaller, since you do not need to pass around the metrics system.

@fab-10 I just updated it, can you please take a look.

Comment on lines +112 to +113
private final ConcurrentHashMap<VersionedHash, BlobsWithCommitments.BlobQuad> blobCacheTracker =
new ConcurrentHashMap<>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is this for? it seems to have no real usage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's being used to maintain the count of blobs in the cache...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

better to add a size method to BlobCache, it is simpler and avoid keeping this extra map

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I was trying to avoid doing that... I have added it now and removed the extra map

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
Comment on lines +112 to +113
private final ConcurrentHashMap<VersionedHash, BlobsWithCommitments.BlobQuad> blobCacheTracker =
new ConcurrentHashMap<>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

better to add a size method to BlobCache, it is simpler and avoid keeping this extra map

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
Copy link
Copy Markdown
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, just add a CHANGELOG entry and its done

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
@7suyash7
Copy link
Copy Markdown
Contributor Author

LGTM, just add a CHANGELOG entry and its done

Done!

@fab-10 fab-10 enabled auto-merge (squash) September 19, 2024 13:44
@fab-10 fab-10 merged commit beaee59 into besu-eth:main Sep 19, 2024
daniellehrner pushed a commit to daniellehrner/besu that referenced this pull request Sep 20, 2024
* Add BlobMetrics

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>

* refactor

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>

* remove unused blob_storage

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>

* add .size() to BlobCache

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>

* Add to Changelog

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>

---------

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants