replace eigen logger#3326
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
| import ( | ||
| "context" | ||
| "fmt" | ||
| "log/slog" |
There was a problem hiding this comment.
Thinking it would make sense to use seilog instead
There was a problem hiding this comment.
seilog is an implementation of slog. When this is fully integrated with our stack, it will be using a seilog instance. What benefit do we gain by using the more specific type at the call sites?
There was a problem hiding this comment.
Another consideration. We may eventually want to spin this out into its own repo, potentially for use by outside parties. If we do that, being more generic for the required log framework would be a good thing to do.
There was a problem hiding this comment.
By "implementation of slog" do you mean that it uses slog?
Our existing sei-chain stack uses seilog throughout, if I'm not mistaken. I can appreciate the goal to make this generally useful but I think it'd be good to focus on making it homogenous in our own stack for now. In the worst case, let a user choose their logger and our own stack will just pick seilog.
Seilog would add continuity with our existing patterns. Two key wins I see over slog are hot-reload for log level and module-scoped log messages.
There was a problem hiding this comment.
seilog implements the slog interface, meaning that you can still use a slogger at runtime even if the interfaces use type slog.
I'm open to moving to seilog in the future, but I'd like to keep the scope of this PR focused. The focus for this PR was to incrementally move LittDB towards being clean by removing a third party dependency.
Seilog is currently missing a critical feature that will require nontrivial refactoring to support. Specifically, seilog requires me to set environment variables in order to configure it, which means that I can't configure it at boot time like I do a normal slogger. I can work around this, but it requires a complex multi-binary startup sequence (see cryptosim pattern).
There was a problem hiding this comment.
Makes sense and I'm aligned with using the interface being objectively the right decision.
What ultimately lead me to write that comment was this:
if cfg.LittConfig.Logger == nil { cfg.LittConfig.Logger = slog.Default()
I left the comment on the slog import to broadly discuss how we are injecting our actual log implementation and match existing sei-chain patterns. I now realize I should have been more specific.
If there are feature limitations, can you create a git issue on the seilog package, or wherever it belongs? As long as we're continuously keeping up with a homogenous logging layer over time then I'm happy.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3326 +/- ##
==========================================
- Coverage 59.17% 59.15% -0.03%
==========================================
Files 2097 2094 -3
Lines 172489 171630 -859
==========================================
- Hits 102075 101522 -553
+ Misses 61557 61304 -253
+ Partials 8857 8804 -53
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Describe your changes and provide context
LittDB originally used an in-house logger from Eigen. We obviously don't want to use the Eigen logger. This PR swaps out the logger for a generic
slog.Logger.Note that LittDB is still not compiling automatically with the remainder of the toolchain. To build these changes,
cdto thelittdirectory and runmake build. There are additional third party dependencies to be removed prior to full integration.