RSDB: metrics and performance improvements #2184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Coverage from tests in coverage: 48.6% of statements across all listed packagescoverage: 59.8% of statements in consensus/istanbul coverage: 42.7% of statements in consensus/istanbul/announce coverage: 55.7% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 58.1% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.4% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2184 +/- ##
==========================================
+ Coverage 55.05% 55.18% +0.12%
==========================================
Files 679 680 +1
Lines 114336 114370 +34
==========================================
+ Hits 62950 63116 +166
+ Misses 47495 47378 -117
+ Partials 3891 3876 -15
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
carterqw2
requested review from
a team and
karlb
and removed request for
a team
hbandura
suggested changes
Sep 8, 2023
| rsdb.rsRLPMeter = metrics.NewRegisteredMeter(namespace+"rs/rlp/encoding/size", nil) | ||
| rsdb.rsRLPEncTimer = metrics.NewRegisteredTimer(namespace+"rs/rlp/encoding/duration", nil) | ||
| rsdb.rsDbSaveTimer = metrics.NewRegisteredTimer(namespace+"rs/db/save/time", nil) | ||
| rsdb.rcvdRLPMeter = metrics.NewRegisteredMeter(namespace+"rcvd/rlp/encoding/size", nil) |
There was a problem hiding this comment.
I remember we used rcvd in the past for tests. I wonder if there's name for this that makes it a bit clearer for a reader. As in , the entry for the collection of received consensus messages.
There was a problem hiding this comment.
The type struct is called rcvd, so I thought I would just follow the convention. I also updated the comment next to the definition of the metric. Would it be enough or you want to change it here? If so, what exactly you would like to change, the name of the metric or the path or both?
There was a problem hiding this comment.
Yes, iirc I named that struct and I regret it highly xD. Maybe it's ok, I just felt that if you read the metric name, without the context of it being in this specific piece of code, it's really tough to understand what it is. That being said, it's probably the same with other metrics, so maybe it's ok.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reuses the existing LevelDB implementation from the ethdb/leveldb module to collect LevelDB metrics for Roundstate DB as well and introduces additional metrics related to RLP-encoding.
Other changes
Tested
Tested on the testnet.
Backwards compatibility
No breaking changes.