-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: add use_local_time_encoder and show_caller config #59
base: master
Are you sure you want to change the base?
feature: add use_local_time_encoder and show_caller config #59
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve significant updates to the logging configuration and functionality within the Changes
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
config.go (2)
98-103
: Enhance field documentation for better clarity.The documentation comments could be more descriptive to help users understand the implications of these settings.
Consider updating the comments as follows:
- // UseLocalTimeEncoder is used to set the encoder to use local time instead of UTC. + // UseLocalTimeEncoder when set to true, formats log timestamps using local time zone + // instead of UTC. This can make logs more readable in production environments + // where local time is preferred. Default is false (UTC). - // ShowCaller is used to set the encoder to show the caller. + // ShowCaller when set to true, includes the caller's file name and line number + // in log entries. This aids in debugging by showing where each log entry + // originated from. Default is false.
143-146
: Consider refactoring duplicate encoder configuration logic.The time encoder and caller key configuration logic is repeated across different modes. This could be refactored to reduce code duplication.
Consider extracting the common configuration into a helper function:
+func (cfg *Config) getEncoderConfig() (timeEncoder zapcore.TimeEncoder, callerKey string) { + if cfg.UseLocalTimeEncoder { + timeEncoder = localTimeEncoder + } else { + // Use the appropriate default encoder based on mode + switch Mode(strings.ToLower(string(cfg.Mode))) { + case production: + timeEncoder = utcEpochTimeEncoder + default: + timeEncoder = utcISO8601TimeEncoder + } + } + + callerKey = zapcore.OmitKey + if cfg.ShowCaller { + callerKey = "caller" + } + + return timeEncoder, callerKey +}Then use it in BuildLogger:
timeEncoder, callerKey := cfg.getEncoderConfig() // Use these values in EncoderConfig for each modeAlso applies to: 155-155, 162-162, 181-184, 192-192, 199-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
config.go
(5 hunks)
🔇 Additional comments (2)
config.go (2)
109-120
: LGTM! Clean implementation of the new configuration options.
The changes properly implement both the caller information and time encoding options in production mode. The code maintains good separation of concerns and follows the existing pattern.
Also applies to: 129-129, 135-135
98-103
: Verify impact on existing log processing pipelines.
The addition of caller information and changes in time format might affect existing log parsing tools or monitoring systems.
Let's check for any log format dependencies in the codebase:
✅ Verification successful
No impact expected on log processing pipelines
Based on the codebase analysis:
- The schema.json shows the logger supports only two standard encodings: "console" and "json"
- The new options (UseLocalTimeEncoder and ShowCaller) are standard zap logger features that don't alter the core JSON structure
- No custom log parsing or processing logic was found that depends on specific time formats or caller information
- The file logger configuration only handles rotation and basic output settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential log parsing or processing code that might be affected by format changes
# Look for log parsing patterns
echo "Searching for log parsing patterns..."
rg -l 'time\.Parse|Parse.*Time|regexp.*ts.*=|regexp.*time.*='
# Look for monitoring or log processing configuration files
echo "Searching for monitoring configurations..."
fd -e yaml -e yml -e json | xargs rg -l 'logstash|elasticsearch|splunk|fluentd|prometheus'
Length of output: 336
Script:
#!/bin/bash
# Let's try a different approach to find potential log format dependencies
# Search for log-related code and configurations
echo "Searching for logging configurations and usage..."
rg -l "log.*format|log.*time|log.*caller"
# Search for test files that might depend on log output format
echo "Searching for test files with log assertions..."
rg -l "assert.*log|expect.*log"
# Look for any custom log processing or parsing code
echo "Searching for custom log processing..."
rg -l "parse.*log|process.*log"
# Search for logging initialization or setup code
echo "Searching for logging setup code..."
ast-grep --pattern 'NewLogger($$$)'
Length of output: 616
Script:
#!/bin/bash
# Let's examine the specific content of the relevant files
# Check schema.json for log format specifications
echo "Checking schema.json for log format dependencies:"
cat schema.json
# Check the logger initialization in plugin.go
echo "Checking logger initialization in plugin.go:"
rg -A 10 -B 10 "NewLogger" plugin.go
# Check config.go for log processing
echo "Checking config.go for log processing logic:"
rg "parse.*log|process.*log" config.go -A 5 -B 5
Length of output: 5598
config.go
Outdated
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | ||
enc.AppendString(t.Local().Format("2006-01-02 15:04:05.000")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider maintaining consistent time format across encoders.
The localTimeEncoder
uses a different time format compared to utcISO8601TimeEncoder
. This inconsistency might cause issues with log parsing tools that expect a consistent format.
Consider using the same ISO8601 format for consistency:
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) {
- enc.AppendString(t.Local().Format("2006-01-02 15:04:05.000"))
+ enc.AppendString(t.Local().Format("2006-01-02T15:04:05-0700"))
}
📝 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.
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | |
enc.AppendString(t.Local().Format("2006-01-02 15:04:05.000")) | |
} | |
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | |
enc.AppendString(t.Local().Format("2006-01-02T15:04:05-0700")) | |
} |
UseLocalTimeEncoder bool `mapstructure:"use_local_time_encoder"` | ||
|
||
// ShowCaller is used to set the encoder to show the caller. | ||
ShowCaller bool `mapstructure:"show_caller"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When developing custom plugins, it was difficult to find the corresponding location in the log, so caller was added
In the production environment, the log time format is not readable enough, so the local time display function is added
I'm not sure if I understand this. The caller is the line where your log is. How is that difficult to find where you send this log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When developing custom plugins, although logs show what happened, pinpointing the exact source of a log entry in complex plugin code can be challenging without caller information. By enabling show_caller, we can immediately see which file and line generated the log, significantly improving debugging efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can it be complex? It is just ctrl+f (or / for vim) -> enter log message -> here you are.
config.go
Outdated
encodeTime := utcEpochTimeEncoder | ||
if cfg.UseLocalTimeEncoder { | ||
encodeTime = localTimeEncoder | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local time is often the source of the errors when using different time zones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For projects that operate within a single timezone (which is common for many applications), local time provides more intuitive logging without the complexity of timezone management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is fair. I agree with that change, but tests should be added + docs + shorter name in the configuration.
config.go
Outdated
@@ -95,15 +95,29 @@ type Config struct { | |||
|
|||
// File logger options | |||
FileLogger *FileLoggerConfig `mapstructure:"file_logger_options"` | |||
|
|||
// UseLocalTimeEncoder is used to set the encoder to use local time instead of UTC. | |||
UseLocalTimeEncoder bool `mapstructure:"use_local_time_encoder"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll think about this, but atm, this name is too long, I suggest to use smt like: use_local_time
.
Hey @shellphy 👋
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #59 +/- ##
===========================================
+ Coverage 0 44.57% +44.57%
===========================================
Files 0 13 +13
Lines 0 866 +866
===========================================
+ Hits 0 386 +386
- Misses 0 449 +449
- Partials 0 31 +31 ☔ View full report in Codecov by Sentry. |
config.go
Outdated
@@ -251,3 +273,7 @@ func utcISO8601TimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | |||
func utcEpochTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | |||
enc.AppendInt64(t.UTC().UnixNano()) | |||
} | |||
|
|||
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { | |||
enc.AppendString(t.Local().Format("2006-01-02 15:04:05.000")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, ISO8601 encode format should be used here to be in sync with the current encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/configs/.rr-file-logger.yaml (1)
24-24
: Consider renaming to match code implementation.The config key
use_local_time
appears to map to theUseLocalTime
field in theConfig
struct. For consistency, consider using eitheruse_local_time_encoder
oruseLocalTimeEncoder
depending on your YAML naming conventions.encoder.go (1)
28-29
: Consider making the padding width configurable.While increasing the padding to 20 characters accommodates longer service names and potentially the new caller information, hardcoding this value might be limiting. Consider making it configurable through the logger config to provide more flexibility.
+const defaultNamePadding = 20 func ColoredNameEncoder(s string, enc zapcore.PrimitiveArrayEncoder) { - if len(s) < 20 { - s += strings.Repeat(" ", 20-len(s)) + if len(s) < defaultNamePadding { + s += strings.Repeat(" ", defaultNamePadding-len(s)) }tests/logger_test.go (1)
341-349
: Enhance test coverage and regex patterns.The test assertions correctly verify the new logging features. However, consider these improvements:
- Make the time format regex more precise:
-`\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}.\d{3}` +`\d{4}-(0[1-9]|1[0-2])-(0[1-9]|[12]\d|3[01]) ([01]\d|2[0-3]):[0-5]\d:[0-5]\d\.\d{3}`
- Make the caller info regex more specific:
-`\w+\.go:\d+` +`[a-zA-Z0-9_]+\.go:\d+`
- Add test cases for when these features are disabled:
// Test when use_local_time=false assert.False(t, matched, "local time format should not be used when disabled") // Test when show_caller=false assert.False(t, callerMatched, "caller info should not be shown when disabled")config.go (1)
98-103
: Consider shorter configuration field names.As suggested in the past review comments, consider using shorter field names in the configuration:
- UseLocalTime bool `mapstructure:"use_local_time"` + UseLocalTime bool `mapstructure:"local_time"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sum
is excluded by!**/*.sum
📒 Files selected for processing (4)
config.go
(6 hunks)encoder.go
(1 hunks)tests/configs/.rr-file-logger.yaml
(1 hunks)tests/logger_test.go
(2 hunks)
🔇 Additional comments (10)
encoder.go (1)
28-29
: Verify impact on terminal display.
The increased padding width might affect log readability in terminals with limited width. Let's verify the typical service name lengths in the codebase to ensure this change is appropriate.
✅ Verification successful
Let me gather more specific information about the service names and padding usage.
Based on the gathered information, I can now generate the final response:
20-character padding for log names is appropriate
The codebase analysis shows that:
- The padding is only used in
ColoredNameEncoder
for formatting log names - Test cases primarily use short names like "error" and basic field values
- No evidence of service names exceeding or getting close to 20 characters
- The padding implementation is isolated to the encoder and consistently applied
The 20-character padding provides sufficient space for service names while maintaining readable log formatting in standard terminal widths (80+ columns).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find service name usage patterns to verify if 20-char padding is sufficient
# Look for logger initialization patterns to analyze service name lengths
# Search for logger initialization patterns
echo "Searching for logger initialization patterns..."
rg -A 2 'NewLogger|InitLogger|WithName|GetLogger'
# Search for potential service name constants or variables
echo "Searching for potential service name definitions..."
ast-grep --pattern 'const $name = "$value"'
Length of output: 519
Script:
#!/bin/bash
# Search for actual service name strings in log statements
echo "Searching for log statements with service names..."
rg -B 2 '\.Info\(|\.Debug\(|\.Error\(|\.Warn\('
# Look for the specific padding implementation usage
echo "Searching for padding usage..."
rg -B 2 'strings\.Repeat.*\s+.*\)'
# Search for struct field definitions that might contain service names
echo "Looking for relevant struct fields..."
ast-grep --pattern 'type $_ struct {
$$$
Name string
$$$
}'
Length of output: 2694
tests/logger_test.go (2)
9-9
: LGTM: Import addition is appropriate.
The regexp
package import is correctly added and necessary for the new test assertions.
341-349
: Verify configuration file setup.
Please ensure that the test configuration file tests/configs/.rr-file-logger.yaml
contains the new options:
✅ Verification successful
Configuration file contains all required options
The configuration file tests/configs/.rr-file-logger.yaml
exists and contains the necessary options being tested:
use_local_time: true
show_caller: true
The test cases correctly verify these configuration options by checking the log output format against the expected patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the test configuration file exists and contains the new options
echo "Checking test configuration file..."
if [ -f "tests/configs/.rr-file-logger.yaml" ]; then
echo "Content of .rr-file-logger.yaml:"
cat "tests/configs/.rr-file-logger.yaml"
echo -e "\nVerifying presence of new options:"
grep -A 2 "logs:" "tests/configs/.rr-file-logger.yaml"
else
echo "Error: Configuration file not found!"
fi
Length of output: 1022
config.go (7)
109-112
: LGTM! Clean implementation of the caller key configuration.
The conditional setting of the caller key is implemented correctly, defaulting to OmitKey when ShowCaller is false.
117-120
: LGTM! Production mode changes are well-integrated.
The conditional time encoder selection and caller key integration are implemented correctly for production mode.
Also applies to: 129-129, 135-135
143-146
: LGTM! Development mode changes mirror production mode structure.
The implementation maintains consistency with the production mode while using appropriate development-specific encoders.
Also applies to: 155-155, 162-162
181-184
: LGTM! Default mode changes follow consistent pattern.
The implementation maintains consistency with other modes while preserving default mode specifics.
Also applies to: 192-192, 199-199
249-251
: LGTM! File logger caller support is properly implemented.
The conditional addition of caller information is correctly implemented for file logger configuration.
280-282
: Maintain consistent time format.
Referring to the previous review comment about maintaining consistent ISO8601 format across encoders.
Line range hint 1-282
: Address missing requirements.
As mentioned in the PR comments, please ensure:
- Add tests for the new configuration options
- Submit documentation updates to the roadrunner-server/docs repository
Would you like help with:
- Generating test cases for the new configuration options?
- Creating documentation for the new features?
✅ Verification successful
Tests and documentation are already in place.
The review comment's concerns have been addressed:
-
Tests for the new configuration options (
use_local_time
andshow_caller
) already exist intests/logger_test.go
:TestFileLogger
test case verifies both features:- Local time format verification via regex
\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}.\d{3}
- Caller info verification via regex
\w+\.go:\d+
- Local time format verification via regex
-
Documentation is maintained in the external docs repository, as indicated by the README.md which points to the official documentation at https://docs.roadrunner.dev/logging-and-observability/logger
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test files related to the new config options
rg -l "TestConfig.*(?:LocalTime|ShowCaller)" --type go
Length of output: 56
Script:
#!/bin/bash
# Let's check for any existing test files in the same directory
fd ".*_test.go" --type f --full-path --base-directory .
# Also check for any test cases that might be testing the logger configuration
rg -l "Test.*Config" --type go
# Check if there are any existing documentation files
fd "README.md|DOCS.md|docs" --type f
Length of output: 197
Script:
#!/bin/bash
# Let's examine the existing test file to see what's already covered
cat tests/logger_test.go
# Also check the README for any documentation guidelines
cat README.md
Length of output: 7802
use_local_time: true | ||
show_caller: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Documentation needed for new logging configuration options
The new configuration options use_local_time
and show_caller
are present in the test configuration but are not documented in the schema. These options should be added to the schema definition under the FileLoggerOptions
section to maintain consistency and provide proper documentation for users.
- Add documentation for
use_local_time
in the schema - Add documentation for
show_caller
in the schema
🔗 Analysis chain
LGTM! Well-structured configuration.
The new logging options are correctly placed under the logs
section and follow the existing configuration structure. The boolean values are appropriate for these feature flags.
Let's verify the configuration schema documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these new options are documented in the schema or docs
# Expected: Find references to these new config options in documentation
# Look for schema definitions or documentation
rg -g '*.{md,json,yaml}' -i "use.*local.*time|show.*caller"
Length of output: 174
Script:
#!/bin/bash
# Let's check for any documentation or schema files that might define logger configuration
fd -e md -e yaml -e json . | grep -i "schema\|config\|logger" | xargs cat 2>/dev/null | grep -i "logger\|log"
Length of output: 2239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
config.go (4)
110-115
: Remove redundant nil initialization.The
nil
initialization forencodeCaller
is redundant as it's the zero value for interface types.-var encodeCaller zapcore.CallerEncoder = nil +var encodeCaller zapcore.CallerEncoder🧰 Tools
🪛 golangci-lint
[warning] 111-111: var-declaration: should drop = nil from declaration of var encodeCaller; it is the zero value
(revive)
292-298
: Extract magic numbers into named constants.The padding width of 40 characters is hard-coded in both caller encoders. This should be a named constant for better maintainability and configurability.
+const ( + // CallerPaddingWidth defines the fixed width for caller path padding + CallerPaddingWidth = 40 +) func ColoredShortCallerEncoder(caller zapcore.EntryCaller, enc zapcore.PrimitiveArrayEncoder) { - enc.AppendString(fmt.Sprintf("\x1b[35m%-40s\x1b[0m", caller.TrimmedPath())) + enc.AppendString(fmt.Sprintf("\x1b[35m%-*s\x1b[0m", CallerPaddingWidth, caller.TrimmedPath())) } func ShortCallerEncoderWithPadding(caller zapcore.EntryCaller, enc zapcore.PrimitiveArrayEncoder) { - enc.AppendString(fmt.Sprintf("%-40s", caller.TrimmedPath())) + enc.AppendString(fmt.Sprintf("%-*s", CallerPaddingWidth, caller.TrimmedPath())) }
Line range hint
1-298
: Add test coverage for new configuration options.Please add test cases to verify:
- Local time encoding functionality
- Verify correct time format
- Verify timezone handling
- Caller information display
- Verify caller info format in console output
- Verify caller info format in file output
- Verify padding behavior with different path lengths
Would you like me to help generate the test cases?
🧰 Tools
🪛 golangci-lint
[warning] 111-111: var-declaration: should drop = nil from declaration of var encodeCaller; it is the zero value
(revive)
103-104
: Consider security implications of exposing caller information.The
ShowCaller
option could potentially expose sensitive path information in production environments. Consider:
- Adding a warning in the documentation about security implications
- Implementing path sanitization for production mode
- Adding configuration option to control the depth of path information
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
config.go
(8 hunks)
🧰 Additional context used
🪛 golangci-lint
config.go
[warning] 111-111: var-declaration: should drop = nil from declaration of var encodeCaller; it is the zero value
(revive)
🔇 Additional comments (1)
config.go (1)
Line range hint 234-259
: LGTM! Clean implementation of file logger caller support.
The implementation correctly:
- Uses a non-colored encoder for file output
- Conditionally adds caller support
- Maintains separation between console and file output configurations
encoder.go
Outdated
if len(s) < 12 { | ||
s += strings.Repeat(" ", 12-len(s)) | ||
if len(s) < 20 { | ||
s += strings.Repeat(" ", 20-len(s)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the service plugin displays the plugin name with the specific service name, the name is too long, and 12 characters are obviously not enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enough for what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such as :
service
sync-data:
command: xxx
the length of 'service.sync-data' is 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't understand what that code is supposed to do. If the len is, lets say: 17. In that case, the full service name would be written without omitting any character.
But, if the service name is lets say: 5, 12-5=7 extra spaces would be added to have some extra padding for the small service names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thus, I'm not really understand your motivation to increase that limit from 12 to 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry. I didn't look closely enough. I made a mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
encoder.go (1)
28-29
: Consider making the padding width configurable.Since the padding width is now larger to accommodate the new caller information, consider making it configurable to allow users to adjust it based on their needs.
Here's a suggested implementation:
+// DefaultNamePadding defines the default padding width for service names +const DefaultNamePadding = 20 + func ColoredNameEncoder(s string, enc zapcore.PrimitiveArrayEncoder) { - if len(s) < 20 { - s += strings.Repeat(" ", 20-len(s)) + if len(s) < DefaultNamePadding { + s += strings.Repeat(" ", DefaultNamePadding-len(s)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
encoder.go
(1 hunks)
🔇 Additional comments (1)
encoder.go (1)
28-29
: Verify the impact of increased padding on log formatting.
The padding increase from 12 to 20 characters could affect log readability and alignment, especially in environments with narrow terminal widths.
Let's check for any existing log format assumptions in the codebase:
✅ Verification successful
Padding increase to 20 characters is safe and aligned with the codebase design
The increased padding is consistent with the logging system's design:
- The logger supports both development and production modes with different formatting
- The padding is only applied in development mode with console encoding
- Tests verify log output without asserting specific width constraints
- Other components using caller information already use 40-character padding (see
ShortCallerEncoderWithPadding
) - The change accommodates longer service names while maintaining alignment with other formatted elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code that might depend on specific log formatting or column widths
# Test 1: Look for log parsing or formatting related code
rg -g '!vendor/*' -g '*.go' 'log.*format|column.*width|padding'
# Test 2: Look for tests that might assert specific log formats
rg -g '!vendor/*' -g '*_test.go' 'assert.*log|expect.*log'
Length of output: 126
Script:
#!/bin/bash
# Let's try a broader search to understand the logging context and usage
# Search for any log formatting or encoder related code
rg -g '!vendor/*' 'ColoredNameEncoder|NameEncoder'
# Look for any test files that might verify log output
fd -e go -e test.go --exec grep -l "log" {}
# Check for any configuration or constants related to log formatting
rg -g '!vendor/*' -g '*.go' 'const.*log|var.*log|type.*log'
Length of output: 537
Script:
#!/bin/bash
# Let's examine the test files and logger implementation for any format dependencies
# Check logger test content
cat tests/logger_test.go
# Look for any specific width or format assertions in tests
rg -g '!vendor/*' -A 5 'ColoredNameEncoder'
# Check the config implementation to understand how the encoder is used
cat config.go
Length of output: 17287
@shellphy Do you have plans to continue working on this PR? |
What should I do to make this feature merge? |
Remove |
Reason for This PR
When developing custom plugins, it was difficult to find the corresponding location in the log, so caller was added
In the production environment, the log time format is not readable enough, so the local time display function is added
Description of Changes
add configures:
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]
git commit -s
).CHANGELOG.md
.Summary by CodeRabbit
New Features
Bug Fixes
Documentation