Skip to content
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

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

shellphy
Copy link

@shellphy shellphy commented Nov 9, 2024

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:

logs:
  use_local_time_encoder: true
  show_caller: true

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]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • New Features

    • Enhanced logging capabilities with options to use local time and show caller information.
    • Updated logging configuration to support new contextual logging options.
  • Bug Fixes

    • Improved test cases for logger functionality to ensure correct log formatting and content.
  • Documentation

    • Updated configuration files to reflect new logging options and their default settings.

Copy link

coderabbitai bot commented Nov 9, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes involve significant updates to the logging configuration and functionality within the config.go file, enhancing the Config structure with new fields for local time usage and caller information. Corresponding modifications are made to the logging configuration file and test cases to validate these new features. The updates ensure that log entries can now include timestamps in local time and caller details, improving the contextual information available in logs.

Changes

File Change Summary
config.go Added fields: UseLocalTime bool, ShowCaller bool. Updated BuildLogger method and added localTimeEncoder. Modified time encoding logic.
tests/configs/.rr-file-logger.yaml Added options: use_local_time: true, show_caller: true in the logs section.
tests/logger_test.go Enhanced tests with assertions for timestamp formatting and caller information in log entries.
encoder.go Updated ColoredNameEncoder to change padding length from 12 to 20 characters for service names.

Poem

In the log's bright glow,
Time dances, high and low,
With callers now in sight,
Our logs shine ever bright!
Local time, a gentle chime,
Enhancing every rhyme! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 mode

Also applies to: 155-155, 162-162, 181-184, 192-192, 199-199

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5fbb485 and ecfe574.

📒 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
Comment on lines 277 to 279
func localTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) {
enc.AppendString(t.Local().Format("2006-01-02 15:04:05.000"))
}
Copy link

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.

Suggested change
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"))
}

@rustatian rustatian self-requested a review November 9, 2024 17:14
@rustatian rustatian added the enhancement New feature or request label Nov 9, 2024
UseLocalTimeEncoder bool `mapstructure:"use_local_time_encoder"`

// ShowCaller is used to set the encoder to show the caller.
ShowCaller bool `mapstructure:"show_caller"`
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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
Comment on lines 117 to 120
encodeTime := utcEpochTimeEncoder
if cfg.UseLocalTimeEncoder {
encodeTime = localTimeEncoder
}
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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"`
Copy link
Member

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.

@rustatian rustatian changed the title add use_local_time_encoder and show_caller config feature: add use_local_time_encoder and show_caller config Nov 9, 2024
@rustatian
Copy link
Member

Hey @shellphy 👋
Thank you for the PR 👍
A few parts are missing:

  1. Tests
  2. Docs PR to the roadrunner-server/docs repository.

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.57%. Comparing base (f11fc67) to head (571f8b6).
Report is 138 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

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"))
Copy link
Member

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.

Copy link

@coderabbitai coderabbitai bot left a 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 the UseLocalTime field in the Config struct. For consistency, consider using either use_local_time_encoder or useLocalTimeEncoder 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:

  1. 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}`
  1. Make the caller info regex more specific:
-`\w+\.go:\d+`
+`[a-zA-Z0-9_]+\.go:\d+`
  1. 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

📥 Commits

Reviewing files that changed from the base of the PR and between ecfe574 and bb8c385.

⛔ 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:

  1. Add tests for the new configuration options
  2. Submit documentation updates to the roadrunner-server/docs repository

Would you like help with:

  1. Generating test cases for the new configuration options?
  2. Creating documentation for the new features?
✅ Verification successful

Tests and documentation are already in place.

The review comment's concerns have been addressed:

  1. Tests for the new configuration options (use_local_time and show_caller) already exist in tests/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+
  2. 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

Comment on lines +24 to +25
use_local_time: true
show_caller: true
Copy link

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

Copy link

@coderabbitai coderabbitai bot left a 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 for encodeCaller 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:

  1. Local time encoding functionality
    • Verify correct time format
    • Verify timezone handling
  2. 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:

  1. Adding a warning in the documentation about security implications
  2. Implementing path sanitization for production mode
  3. Adding configuration option to control the depth of path information
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bb8c385 and 0e06e7a.

📒 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

config.go Show resolved Hide resolved
encoder.go Outdated
if len(s) < 12 {
s += strings.Repeat(" ", 12-len(s))
if len(s) < 20 {
s += strings.Repeat(" ", 20-len(s))
}
Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not enough for what?

Copy link
Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0e06e7a and c1430a7.

📒 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

@rustatian rustatian marked this pull request as draft November 10, 2024 23:22
@rustatian
Copy link
Member

@shellphy Do you have plans to continue working on this PR?

@shellphy
Copy link
Author

What should I do to make this feature merge?

@rustatian
Copy link
Member

Remove show_caller, resolve all the comments, ping me after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants