Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
| limiter = rate.NewLimiter(rate.Limit(scConfig.HistoricalProofRateLimit), burst) | ||
| } | ||
| scStore := composite.NewCompositeCommitStore(scDir, logger, scConfig) | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Any specific reason for adding this context?
There was a problem hiding this comment.
The new metrics utility collects metrics periodically in a background goroutine. I use the context to manage that goroutine's lifecycle, so it can tear itself down that goroutine when it is time to do so (e.g. at the end of a unit test).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sei-db/db_engine/pebbledb/db.go
Outdated
| } | ||
|
|
||
| return &pebbleDB{db: db}, nil | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
it looks we want to pass context.Context down through the store constructors, do we want to use context.Background() here?
There was a problem hiding this comment.
Good point, I've piped a context into this method.
yzang2019
left a comment
yzang2019
left a comment
There was a problem hiding this comment.
LGTM, one comment though is whether adding so many metrics will have performance impact, maybe add a flag and only turn on metrics when it's true
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3010 +/- ##
==========================================
+ Coverage 58.27% 58.36% +0.08%
==========================================
Files 2108 2109 +1
Lines 173730 174854 +1124
==========================================
+ Hits 101248 102054 +806
- Misses 63451 63775 +324
+ Partials 9031 9025 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
I think this will be safe performance wise, but I agree it's best to be cautiously defense for this type of thing. Added a config flag we can use to disable this new suite of metrics, if needed. |
Add lots of PebbleDB Metrics <img width="1536" height="1024" alt="image" src="https://github.com/user-attachments/assets/7215f885-ba07-418a-aa82-ab29e04e1309" /> Ran locally --------- Co-authored-by: Cody Littley <cody.littley@seinetwork.io>
3 participants
Describe your changes and provide context
Add lots of PebbleDB Metrics
Testing performed to validate your change
Ran locally