perf(log): use sonic json lib #22233
Conversation
tac0turtle
requested review from
a team,
julienrbrt and
sontrinh16
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
![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 and nitpick comments (14)
log/go.mod (2)
11-14: New indirect dependencies addedThe new indirect dependencies appear to be related to the Sonic JSON library and its performance optimizations. This is in line with the PR's objective.
Consider updating
golang.org/x/archto a released version instead of using a specific commit hash:-golang.org/x/arch v0.0.0-20210923205945-b76863e36670 // indirect +golang.org/x/arch v0.7.0 // indirectThis change would make it easier to track and update the dependency in the future.
Also applies to: 17-18
Remaining References to
gotest.tools/v3FoundThe removal of
gotest.tools/v3was not entirely successful. Several test files still importgotest.tools/v3/assertor related packages. Please update these files to remove the dependency and ensure that all tests remain functional withoutgotest.tools/v3.🔗 Analysis chain
Line range hint
1-20: Verify test coverage after removal of gotest.tools/v3The removal of the
gotest.tools/v3dependency suggests changes in the testing approach. Please ensure that all tests are still functional and that no testing capabilities have been lost.Let's verify the test coverage:
Please review the output of these commands to confirm that:
- There are no remaining references to
gotest.tools.- All test files are present and contain test functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of gotest.tools and verify test files # Test 1: Search for any remaining imports of gotest.tools rg --type go 'gotest\.tools' # Test 2: List all test files to ensure they still exist fd --type f --extension go --exec grep -l '^func Test' {}Length of output: 24621
Script:
#!/bin/bash # Description: Corrected script to check for remaining uses of gotest.tools and verify test files # Test 1: Search for any remaining imports of gotest.tools rg --type go 'gotest\.tools' # Test 2: List all test files to ensure they still exist and contain test functions fd --type f --extension go | xargs grep -l '^func Test'Length of output: 28699
log/writer_test.go (3)
22-24: LGTM with suggestion: Consider enhancing error messageThe change to use a standard Go
ifstatement withstrings.Containsis good. It removes the dependency on theassertpackage and usest.Errorfappropriately.Consider enhancing the error message to include the actual output for easier debugging:
if !strings.Contains(buf.String(), "this log line should be displayed") { t.Errorf("expected log line to contain 'this log line should be displayed', but got: %s", buf.String()) }
28-30: LGTM with suggestion: Consider enhancing error messageThe change to use a standard Go
ifstatement withbuf.Len()is good. It's consistent with the previous changes and efficiently checks for an empty buffer.Consider enhancing the error message to include the actual output if there is any:
if buf.Len() != 0 { t.Errorf("expected log line to be filtered, but got unexpected output: %s", buf.String()) }
Line range hint
1-30: Unit test coverage assessmentThe test coverage for the
FilteredWriterfunctionality appears sufficient for the core changes in this pull request. It effectively tests both cases where log lines should be displayed and filtered based on the specified log levels.To further enhance the test coverage, consider adding the following test cases:
- Test with multiple log levels and modules to ensure the filtering works correctly in more complex scenarios.
- Test edge cases, such as empty log messages or invalid module names.
- Test the behavior when changing log levels dynamically, if supported.
These additional tests would provide more comprehensive coverage and increase confidence in the robustness of the logging system.
log/logger_test.go (2)
32-37: Improved error reporting in test assertionsThe changes from
assert.Assertto conditional checks witht.Fatalfimprove the error reporting by providing more specific messages when assertions fail. This enhancement makes it easier to debug test failures.Consider using a helper function to reduce code duplication:
func assertContains(t *testing.T, haystack, needle string) { t.Helper() if !strings.Contains(haystack, needle) { t.Fatalf("expected %q, got: %s", needle, haystack) } }Then use it in the test:
assertContains(t, buf.String(), "mock_message1=true") assertContains(t, buf.String(), "mock_message2=true") assertContains(t, buf.String(), "hello world")This would make the test more concise and easier to maintain.
Also applies to: 42-44
Line range hint
1-44: Enhance test coverage for PR objectivesWhile the existing tests cover the basic functionality of the logger, there are a few areas that could benefit from additional testing to align with the PR objectives:
Performance testing: Consider adding a benchmark test to compare the performance of the new Sonic JSON library with the previous implementation, especially for larger datasets.
Stack trace removal: Add a test to ensure that stack traces are no longer present in the logger output.
JSON serialization: Include a test that verifies the correct JSON serialization of log messages, particularly for complex data structures.
Would you like assistance in drafting these additional tests to ensure comprehensive coverage of the PR changes?
log/level_test.go (6)
23-28: Improved error and nil checks for valid log level parsingThe explicit error and nil checks enhance the test's robustness. The use of
t.Fatalffor the nil check is appropriate as it prevents further execution if the filter is nil.Consider combining the error and nil checks to reduce the number of assertions:
if err != nil || filter == nil { t.Fatalf("unexpected error or nil filter: %v", err) }This change would make the test more concise while maintaining its effectiveness.
30-63: Comprehensive filter behavior checks with improved error reportingThe explicit checks for various log levels and modules enhance the test's clarity and provide more detailed error reporting. Each case is well-covered with specific error messages.
To improve maintainability and reduce repetition, consider refactoring this section to use a table-driven test approach. For example:
testCases := []struct { module string level string expected bool }{ {"consensus", "debug", false}, {"consensus", "info", false}, // ... add all other cases here } for _, tc := range testCases { if filter(tc.module, tc.level) != tc.expected { t.Errorf("expected filter(%s, %s) to return %v", tc.module, tc.level, tc.expected) } }This refactoring would make the test more concise and easier to maintain or extend in the future.
67-72: Consistent error and nil checks for "error" log level parsingThe explicit error and nil checks are appropriate and consistent with the earlier checks in the test. However, this code is very similar to the checks on lines 23-28.
To reduce duplication and improve maintainability, consider extracting this common checking logic into a helper function:
func assertValidFilter(t *testing.T, filter log.Filter, err error) { t.Helper() if err != nil || filter == nil { t.Fatalf("unexpected error or nil filter: %v", err) } } // Usage: filter, err = log.ParseLogLevel(level) assertValidFilter(t, filter, err)This refactoring would make the test more DRY (Don't Repeat Yourself) and easier to maintain.
74-79: Consistent filter behavior checks for "error" log levelThe explicit checks for the "error" log level across different modules are consistent with the earlier filter behavior checks and provide clear, specific error messages.
As suggested earlier, these checks could be incorporated into a table-driven test approach to improve maintainability and reduce repetition. This would allow for easy addition or modification of test cases in the future.
81-89: Comprehensive filter behavior checks for non-error log levelsThe explicit checks for non-error log levels across different modules are thorough and consistent with the earlier filter behavior checks. The error messages are clear and specific.
To maintain consistency and improve maintainability, these checks should also be incorporated into the previously suggested table-driven test approach. This would make the entire test suite more uniform and easier to extend or modify in the future.
Line range hint
1-89: Overall improvement in test clarity and error reportingThe changes in this file significantly enhance the clarity and specificity of error reporting in the tests. By replacing assertions with explicit error checks, the test failures will now provide more detailed and actionable information.
The test coverage remains comprehensive, covering various scenarios including empty input, invalid log levels, and different combinations of modules and log levels. This ensures that the
ParseLogLevelfunction is thoroughly tested.To further improve the test file:
- Consider implementing table-driven tests for the repetitive filter behavior checks.
- Extract common error checking logic into helper functions to reduce duplication.
- Ensure that the
ParseLogLevelfunction's documentation is updated to reflect any changes in behavior or error handling that these tests imply.These changes align well with Go testing best practices and will make the tests more maintainable and easier to extend in the future.
log/logger.go (1)
9-25: Add a comment explaining the necessity of theinitfunction.According to the Uber Go Style Guide,
initfunctions should be used sparingly and must include a comment explaining why the initialization is required in this way. Please add a comment to theinitfunction to provide clarity on its purpose.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
log/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- log/go.mod (1 hunks)
- log/level_test.go (1 hunks)
- log/logger.go (1 hunks)
- log/logger_test.go (1 hunks)
- log/options.go (0 hunks)
- log/writer.go (2 hunks)
- log/writer_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- log/options.go
🧰 Additional context used
📓 Path-based instructions (5)
log/level_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"log/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.log/logger_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"log/writer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.log/writer_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (7)
log/go.mod (1)
6-6: Approved: Sonic JSON library integration for performance improvementThe replacement of
github.com/pkg/errorswithgithub.aaakk.us.kg/bytedance/sonicaligns with the PR objective to improve JSON handling performance. This change is expected to be beneficial, especially for larger datasets.However, this change might have implications on error handling throughout the codebase. Let's verify the impact:
log/writer_test.go (1)
16-18: LGTM: Improved error handlingThe change to use
if err != nilwitht.Fatalfis a good improvement. It follows idiomatic Go error handling and provides clear error reporting that will stop the test if the log level parsing fails.log/writer.go (2)
6-7: LGTM: Import statement change is correct and follows style guidelines.The addition of the Sonic JSON library import and the removal of the standard
encoding/jsonpackage (not visible in the diff) align with the PR objective. The import statement follows the Uber Golang style guide with grouped imports and a blank line between standard and third-party packages.
32-32: LGTM: Sonic JSON library integration looks good. Consider verifying performance improvements.The replacement of
json.Unmarshalwithsonic.Unmarshalis correct and aligns with the PR objective. The error handling remains appropriate, and the overall logic of theWritemethod is preserved.To ensure the performance benefits, especially for larger JSON payloads, consider running benchmarks. Here's a script to help verify the performance improvement:
log/level_test.go (2)
11-13: Improved error checking for empty log levelThe replacement of the assertion with an explicit error check enhances the clarity of the test. The error message is descriptive and follows Go testing conventions.
17-19: Enhanced error checking for invalid log levelThe explicit error check improves the test's clarity. The error message is detailed, including the specific invalid input, which aids in debugging.
log/logger.go (1)
17-23: Verify thatsonic.Marshalcorrectly handlesjson.Marshalerandencoding.TextMarshalerinterfaces.While replacing
json.Marshalwithsonic.Marshalcan improve performance, it's important to ensure thatsonic.Marshalproperly respects thejson.Marshalerandencoding.TextMarshalerinterfaces. Differences in handling these interfaces could lead to unexpected serialization results.Run the following script to search for types implementing
json.Marshalerorencoding.TextMarshalerand verify their serialization:This will help identify if such types exist in the codebase and whether additional testing is needed to ensure compatibility with
sonic.Marshal.
log/logger.go
Outdated
| "github.com/bytedance/sonic" | ||
| "github.com/rs/zerolog" | ||
| "github.com/rs/zerolog/pkgerrors" | ||
| ) | ||
|
|
||
| func init() { | ||
| zerolog.InterfaceMarshalFunc = func(i any) ([]byte, error) { | ||
| switch v := i.(type) { | ||
| case json.Marshaler: | ||
| return json.Marshal(i) | ||
| return sonic.Marshal(i) | ||
| case encoding.TextMarshaler: | ||
| return json.Marshal(i) | ||
| return sonic.Marshal(i) | ||
| case fmt.Stringer: | ||
| return json.Marshal(v.String()) | ||
| return sonic.Marshal(v.String()) | ||
| default: | ||
| return json.Marshal(i) | ||
| return sonic.Marshal(i) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication between init and WithJSONMarshal.
The init function and WithJSONMarshal both set zerolog.InterfaceMarshalFunc with similar logic. To adhere to the DRY (Don't Repeat Yourself) principle from best practices, consider refactoring the init function to call WithJSONMarshal(sonic.Marshal). This would reduce code duplication and improve maintainability.
Apply this diff to refactor the code:
func init() {
- zerolog.InterfaceMarshalFunc = func(i any) ([]byte, error) {
- switch v := i.(type) {
- case json.Marshaler:
- return sonic.Marshal(i)
- case encoding.TextMarshaler:
- return sonic.Marshal(i)
- case fmt.Stringer:
- return sonic.Marshal(v.String())
- default:
- return sonic.Marshal(i)
- }
- }
+ WithJSONMarshal(sonic.Marshal)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/bytedance/sonic" | |
| "github.com/rs/zerolog" | |
| "github.com/rs/zerolog/pkgerrors" | |
| ) | |
| func init() { | |
| zerolog.InterfaceMarshalFunc = func(i any) ([]byte, error) { | |
| switch v := i.(type) { | |
| case json.Marshaler: | |
| return json.Marshal(i) | |
| return sonic.Marshal(i) | |
| case encoding.TextMarshaler: | |
| return json.Marshal(i) | |
| return sonic.Marshal(i) | |
| case fmt.Stringer: | |
| return json.Marshal(v.String()) | |
| return sonic.Marshal(v.String()) | |
| default: | |
| return json.Marshal(i) | |
| return sonic.Marshal(i) | |
| } | |
| } | |
| "github.com/bytedance/sonic" | |
| "github.com/rs/zerolog" | |
| ) | |
| func init() { | |
| WithJSONMarshal(sonic.Marshal) | |
| } |
tac0turtle
force-pushed
the
marko/logger_sonic
branch
from
a55a562 to
232c7ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
log/logger.go (4)
9-9: LGTM! Consider removing unused import.The addition of the
github.com/bytedance/sonicimport aligns with the PR objective to use a more performant JSON library. However, it appears that thegithub.aaakk.us.kg/pkg/errorsimport is no longer used in this file.Consider removing the unused import:
- "github.com/pkg/errors"
19-25: LGTM! Consider refactoring to reduce duplication.The changes consistently replace
json.Marshalwithsonic.Marshal, which aligns with the PR objective to use the Sonic JSON library for better performance.To reduce code duplication, consider refactoring the
initfunction to use a singlesonic.Marshalcall:func init() { zerolog.InterfaceMarshalFunc = func(i any) ([]byte, error) { return sonic.Marshal(i) } }This simplification is possible because
sonic.Marshalcan handle all the types currently being checked.
Line range hint
77-89: Consider refactoring to eliminate code duplication.The
WithJSONMarshalfunction now duplicates the logic in theinitfunction. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring both functions to use a common implementation.Here's a suggested refactor:
- Create a new function:
func setInterfaceMarshalFunc(marshaler func(v any) ([]byte, error)) { zerolog.InterfaceMarshalFunc = func(i any) ([]byte, error) { return marshaler(i) } }
- Update the
initfunction:func init() { setInterfaceMarshalFunc(sonic.Marshal) }
- Update the
WithJSONMarshalfunction:func WithJSONMarshal(marshaler func(v any) ([]byte, error)) { setInterfaceMarshalFunc(marshaler) }This refactoring will reduce code duplication and improve maintainability.
116-116: LGTM! Update documentation to reflect removed feature.The removal of the stack trace functionality aligns with the PR objective and simplifies the logger configuration.
Please ensure that any documentation referring to the stack trace feature is updated to reflect its removal. This includes:
- Updating the package documentation if it mentions stack trace functionality.
- Removing any references to
StackTracein theOptiontype or related configuration structures.- Updating any examples or user guides that might reference this feature.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
log/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- log/go.mod (1 hunks)
- log/logger.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- log/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
log/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
This pr swaps the json library to a more performant one. The difference is not drastic on small values, but on larger values they will be, i.e. consensus data from comet.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit