refactor(v2): use non global logger #1037
Conversation
WalkthroughThis pull request introduces a comprehensive logging refactoring for the IAVL v2 project. The changes involve replacing the existing Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
v2/multitree.go (1)
Line range hint
31-40: Initialize the logger field in NewMultiTree constructor.The constructor doesn't initialize the
loggerfield that was added to theMultiTreestruct.Apply this diff to initialize the logger:
func NewMultiTree(logger Logger, rootPath string, opts TreeOptions) *MultiTree { return &MultiTree{ Trees: make(map[string]*Tree), doneCh: make(chan saveVersionResult, 1000), errorCh: make(chan error, 1000), treeOpts: opts, pool: NewNodePool(), rootPath: rootPath, + logger: logger, } }
🧹 Nitpick comments (4)
v2/logger.go (2)
26-36: Consider adding unexported constructor for noopLoggerWhile the implementation is correct, consider adding an unexported constructor function to ensure proper initialization, even though the struct is empty.
type noopLogger struct{} +func newNoopLogger() *noopLogger { + return &noopLogger{} +} + func (l *noopLogger) Info(string, ...any) {}
38-55: Consider adding configuration options for testLoggerThe testLogger implementation directly forwards to slog without any configuration options. Consider adding options for:
- Log level filtering
- Output destination
- Formatting preferences
🧰 Tools
🪛 golangci-lint (1.62.2)
46-46: File is not
gofumpt-ed(gofumpt)
49-49: File is not
gofumpt-ed(gofumpt)
52-52: File is not
gofumpt-ed(gofumpt)
v2/sqlite_batch.go (1)
128-132: Consider structured logging for metricsWhile the Debug level is appropriate for performance metrics, consider using structured logging instead of string formatting for better machine readability:
-b.logger.Debug(fmt.Sprintf("db=tree count=%s dur=%s batch=%d rate=%s", - humanize.Comma(b.treeCount), - time.Since(b.treeSince).Round(time.Millisecond), - batchSize, - humanize.Comma(int64(float64(batchSize)/time.Since(b.treeSince).Seconds())))) +b.logger.Debug("tree batch metrics", + "db", "tree", + "count", humanize.Comma(b.treeCount), + "duration", time.Since(b.treeSince).Round(time.Millisecond), + "batch_size", batchSize, + "rate", humanize.Comma(int64(float64(batchSize)/time.Since(b.treeSince).Seconds())))Also applies to: 247-249
v2/snapshot.go (1)
Line range hint
279-283: Pre-allocate the versions slice.Pre-allocating the slice with the known size will improve performance by avoiding reallocations.
Apply this diff to pre-allocate the slice:
- var versions []int64 // where is this used? + versions := make([]int64, 0, len(uniqueVersions)) // Pre-allocate with known size for v := range uniqueVersions { versions = append(versions, v) }🧰 Tools
🪛 golangci-lint (1.62.2)
279-279: Consider pre-allocating
versions(prealloc)
281-281: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
v2/cmd/gen/gen.gois excluded by!**/gen/**v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
v2/CHANGELOG.md(1 hunks)v2/README.md(1 hunks)v2/cmd/rollback/rollback.go(2 hunks)v2/cmd/snapshot/snapshot.go(2 hunks)v2/go.mod(1 hunks)v2/logger.go(1 hunks)v2/multitree.go(7 hunks)v2/snapshot.go(14 hunks)v2/sqlite.go(19 hunks)v2/sqlite_batch.go(3 hunks)v2/sqlite_test.go(4 hunks)v2/sqlite_writer.go(13 hunks)v2/tree.go(0 hunks)v2/tree_test.go(8 hunks)
💤 Files with no reviewable changes (1)
- v2/tree.go
✅ Files skipped from review due to trivial changes (2)
- v2/README.md
- v2/CHANGELOG.md
🧰 Additional context used
🪛 golangci-lint (1.62.2)
v2/snapshot.go
279-279: Consider pre-allocating versions
(prealloc)
v2/logger.go
46-46: File is not gofumpt-ed
(gofumpt)
49-49: File is not gofumpt-ed
(gofumpt)
52-52: File is not gofumpt-ed
(gofumpt)
🔇 Additional comments (12)
v2/logger.go (1)
5-24: Well-designed logging interface!The Logger interface is clean, minimal, and follows good practices:
- Clear documentation mentioning compatibility with Cosmos SDK
- Consistent method signatures across all logging levels
- Strong typing with string keys requirement
v2/cmd/rollback/rollback.go (1)
10-10: LGTM! Clean logger integrationThe logger initialization and usage are correct. The Info level is appropriate for this operational message.
Also applies to: 26-26
v2/cmd/snapshot/snapshot.go (1)
10-10: LGTM! Clean logger integrationThe logger initialization and usage are correct. The Info level is appropriate for this operational message.
Also applies to: 25-25
v2/sqlite_batch.go (1)
15-15: LGTM! Clean logger field updateThe logger field type change from zerolog.Logger to Logger is correct and aligns with the new logging interface.
v2/sqlite_test.go (1)
Line range hint
126-138: LGTM!The logging changes in the test file look good. Using
t.Logandt.Logfis the correct approach for test logging.v2/tree_test.go (1)
175-175: LGTM! Logger initialization is consistent across test functions.The changes consistently add
NewTestLogger()to allNewMultiTreeandImportMultiTreecalls, properly implementing the new non-global logger requirement.Also applies to: 193-193, 208-208, 221-221, 235-235, 265-265, 287-287, 630-630
v2/sqlite.go (5)
28-29: LGTM! Logger fields properly added to structs.The Logger field is appropriately added to both SqliteDbOptions and SqliteDb structs, maintaining consistency with the new logging architecture.
Also applies to: 55-55
69-71: LGTM! Proper logger initialization in default options.The logger is correctly initialized with a no-op logger by default, following the principle of providing sensible defaults.
139-139: LGTM! Logger properly initialized in constructor.The logger is correctly initialized from the options in the NewSqliteDb constructor.
1124-1126: LGTM! Logger accessor method added.The Logger() method provides proper access to the logger instance, following good encapsulation practices.
91-92: Verify log level usage across logging statements.The logging statements use various levels (Info, Debug, Warn). Consider establishing consistent guidelines for when to use each log level.
Run this script to analyze log level usage patterns:
Also applies to: 185-185, 215-215, 435-435, 603-603, 681-681, 705-705, 709-709, 868-868, 947-947, 954-954, 958-958, 959-959, 1020-1021, 1032-1033, 1043-1043, 1070-1074
v2/go.mod (1)
25-25: Verify compatibility with updated dependencies.The following dependencies have been updated:
- github.com/mattn/go-isatty v0.0.20
- github.com/rs/zerolog v1.33.0
- golang.org/x/sys v0.22.0
Please ensure these updates don't introduce any breaking changes.
Run this script to check for any compatibility issues:
Also applies to: 31-31, 33-33
✅ Verification successful
Dependencies update is safe to proceed
The version changes are all minor or patch updates following semantic versioning, indicating backward compatibility:
- zerolog: v1.30.0 -> v1.33.0 (minor)
- isatty: v0.0.19 -> v0.0.20 (patch)
- sys: v0.13.0 -> v0.28.0 (minor)
Additionally, these are all indirect dependencies, which means they're not directly used in the codebase but are required by other dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes in updated dependencies # Test: Look for any major version bumps or breaking changes echo "Checking go.sum for previous versions:" rg "github.com/rs/zerolog" go.sum rg "github.com/mattn/go-isatty" go.sum rg "golang.org/x/sys" go.sumLength of output: 1518
Script:
#!/bin/bash # Search for imports and usage of the updated packages echo "Checking for imports and usage:" rg "github.com/rs/zerolog" -l rg "github.com/mattn/go-isatty" -l echo -e "\nChecking git history for previous versions:" git log -p -- go.mod | grep -A 5 -B 5 "github.com/rs/zerolog\|github.com/mattn/go-isatty"Length of output: 396
Script:
#!/bin/bash # Check actual usage patterns and version constraints echo "Checking version constraints in all modules:" for f in $(fd go.mod$); do echo -e "\n=== $f ===" grep -A 1 -B 1 "github.com/rs/zerolog\|github.com/mattn/go-isatty\|golang.org/x/sys" "$f" done echo -e "\nChecking actual usage patterns:" rg "zerolog\." -B 1 -A 1 rg "isatty\." -B 1 -A 1Length of output: 2595
| w.logger.Error("tree loop failed", "error", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Replace os.Exit with error return.
Using os.Exit in a library is not recommended as it:
- Prevents proper cleanup
- Doesn't allow the caller to handle the error
- Makes testing difficult
Apply this diff to return errors instead:
func (w *sqlWriter) start(ctx context.Context) {
+ errCh := make(chan error, 2)
go func() {
err := w.treeLoop(ctx)
if err != nil {
w.logger.Error("tree loop failed", "error", err)
- os.Exit(1)
+ errCh <- fmt.Errorf("tree loop failed: %w", err)
}
}()
go func() {
err := w.leafLoop(ctx)
if err != nil {
w.logger.Error("leaf loop failed", "error", err)
- os.Exit(1)
+ errCh <- fmt.Errorf("leaf loop failed: %w", err)
}
}()
+
+ select {
+ case err := <-errCh:
+ return err
+ case <-ctx.Done():
+ return ctx.Err()
+ }
}Also applies to: 68-69
| // Logger defines basic logger that IAVL expects. | ||
| // It is a subset of the cosmossdk.io/core/log.Logger interface. | ||
| // cosmossdk.io/log/log.Logger implements this interface. | ||
| type Logger interface { |
There was a problem hiding this comment.
why not depend on .io/core/log?
ref: cosmos/cosmos-sdk#23326
Summary by CodeRabbit
New Features
Improvements
zerologwith a custom logger interfaceDependency Updates
zerolog,go-isatty, andgolang.org/x/sysDocumentation