🔥 Feature: Add AllLogger to Config #3153
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe pull request introduces enhancements to the Fiber framework's logging capabilities. It adds a new function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
haochunchang
force-pushed
the
feature/logger-interface
branch
2 times, most recently
from
803191e to
b5aa388
Compare
haochunchang
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3153 +/- ##
==========================================
+ Coverage 82.73% 82.82% +0.09%
==========================================
Files 115 115
Lines 11258 11287 +29
==========================================
+ Hits 9314 9349 +35
+ Misses 1543 1539 -4
+ Partials 401 399 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
log/log.go (2)
59-59: Alignment with PR objectivesWhile the addition of
SetFlags(flags int)to theControlLoggerinterface enhances logger configurability, it doesn't fully address the PR objectives of allowing users to integrate a custom logger adhering to the Fiber logger interface.To better align with the PR objectives, consider:
- Implementing a way for users to provide their custom logger implementation.
- Ensuring that the custom logger can be used in place of the default logger throughout the Fiber framework.
- Providing clear documentation on how users can implement and integrate their custom loggers.
Would you like assistance in drafting a more comprehensive change that fully addresses the PR objectives?
Line range hint
1-114: Overall file review and next stepsThe change to add
SetFlags(flags int)to theControlLoggerinterface is a step in the right direction for enhancing logger configurability. However, this file alone doesn't fully address the PR objectives of allowing users to integrate custom loggers adhering to the Fiber logger interface.Suggestions for next steps:
- Review other files in the PR to ensure the full implementation of custom logger integration.
- If not already done, implement a mechanism for users to provide and use their custom logger implementations.
- Update or create documentation explaining how users can implement and integrate custom loggers.
- Consider adding examples demonstrating the usage of custom loggers, as mentioned in the PR objectives.
If you'd like assistance in implementing any of these suggestions or reviewing other parts of the PR, please let me know.
middleware/logger/config.go (3)
47-48: Approve new Logger field, but suggest adding a comment.The new
Loggerfield of typefiberlog.AllLoggeris a good addition that aligns with the PR objectives. It allows users to provide custom loggers that implement theAllLoggerinterface.However, to improve clarity for users, consider adding a comment explaining the usage of this field, similar to other fields in the struct. For example:
// Logger allows the use of a custom logger that implements the AllLogger interface. // If not set, the default Fiber logger will be used. // // Optional. Default: nil Logger fiberlog.AllLoggerThis comment would provide clear guidance on the field's purpose and default behavior.
Line range hint
111-153: Update configDefault function to handle the new Logger field.The
configDefaultfunction should be updated to handle the newLoggerfield. This ensures consistent initialization of all configuration fields. Consider adding the following logic:if cfg.Logger == nil { cfg.Logger = ConfigDefault.Logger }This addition will maintain consistency with how other fields are handled in the function. If
nilis the intended default, you might want to add a comment explaining this behavior.Additionally, consider if any logic is needed to ensure compatibility between
cfg.Loggerandcfg.LoggerFuncif both are provided.
Inconsistency between Config struct and ConfigDefault initialization
The
Configstruct inmiddleware/logger/config.godoes not include aLoggerfield, yetConfigDefaultis initializing fields likeLoggerFuncandBeforeHandlerFunc. This discrepancy can lead to runtime errors or unexpected behavior. Please ensure that theConfigstruct is updated to include theLoggerfield or adjustConfigDefaultto align with the existing struct definition.🔗 Analysis chain
Line range hint
97-108: Consider updating ConfigDefault for the new Logger field.The
ConfigDefaultvariable has not been updated to include a default value for the newLoggerfield. While this might be intentional, it's worth considering either:
- Setting a default value (e.g.,
nilor a default logger instance) inConfigDefault, or- Adding a comment explaining why the
Loggerfield is intentionally left unset in the default configuration.This will help maintain consistency and clarity in the configuration setup. Additionally, ensure that the rest of the codebase handles a
nillogger gracefully if that's the intended default behavior.To verify the current usage and handling of the logger, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the Logger field and default logger handling rg --type go -e 'cfg\.Logger' -e 'defaultLogger' middleware/Length of output: 3205
Script:
#!/bin/bash # Retrieve the definition of the Config struct in middleware/logger/config.go cat middleware/logger/config.go | grep -A 10 'type Config struct' # Search for any instances where ConfigDefault is initialized or modified rg --type go 'ConfigDefault' middleware/Length of output: 13958
log/default.go (1)
213-215: LGTM! Consider adding documentation and tests.The implementation of
SetFlagslooks good and aligns with the PR objectives by enhancing the logger's capabilities. However, there are a couple of improvements we should make:
- As this is an exported method (part of the public API), please add a comment to document its purpose and usage.
- The static analysis indicates that this new method is not covered by tests. Let's add unit tests to ensure its functionality.
Would you like assistance in generating the documentation comment or unit tests for this new method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 213-214: log/default.go#L213-L214
Added lines #L213 - L214 were not covered by testsdocs/middleware/logger.md (1)
111-111: Approve addition ofLoggerFuncwith minor suggestion.The new
LoggerFuncproperty is a great addition that aligns with the PR objectives, allowing users to integrate custom logging functions. This enhances the flexibility of the logger middleware as requested in issue #2737.Consider this minor grammatical improvement:
- If you don't define anything for this field, it'll use default logger of Fiber. + If you don't define anything for this field, it'll use the default logger of Fiber.🧰 Tools
🪛 LanguageTool
[uncategorized] ~111-~111: Possible missing article found.
Context: ...fine anything for this field, it'll use default logger of Fiber. | `see default_logger...(AI_HYDRA_LEO_MISSING_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- docs/middleware/logger.md (1 hunks)
- log/default.go (1 hunks)
- log/log.go (1 hunks)
- middleware/logger/config.go (2 hunks)
- middleware/logger/logger_test.go (3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/middleware/logger.md
[uncategorized] ~111-~111: Possible missing article found.
Context: ...fine anything for this field, it'll use default logger of Fiber. | `see default_logger...(AI_HYDRA_LEO_MISSING_THE)
🪛 GitHub Check: codecov/patch
log/default.go
[warning] 213-214: log/default.go#L213-L214
Added lines #L213 - L214 were not covered by tests
🪛 GitHub Check: lint
middleware/logger/logger_test.go
[failure] 193-193:
unused-parameter: parameter 'c' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 913-913:
type assertion must be checked (forcetypeassert)
🔇 Additional comments (5)
middleware/logger/config.go (2)
9-9: LGTM: New import for fiberlog.The new import for
fiberlogis correctly added and aliased. This import is necessary for the newLoggerfield in theConfigstruct.
Line range hint
1-153: Summary: Good progress on logger enhancement, consider broader implementation.The changes in this file are a good start towards enhancing the logger middleware as per the PR objectives. The addition of the
Loggerfield in theConfigstruct allows for custom logger integration.To ensure a complete implementation:
- Verify that other files in the middleware package use this new
Loggerfield appropriately.- Update any documentation or examples to reflect this new capability.
- Consider adding unit tests to verify the behavior with both default and custom loggers.
To check for potential usage and necessary updates in other files:
#!/bin/bash # Search for files that might need updates due to the new Logger field rg --type go -e 'type Config struct' -e 'func New\(' middleware/This will help identify other locations where the
Configstruct is used or where the middleware is initialized, which might need updates to accommodate the newLoggerfield.log/default.go (1)
213-215: Verify full implementation of PR objectivesThe addition of the
SetFlagsmethod is a good enhancement to the logger's capabilities. However, the PR objectives mention using the Logger interface instead offmt.Sprintf(). Could you clarify how this change relates to that objective? Are there additional changes in other files that implement this shift to using the Logger interface?To help verify the full implementation, you can run the following script:
This will help us understand if the Logger interface is being used as intended and if there are any remaining instances of
fmt.Sprintf()in logging contexts.✅ Verification successful
PR objectives verified: Logger interface correctly replaces fmt.Sprintf in logging contexts
All instances of
fmt.Sprintfwithin logging functions have been successfully replaced with the Logger interface. No remainingfmt.Sprintfusages were found in logging contexts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Logger interface usage and fmt.Sprintf removal # Search for Logger interface usage echo "Searching for Logger interface usage:" rg --type go "Logger\s+interface" # Search for remaining fmt.Sprintf usage in logging context echo "Searching for remaining fmt.Sprintf usage in logging context:" rg --type go "fmt\.Sprintf.*log"Length of output: 524
Script:
#!/bin/bash # Description: Comprehensive search for fmt.Sprintf usage in the codebase # Search for all fmt.Sprintf usages echo "Searching for all fmt.Sprintf usages:" rg --type go "fmt\.Sprintf" # Additionally, search for fmt.Sprintf within common logging function calls echo "Searching for fmt.Sprintf within logging functions:" rg --type go "fmt\.Sprintf\s*\(.*\)\s*log\."Length of output: 7200
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 213-214: log/default.go#L213-L214
Added lines #L213 - L214 were not covered by testsmiddleware/logger/logger_test.go (2)
18-18: Import statement for 'fiberlog' package is correctly addedThe import of
fiberlogpackage is necessary for integrating the Fiber logger. The import statement is properly added.
907-931: Benchmark 'DefaultFormatWithFiberLog' is correctly implementedThe new benchmark case
DefaultFormatWithFiberLogis properly added toBenchmark_Logger_Parallel. It effectively benchmarks the logger middleware using the Fiber logger in a parallel context, which is consistent with the existing benchmarking structure.🧰 Tools
🪛 GitHub Check: lint
[failure] 913-913:
type assertion must be checked (forcetypeassert)
log/log.go
Outdated
| @@ -56,6 +56,7 @@ type CommonLogger interface { | |||
| type ControlLogger interface { | |||
| SetLevel(level Level) | |||
| SetOutput(w io.Writer) | |||
| SetFlags(flags int) | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation Missing for SetFlags Method
The SetFlags(flags int) method in the ControlLogger interface lacks documentation. Providing comments or godoc annotations for this method is essential to clarify its purpose and usage, ensuring that other developers can effectively utilize and maintain this functionality.
- Add documentation comments for the
SetFlagsmethod inlog/log.go.
🔗 Analysis chain
New method added to ControlLogger interface
The addition of SetFlags(flags int) to the ControlLogger interface enhances the configurability of loggers implementing this interface. This change allows for more fine-grained control over the logging output format.
However, there are a few points to consider:
- The PR objectives mention introducing a new field for the middleware/logger, but this change only adds a method to an existing interface. It's not clear if this fully addresses the stated objectives.
- The
flagsparameter type (int) suggests this might be using predefined constants for different flag options. It would be helpful to have documentation or constants defined for the possible flag values.
To ensure this change is consistent with the rest of the codebase and meets the PR objectives, let's run the following checks:
Further Verification Needed for SetFlags Documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of SetFlags method
# Check if there are any existing usages of SetFlags in the codebase
echo "Checking for existing SetFlags usages:"
rg --type go "SetFlags\(" -C 3
# Check if there are any flag constants defined in the codebase
echo "Checking for flag constants:"
rg --type go "const.*Flag" -C 3
# Check if there's any documentation for the SetFlags method
echo "Checking for SetFlags documentation:"
rg --type go "SetFlags.*godoc" -C 3
Length of output: 1844
Script:
#!/bin/bash
# Description: Check for comments above the SetFlags method in log/log.go
# Extract lines around the SetFlags method to find any associated comments
rg --type go -A 5 -B 2 "SetFlags\(flags int\)" log/log.go
Length of output: 294
There was a problem hiding this comment.
Can you update log docs @haochunchang for SetFlags
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai pls prepare all the changes from the hints
There was a problem hiding this comment.
@ReneWerner87, I've prepared the changes from the hints. Here's the updated code with documentation for the SetFlags method in log/log.go:
// ControlLogger provides methods to configure a logger.
type ControlLogger interface {
SetLevel(level Level)
SetOutput(w io.Writer)
// SetFlags sets the output flags for the logger.
// The flags parameter controls the logging properties, such as datetime format, file name, and line number.
SetFlags(flags int)
}Let me know if there's anything else I can assist with.
|
@haochunchang thx for the PR, we will have a look can you check the pre hints from our review ai |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/middleware/logger.md (2)
112-112: Enhance documentation for theLoggerpropertyWhile the addition of the
Loggerproperty is a valuable enhancement, the documentation could be improved to provide more clarity and guidance for users. Consider the following suggestions:
- Provide a brief explanation or a link to the documentation for the
AllLoggerinterface.- Clarify the interaction between
LoggerandLoggerFuncwhen both are defined. Which one takes precedence?- Add an example in the "Examples" section demonstrating how to use the
Loggerproperty with a custom logger implementing theAllLoggerinterface.These improvements will help users better understand and utilize the new logging capabilities.
Line range hint
1-112: Add examples for new logging featuresThe documentation has been updated to include the new
LoggerFuncandLoggerproperties, which is great. However, to fully align with the PR objectives and provide maximum value to users, consider adding specific examples that demonstrate:
- How to use the
LoggerFuncproperty with a custom logging function.- How to implement and use a custom logger with the
Loggerproperty.These examples would help users understand how to leverage the new logging capabilities in their Fiber applications.
🧰 Tools
🪛 LanguageTool
[style] ~108-~108: In American English, abbreviations like “etc.” require a period.
Context: ...America/New_York" and "Asia/Chongqing", etc ...(ETC_PERIOD)
middleware/logger/logger_test.go (3)
185-214: LGTM! Consider adding more test cases.The new test function
Test_Logger_Fiber_Loggereffectively verifies the integration of Fiber's default logger with the middleware. It correctly sets up a custom logger function and checks if error messages are captured and formatted as expected.To enhance test coverage, consider adding more test cases that verify different logging scenarios, such as successful requests or requests with different status codes.
764-784: LGTM! Consider parameterizing the benchmark.The new benchmark case "DefaultFormatWithFiberLog" effectively measures the performance of the logger middleware when using Fiber's default logger. It's well-integrated with the existing benchmark structure.
To make the benchmark more flexible and reusable, consider parameterizing the log format string. This would allow testing different log formats without duplicating the benchmark code.
929-949: LGTM! Consider adding a comparison comment.The new parallel benchmark case "DefaultFormatWithFiberLog" effectively measures the concurrent performance of the logger middleware when using Fiber's default logger. It's well-integrated with the existing parallel benchmark structure.
Consider adding a comment comparing this benchmark with its non-parallel counterpart. This would help developers understand the performance implications of using the Fiber logger in concurrent scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- docs/middleware/logger.md (1 hunks)
- middleware/logger/config.go (2 hunks)
- middleware/logger/logger_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/logger/config.go
🔇 Additional comments (2)
docs/middleware/logger.md (1)
111-111: LGTM: NewLoggerFuncproperty well-documentedThe addition of the
LoggerFuncproperty to the Config table is well-documented and aligns with the PR objectives. It provides users with the flexibility to integrate custom logging functions, which is a valuable enhancement to the logger middleware.middleware/logger/logger_test.go (1)
Line range hint
1-1090: Great addition of Fiber logger integration tests and benchmarks!The new test function and benchmark cases effectively cover the integration of Fiber's default logger with the middleware. They are well-structured, consistent with the existing code, and address the PR objective of utilizing the Logger interface.
These additions will help ensure the reliability and performance of the new logging functionality. Great job on maintaining code quality and test coverage!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/middleware/logger.md (3)
91-103: Great addition of custom logger example!The new example effectively demonstrates how to use a custom logger with the Fiber Logger Interface, which aligns well with the PR objectives. It clearly shows the usage of both
LoggerFuncandLoggerproperties in the config.To further improve clarity, consider adding a brief comment explaining the purpose of
fiberlog.DefaultLogger()in line 101.
124-124: Excellent addition ofLoggerFuncproperty!The description of the
LoggerFuncproperty is comprehensive and clearly explains its purpose and compatibility with various logging libraries. This addition aligns well with the PR objectives of enhancing logging capabilities.To improve clarity, consider explicitly stating the default value instead of referencing
defaultLoggerInstance. For example:
"Default: SeedefaultLoggerInstancein default_logger.go"
125-125: Good addition ofLoggerproperty, addressing previous feedback!The description of the
Loggerproperty effectively explains its purpose and relationship with theLoggerFunc. This addition aligns with the PR objectives and addresses the previous review comment requesting a description for this property.To further improve clarity:
- Consider adding a brief explanation of what the
AllLoggerinterface entails.- Provide a short example or link to documentation about implementing the
AllLoggerinterface.- Clarify the default value. Instead of "nil", consider stating "Default: None (nil)".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docs/middleware/logger.md (2 hunks)
🔇 Additional comments (1)
docs/middleware/logger.md (1)
Line range hint
91-125: Overall excellent documentation updates!The changes to the Logger middleware documentation effectively communicate the new logging capabilities introduced in this PR. The additions align well with the objectives of enhancing logging flexibility and supporting custom loggers.
The new example and property descriptions provide clear guidance for users on how to leverage these new features. The minor suggestions provided in the individual comments will further improve clarity and completeness.
Great job on these documentation updates!
|
Hi, I have addressed a few comments from the coderabbitai. |
|
Thx will do it tomorrow |
There was a problem hiding this comment.
@haochunchang
thank you for the effort, have not made any comments
|
I think we can add a wrapper instead of messing with new config field and a function. Hereby, fiber logger interface can be used with the middleware by adjusting type dumbLogger struct {
logger log.AllLogger
level log.Level
}
func (l *dumbLogger) Write(p []byte) (n int, err error) {
switch l.level {
case log.LevelDebug:
l.logger.Debug(string(p))
case log.LevelError:
l.logger.Error(string(p))
case log.LevelFatal:
l.logger.Fatal(string(p))
case log.LevelInfo:
l.logger.Info(string(p))
case log.LevelTrace:
l.logger.Trace(string(p))
case log.LevelWarn:
l.logger.Warn(string(p))
}
return len(p), nil
}
func LoggerToWriter(logger log.AllLogger, level log.Level) io.Writer {
return &dumbLogger{
logger: logger,
level: level,
}
} |
|
Cool. Then we can use this wrapper like the following: cfg.Output = LoggerToWriter(logger, level)Is that what you mean? |
Yes it is |
haochunchang
force-pushed
the
feature/logger-interface
branch
from
598b9d8 to
3dd20b4
Compare
|
I think using the wrapper method @efectn suggested can prevent disrupting the original Config and default logger 👍 If using the wrapper method, then we only need to add an example into the documentation to provide users an example of using fiberlog interface. I also added a unit test and benchmark tests for using the wrapper method. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
log/default.go (1)
213-215: LGTM! Consider adding a comment for clarity.The
SetFlagsmethod is a good addition, providing more control over the logging output. It's consistent with other setter methods in the struct and aligns well with the PR objectives.Consider adding a brief comment to explain the purpose of this method and possibly link to the standard log package documentation for flag values. For example:
// SetFlags sets the output flags for the logger. // Flag values are defined in the standard log package. func (l *defaultLogger) SetFlags(flag int) { l.stdlog.SetFlags(flag) }docs/middleware/logger.md (2)
92-117: LGTM! Consider adding a brief explanation.The new example for using a custom logger with the Fiber Logger interface is well-implemented and aligns with the PR objectives. It provides a clear demonstration of how to integrate custom logging solutions.
Consider adding a brief explanation before the code block to introduce the concept of using a custom logger with the Fiber Logger interface. This will help users understand the purpose and context of the example more quickly.
🧰 Tools
🪛 Markdownlint
94-94: Column: 1
Hard tabs(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs(MD010, no-hard-tabs)
100-100: Column: 1
Hard tabs(MD010, no-hard-tabs)
101-101: Column: 1
Hard tabs(MD010, no-hard-tabs)
102-102: Column: 1
Hard tabs(MD010, no-hard-tabs)
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
109-109: Column: 1
Hard tabs(MD010, no-hard-tabs)
Line range hint
1-210: Suggestions for improved organization and readabilityThe updated documentation provides valuable information and examples for users. To further enhance its readability and organization, consider the following suggestions:
- Move the new custom logger example (lines 92-117) to the "Examples" section, grouping it with other usage examples.
- Add a subsection header for "Custom Logger Integration" before the new example to clearly separate it from other examples.
- Update the table of contents (if present) to reflect the new additions.
- Consider adding a brief introduction to the concept of custom loggers and their benefits at the beginning of the document.
These changes will help users navigate the documentation more easily and understand the new features in the context of the existing functionality.
🧰 Tools
🪛 Markdownlint
94-94: Column: 1
Hard tabs(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs(MD010, no-hard-tabs)
100-100: Column: 1
Hard tabs(MD010, no-hard-tabs)
101-101: Column: 1
Hard tabs(MD010, no-hard-tabs)
102-102: Column: 1
Hard tabs(MD010, no-hard-tabs)
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
109-109: Column: 1
Hard tabs(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- docs/middleware/logger.md (2 hunks)
- log/default.go (1 hunks)
- log/log.go (1 hunks)
- middleware/logger/logger_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- log/log.go
🧰 Additional context used
🪛 Markdownlint
docs/middleware/logger.md
94-94: Column: 1
Hard tabs(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs(MD010, no-hard-tabs)
100-100: Column: 1
Hard tabs(MD010, no-hard-tabs)
101-101: Column: 1
Hard tabs(MD010, no-hard-tabs)
102-102: Column: 1
Hard tabs(MD010, no-hard-tabs)
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
109-109: Column: 1
Hard tabs(MD010, no-hard-tabs)
138-138: Expected: 4; Actual: 5; Too many cells, extra data will be missing
Table column count(MD056, table-column-count)
🔇 Additional comments (3)
log/default.go (1)
213-215: Summary: Good addition with minor suggestions.The addition of the
SetFlagsmethod enhances the flexibility of the logger, aligning well with the PR objectives. The implementation is clean and consistent with existing code. Consider adding a brief comment for clarity and ensure consistency with theAllLoggerinterface across the package.docs/middleware/logger.md (1)
138-138: Address previous review commentsTwo previous review comments need attention:
The suggestion to add a description for the
Loggerproperty. It seems this property might have been renamed toLoggerFuncor removed. Please clarify this change and update the documentation accordingly.The request to add the default value of the new property to the default config section. While the default value is mentioned in the table, it should also be added to the "Default Config" section for consistency.
Could you please clarify the status of the
Loggerproperty and update the "Default Config" section to include the newLoggerFuncproperty?🧰 Tools
🪛 Markdownlint
138-138: Expected: 4; Actual: 5; Too many cells, extra data will be missing
Table column count(MD056, table-column-count)
middleware/logger/logger_test.go (1)
199-223: Test 'Test_Logger_Fiber_Logger' successfully validates custom logger integrationThe new test function
Test_Logger_Fiber_Loggercorrectly injects a custom Fiber logger and verifies that it receives the log output as expected. This addresses the previous request to demonstrate that the custom logger receives the data.
log/default.go
Outdated
| func (l *defaultLogger) SetFlags(flag int) { | ||
| l.stdlog.SetFlags(flag) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add SetFlags to the AllLogger interface for consistency.
The SetFlags method in defaultLogger is not part of the AllLogger interface. To maintain interface consistency across the package, please add the SetFlags method to the AllLogger interface.
🔗 Analysis chain
Verify interface consistency across the package.
The SetFlags method is a valuable addition to the defaultLogger. However, it's important to ensure consistency across the package.
Please run the following script to check if the SetFlags method is added to the AllLogger interface:
If the SetFlags method is not part of the AllLogger interface, consider adding it to maintain consistency and allow users to set flags on any logger implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SetFlags method is added to the AllLogger interface
# Test: Search for the AllLogger interface definition
echo "Searching for AllLogger interface definition:"
rg --type go -A 10 "type AllLogger interface" ./log
# Test: Check if SetFlags is part of the AllLogger interface
echo "Checking if SetFlags is part of the AllLogger interface:"
rg --type go "SetFlags\(.*\)" ./log
Length of output: 905
|
Wrapper method sounds good |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
log/log.go (3)
65-66: Consider improving type safety of GetLoggerInstanceThe
GetLoggerInstance() anymethod uses a genericanyreturn type, which loses type safety and requires type assertions at the call site. Consider:
- Using a more specific interface type that defines common logger operations
- Adding examples in the documentation showing type assertion usage
Example documentation improvement:
-// GetLoggerInstance returns the logger instance. It can be used to adjust the logger configurations in case of need. +// GetLoggerInstance returns the underlying logger instance, which can be used to adjust logger-specific configurations. +// Example usage: +// if stdLogger, ok := logger.GetLoggerInstance().(*log.Logger); ok { +// stdLogger.SetFlags(log.Ldate | log.Ltime) +// } GetLoggerInstance() any
57-61: Add parameter validation for SetLevelThe
SetLevelmethod should validate that the input level is within the defined constants (LevelTrace to LevelPanic).Example implementation:
// SetLevel sets logging level. // // Available levels: Trace, Debug, Info, Warn, Error, Fatal, Panic. // Returns error if level is out of range. SetLevel(level Level) error
75-75: Enhance WithContext documentation with usage examplesThe
WithContextmethod would benefit from documentation explaining common use cases and context propagation.Example documentation improvement:
-// WithContext returns a new logger with the given context. +// WithContext returns a new logger with the given context. This allows for context-aware logging +// where log entries can include context-specific fields (e.g., request ID, trace ID). +// +// Example usage: +// ctx := context.WithValue(context.Background(), "requestID", "123") +// ctxLogger := logger.WithContext(ctx) +// ctxLogger.Info("Processing request") // Log will include requestID WithContext(ctx context.Context) CommonLoggermiddleware/logger/logger_test.go (1)
187-227: Add test cases for colorized output and concurrent logging.The test function should be expanded to include:
- Verification of colorized log output as requested in past review comments
- Validation of thread-safety when using the Fiber logger in concurrent scenarios
Add these test cases:
func Test_Logger_Fiber_Logger(t *testing.T) { t.Parallel() + t.Run("colorized output", func(t *testing.T) { + app := fiber.New() + buf := bytebufferpool.Get() + defer bytebufferpool.Put(buf) + + logger := fiberlog.DefaultLogger() + logger.SetOutput(buf) + + app.Use(New(Config{ + Format: "${error}", + Output: LoggerToWriter(logger, fiberlog.LevelError), + EnableColors: true, + })) + + app.Get("/", func(_ fiber.Ctx) error { + return errors.New("colorized error") + }) + + resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) + require.NoError(t, err) + require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode) + require.Contains(t, buf.String(), "\x1b[31m") // ANSI red color code + }) + + t.Run("concurrent logging", func(t *testing.T) { + app := fiber.New() + buf := &syncBuffer{buffer: bytebufferpool.Get()} + defer bytebufferpool.Put(buf.buffer) + + logger := fiberlog.DefaultLogger() + logger.SetOutput(buf) + + app.Use(New(Config{ + Format: "${error}", + Output: LoggerToWriter(logger, fiberlog.LevelError), + })) + + app.Get("/", func(_ fiber.Ctx) error { + return errors.New("concurrent error") + }) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + resp, _ := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) + require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode) + }() + } + wg.Wait() + + require.Equal(t, 10, strings.Count(buf.buffer.String(), "concurrent error")) + }) } +// syncBuffer is a thread-safe buffer for testing concurrent writes +type syncBuffer struct { + buffer *bytebufferpool.ByteBuffer + mu sync.Mutex +} + +func (b *syncBuffer) Write(p []byte) (n int, err error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.Write(p) +}docs/whats_new.md (2)
634-639: Enhance documentation for GetLoggerInstance method.Consider adding a code example demonstrating how to use the GetLoggerInstance method and explaining common use cases. This would help users better understand when and how to use this feature.
Add a code example like this:
## 📃 Log `fiber.AllLogger` interface now has a new method called `GetLoggerInstance`. This method can be used to get the underlying logger instance from the Fiber logger middleware. This is useful when you want to configure the logger middleware with a custom logger and still want to access the underlying logger instance. You can find more details about this feature in [/docs/api/log.md](./api/log.md#getloggerinstance). + +### Example + +```go +app := fiber.New() + +// Create a custom logger +customLogger := mylogger.New() + +// Configure logger middleware with custom logger +app.Use(logger.New(logger.Config{ + Output: customLogger, +})) + +// Get the underlying logger instance +if logger, ok := customLogger.(fiber.AllLogger); ok { + instance := logger.GetLoggerInstance() + // Use the instance for additional configuration +} +```🧰 Tools
🪛 Markdownlint (0.35.0)
636-636: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
715-753: Improve Logger middleware documentation and formatting.
- Replace hard tabs with spaces in the code example for consistency.
- Add more context about the LoggerToWriter function's purpose and benefits.
Apply these changes:
### Logger -New helper function called `LoggerToWriter` has been added to the logger middleware. +The new `LoggerToWriter` helper function simplifies integration with third-party loggers by converting them into a format compatible with Fiber's logger middleware. This eliminates the need for custom adapters or wrappers. This function allows you to use 3rd party loggers such as `logrus` or `zap` with the Fiber logger middleware without any extra afford. For example, you can use `zap` with Fiber logger middleware like this: ```go package main import ( "github.com/gofiber/contrib/fiberzap/v2" "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/log" "github.com/gofiber/fiber/v3/middleware/logger" ) func main() { // Create a new Fiber instance app := fiber.New() // Create a new zap logger which is compatible with Fiber AllLogger interface zap := fiberzap.NewLogger(fiberzap.LoggerConfig{ ExtraKeys: []string{"request_id"}, }) // Use the logger middleware with zerolog logger app.Use(logger.New(logger.Config{ Output: logger.LoggerToWriter(zap, log.LevelDebug), })) // Define a route app.Get("/", func(c fiber.Ctx) error { return c.SendString("Hello, World!") }) // Start server on http://localhost:3000 app.Listen(":3000") }
+### Additional Examples
+
+TheLoggerToWriterfunction also works with other popular loggers:
+
+```go
+// Using logrus
+logrusLogger := logrus.New()
+app.Use(logger.New(logger.Config{
- Output: logger.LoggerToWriter(logrusLogger, log.LevelDebug),
+}))+// Using zerolog
+zerologLogger := zerolog.New(os.Stdout)
+app.Use(logger.New(logger.Config{
- Output: logger.LoggerToWriter(zerologLogger, log.LevelDebug),
+}))
+```<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 717-717: Expected: 0 or 2; Actual: 1 Trailing spaces (MD009, no-trailing-spaces) --- 724-724: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 725-725: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 726-726: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 727-727: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 731-731: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 732-732: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 735-735: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 736-736: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 737-737: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 739-739: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 740-740: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 741-741: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 742-742: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 744-744: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 745-745: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 746-746: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 747-747: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 749-749: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 750-750: Column: 1 Hard tabs (MD010, no-hard-tabs) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ed708f506f53b01e1a6e02c7ee104f5129bd7624 and 2b2b5fe6f93334c3ee53f7bf786dbb75a8c135c5. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `docs/api/log.md` (1 hunks) * `docs/whats_new.md` (3 hunks) * `log/default.go` (1 hunks) * `log/log.go` (1 hunks) * `middleware/logger/logger_test.go` (6 hunks) * `middleware/logger/utils.go` (2 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * log/default.go * middleware/logger/utils.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> <details> <summary>docs/whats_new.md</summary> 636-636: Expected: 0 or 2; Actual: 1 Trailing spaces (MD009, no-trailing-spaces) --- 717-717: Expected: 0 or 2; Actual: 1 Trailing spaces (MD009, no-trailing-spaces) --- 724-724: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 725-725: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 726-726: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 727-727: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 731-731: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 732-732: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 735-735: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 736-736: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 737-737: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 739-739: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 740-740: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 741-741: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 742-742: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 744-744: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 745-745: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 746-746: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 747-747: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 749-749: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 750-750: Column: 1 Hard tabs (MD010, no-hard-tabs) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary> <details> <summary>log/log.go (1)</summary> `57-75`: **LGTM: Changes align well with PR objectives** The new methods in `ControlLogger` and `AllLogger` interfaces effectively support the integration of custom loggers and structured logging as requested in issue #2737. The combination of configuration methods (`SetLevel`, `SetOutput`), instance access (`GetLoggerInstance`), and context support (`WithContext`) provides a flexible foundation for custom logging implementations. </details> <details> <summary>docs/api/log.md (1)</summary> `189-189`: **LGTM: Section placement is appropriate.** The new GetLoggerInstance section is well-placed at the end of the document, following a natural progression from basic to advanced topics. </details> <details> <summary>middleware/logger/logger_test.go (1)</summary> `204-204`: **Verify thread-safety of LoggerToWriter function.** The `LoggerToWriter` function is used in concurrent scenarios. Let's verify its implementation is thread-safe. <details> <summary>✅ Verification successful</summary> Let me gather more information about the `customLoggerWriter` type since it's used by `LoggerToWriter`. --- Let me check if the logger instance itself (`fiberlog.AllLogger`) is thread-safe by examining its interface and implementation. --- Let me check the CommonLogger and ControlLogger interfaces to understand the complete logger interface hierarchy. --- Based on the analysis of the code, I can now provide the final response: **LoggerToWriter implementation is thread-safe by design** The `LoggerToWriter` function is thread-safe because: 1. It creates an immutable `customLoggerWriter` struct with read-only fields 2. The `Write` method only performs read operations on these fields 3. The actual logging operations are delegated to the logger instance (`fiberlog.AllLogger`) 4. The logger interface design (CommonLogger, ControlLogger) suggests that implementations handle their own synchronization The thread-safety responsibility lies with the concrete logger implementation provided to `LoggerToWriter`, which is a standard practice in Go's io interfaces. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Check if LoggerToWriter implementation uses proper synchronization # Look for the LoggerToWriter implementation ast-grep --pattern 'func LoggerToWriter($_, $_) $$$' # Check for mutex usage in the implementation rg -A 5 'sync\.Mutex|sync\.RWMutex'Length of output: 7136
Script:
#!/bin/bash # Check customLoggerWriter implementation ast-grep --pattern 'type customLoggerWriter struct {$$$}' # Look for Write method implementation ast-grep --pattern 'func ($_) Write($_) $$$'Length of output: 2825
Script:
#!/bin/bash # Check AllLogger interface and its implementations ast-grep --pattern 'type AllLogger interface {$$$}' # Look for the default logger implementation rg -A 5 'type DefaultLogger struct'Length of output: 356
Script:
#!/bin/bash # Check CommonLogger and ControlLogger interfaces ast-grep --pattern 'type CommonLogger interface {$$$}' ast-grep --pattern 'type ControlLogger interface {$$$}' # Look for the actual logger implementation rg -A 10 'func New\(' log/Length of output: 817
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
docs/middleware/logger.md (2)
93-129: Add examples for other popular logging libraries.The section would benefit from examples showing integration with other popular logging libraries.
Add examples for zerolog and logrus:
// Example with zerolog import "github.com/rs/zerolog" logger := zerolog.New(os.Stdout) app.Use(logger.New(logger.Config{ Output: logger.LoggerToWriter(logger, log.LevelInfo), })) // Example with logrus import "github.com/sirupsen/logrus" logrusLogger := logrus.New() app.Use(logger.New(logger.Config{ Output: logger.LoggerToWriter(logrusLogger, log.LevelInfo), }))🧰 Tools
🪛 LanguageTool
[style] ~94-~94: Consider a shorter alternative to avoid wordiness.
Context: ...e Logger Middleware with Other Loggers In order to use Fiber logger middleware with other ...(IN_ORDER_TO_PREMIUM)
149-149: Fix documentation formatting and clarity.-| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | Custom logger function for integration with logging libraries (Zerolog, Zap, Logrus, etc). Defaults to Fiber's default logger if not defined. | `see default_logger.go defaultLoggerInstance` | +| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | Custom logger function for integration with logging libraries (Zerolog, Zap, Logrus, etc.). Defaults to Fiber's default logger if not defined. | `defaultLoggerInstance` |🧰 Tools
🪛 LanguageTool
[style] ~149-~149: In American English, abbreviations like “etc.” require a period.
Context: ...ogging libraries (Zerolog, Zap, Logrus, etc). Defaults to Fiber's default logger if...(ETC_PERIOD)
docs/whats_new.md (3)
634-639: Documentation for GetLoggerInstance needs more details.The documentation should provide more context about the
GetLoggerInstancemethod, including:
- Return type of the method
- Common use cases
- Example code showing how to retrieve and use the logger instance
Consider adding a code example like this:
// Get the underlying logger instance logger := app.Config.Logger.GetLoggerInstance() // Use the logger instance directly logger.Info("Direct logging through the instance")
715-752: Example code needs improvement for clarity and best practices.While the example demonstrates the integration with zap logger, it could be enhanced:
- Add comments explaining the purpose of
ExtraKeys- Show how to use the custom fields like
request_idin the actual logging- Add error handling for the
ListencallConsider updating the example:
func main() { // Create a new Fiber instance app := fiber.New() // Create a new zap logger which is compatible with Fiber AllLogger interface + // ExtraKeys allows you to log additional fields from the request context zap := fiberzap.NewLogger(fiberzap.LoggerConfig{ ExtraKeys: []string{"request_id"}, }) - // Use the logger middleware with zerolog logger + // Use the logger middleware with zap logger app.Use(logger.New(logger.Config{ Output: logger.LoggerToWriter(zap, log.LevelDebug), })) // Define a route app.Get("/", func(c fiber.Ctx) error { + // Example of using request_id in logging + c.Locals("request_id", "123") return c.SendString("Hello, World!") }) // Start server on http://localhost:3000 - app.Listen(":3000") + if err := app.Listen(":3000"); err != nil { + zap.Fatal("Server failed to start", "error", err) + } }
715-715: Add context about the relationship to issue #2737.Consider adding a note explaining how this feature addresses issue #2737's request for the logger middleware to utilize the Logger interface instead of fmt.Sprintf().
Add this context before the section:
+// This feature addresses issue #2737 by allowing the logger middleware to utilize any logger +// that implements the Logger interface, moving away from the default fmt.Sprintf logger. ### Logger
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/api/log.md(1 hunks)docs/middleware/logger.md(3 hunks)docs/whats_new.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/middleware/logger.md
[style] ~94-~94: Consider a shorter alternative to avoid wordiness.
Context: ...e Logger Middleware with Other Loggers In order to use Fiber logger middleware with other ...
(IN_ORDER_TO_PREMIUM)
[style] ~149-~149: In American English, abbreviations like “etc.” require a period.
Context: ...ogging libraries (Zerolog, Zap, Logrus, etc). Defaults to Fiber's default logger if...
(ETC_PERIOD)
🔇 Additional comments (1)
docs/middleware/logger.md (1)
167-167: LGTM!
The default configuration correctly includes the new LoggerFunc field.
…chang/fiber into feature/logger-interface
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
docs/whats_new.md (1)
715-752: Fix inconsistencies in the logger middleware example.
- The comment on line 739 mentions "zerolog" but the example uses "zap" logger.
- Consider making the log level configurable instead of hardcoding it to debug level.
Apply these improvements to make the example more robust:
// Create a new zap logger which is compatible with Fiber AllLogger interface zap := fiberzap.NewLogger(fiberzap.LoggerConfig{ ExtraKeys: []string{"request_id"}, }) + if zap == nil { + log.Fatal("Failed to initialize zap logger") + } - // Use the logger middleware with zerolog logger + // Use the logger middleware with zap logger app.Use(logger.New(logger.Config{ - Output: logger.LoggerToWriter(zap, log.LevelDebug), + Output: logger.LoggerToWriter(zap, log.LevelInfo), // Or make it configurable }))log/default_test.go (1)
224-238: Add parallel test execution and error cases.The test function could be improved by:
- Adding
t.Parallel()for better test performance- Testing error cases when type assertion fails
Apply this diff to enhance the test coverage:
func Test_Logger(t *testing.T) { + t.Parallel() + underlyingLogger := log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds) setLogger := &defaultLogger{ stdlog: underlyingLogger, depth: 4, } require.Equal(t, underlyingLogger, setLogger.Logger()) logger, ok := setLogger.Logger().(*log.Logger) require.True(t, ok) logger.SetFlags(log.LstdFlags | log.Lshortfile | log.Lmicroseconds) require.Equal(t, log.LstdFlags|log.Lshortfile|log.Lmicroseconds, setLogger.stdlog.Flags()) + + // Test invalid type assertion + invalidLogger := &defaultLogger{ + stdlog: struct{}{}, // Not a *log.Logger + depth: 4, + } + _, ok = invalidLogger.Logger().(*log.Logger) + require.False(t, ok) }middleware/logger/logger_test.go (1)
189-262: Add parallel execution for subtests.The test function could be improved by adding
t.Parallel()for each subtest to improve test performance.Apply this diff to enable parallel execution:
for _, tc := range testCases { level := strconv.Itoa(int(tc.level)) t.Run(level, func(t *testing.T) { + t.Parallel() buf.Reset()🧰 Tools
🪛 golangci-lint (1.62.2)
189-189: Test_Logger_LoggerToWriter's subtests should call t.Parallel
(tparallel)
203-203: fieldalignment: struct with 16 pointer bytes could be 8
(govet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
docs/api/log.md(1 hunks)docs/whats_new.md(3 hunks)log/default.go(1 hunks)log/default_test.go(1 hunks)log/log.go(1 hunks)middleware/logger/logger_test.go(5 hunks)middleware/logger/utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- log/default.go
- log/log.go
- middleware/logger/utils.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
middleware/logger/logger_test.go
189-189: Test_Logger_LoggerToWriter's subtests should call t.Parallel
(tparallel)
203-203: fieldalignment: struct with 16 pointer bytes could be 8
(govet)
🔇 Additional comments (2)
docs/whats_new.md (1)
634-639: LGTM! Clear and well-structured documentation.
The introduction of the new Logger method in the fiber.AllLogger interface is well documented with appropriate references.
docs/api/log.md (1)
189-204: Enhance the Logger section documentation.
The documentation needs improvements for clarity and best practices.
Apply this diff to improve the documentation:
## Logger
-You can use Logger to retrieve the logger instance. It is useful when you need to access underlying methods of the logger.
-To retrieve the Logger instance, use the following method:
+Logger returns the underlying logger implementation. This is useful when you need to:
+- Access logger-specific methods not exposed through the Fiber logger interface
+- Configure low-level logger settings
+- Integrate with external systems that expect the concrete logger type
+
+Note: This method is not guaranteed to be thread-safe. Ensure proper synchronization when modifying logger settings.
```go
logger := fiberlog.DefaultLogger() // Call DefaultLogger to get the default logger instance
-stdlogger, ok := logger.Logger().(*log.Logger) // Get the logger instance and assert it to *log.Logger
+// Safely type assert to *log.Logger
+stdlogger, ok := logger.Logger().(*log.Logger)
if !ok {
- panic("logger is not *log.Logger")
+ log.Printf("unexpected logger type: %T", logger.Logger())
+ return
}
-stdlogger.SetFlags(0) // Hide timestamp by setting flags to 0
+// Now you can safely use logger-specific methods
+stdlogger.SetFlags(log.LstdFlags | log.Lshortfile) // Example: Add file and line number to logs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/whats_new.md (1)
716-753: Consider enhancing the example code.The example code is well-structured but could benefit from the following improvements:
- Add error handling for
app.Listen()- Fix the comment that mentions "zerolog" when the example uses "zap"
Here's the suggested improvement:
// Create a new Fiber instance app := fiber.New() // Create a new zap logger which is compatible with Fiber AllLogger interface zap := fiberzap.NewLogger(fiberzap.LoggerConfig{ ExtraKeys: []string{"request_id"}, }) -// Use the logger middleware with zerolog logger +// Use the logger middleware with zap logger app.Use(logger.New(logger.Config{ Output: logger.LoggerToWriter(zap, log.LevelDebug), })) // Define a route app.Get("/", func(c fiber.Ctx) error { return c.SendString("Hello, World!") }) // Start server on http://localhost:3000 -app.Listen(":3000") +if err := app.Listen(":3000"); err != nil { + log.Fatal(err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
docs/api/log.md(2 hunks)docs/whats_new.md(3 hunks)log/log.go(1 hunks)middleware/logger/logger_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/log.md
- log/log.go
🔇 Additional comments (4)
middleware/logger/logger_test.go (4)
189-263: LGTM! Well-structured test implementation.
The test thoroughly covers different log levels and error cases, with proper setup and cleanup of resources.
189-263: Add test case for colorized log output.
As requested in the comments, please add a test case to verify the colorized output when using LoggerToWriter.
Example test case:
t.Run("colorized", func(t *testing.T) {
buf.Reset()
logger := fiberlog.DefaultLogger()
stdlogger, ok := logger.Logger().(*log.Logger)
require.True(t, ok)
stdlogger.SetFlags(0)
logger.SetOutput(buf)
app.Use(New(Config{
Format: "${error}",
Output: LoggerToWriter(logger, fiberlog.LevelError),
EnableColors: true,
}))
app.Get("/colorized", func(_ fiber.Ctx) error {
return errors.New("colorized error")
})
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/colorized", nil))
require.NoError(t, err)
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode)
require.Contains(t, buf.String(), "[Error]")
require.Contains(t, buf.String(), "colorized error")
})817-828: LGTM! Well-implemented benchmark case.
The benchmark case follows the established patterns and properly tests the performance of the new logger integration.
817-828: Refactor benchmark setup code to reduce duplication.
The setup code is duplicated between serial and parallel benchmarks. Consider extracting it into a helper function.
+func setupFiberLogBenchmark() (*fiber.App, fiber.Handler) {
+ app := fiber.New()
+ logger := fiberlog.DefaultLogger()
+ logger.SetOutput(io.Discard)
+ app.Use(New(Config{
+ Output: LoggerToWriter(logger, fiberlog.LevelDebug),
+ }))
+ handler := func(c fiber.Ctx) error {
+ return c.SendString("Hello, World!")
+ }
+ return app, handler
+}
b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) {
- app := fiber.New()
- logger := fiberlog.DefaultLogger()
- logger.SetOutput(io.Discard)
- app.Use(New(Config{
- Output: LoggerToWriter(logger, fiberlog.LevelDebug),
- }))
- app.Get("/", func(c fiber.Ctx) error {
- return c.SendString("Hello, World!")
- })
+ app, handler := setupFiberLogBenchmark()
+ app.Get("/", handler)
benchmarkSetup(bb, app, "/")
})Also applies to: 973-984
expandable example
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/whats_new.md (1)
716-758: LGTM! Well-structured logger middleware documentation with a comprehensive example.The documentation effectively demonstrates the usage of the new
LoggerToWriterhelper function with a clear example using zap logger. The code example follows best practices and includes proper imports and configuration.Consider adding a note about error handling for the
app.Listen(":3000")call in the example, as it returns an error that should be handled:- app.Listen(":3000") + log.Fatal(app.Listen(":3000"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/whats_new.md(3 hunks)
🔇 Additional comments (1)
docs/whats_new.md (1)
635-640: LGTM! Clear and concise introduction of the new Logger method.
The documentation clearly explains the new Logger method added to the fiber.AllLogger interface and provides a helpful reference to detailed documentation.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
This adds a new field for the middleware/logger to allow users to pass in a logger with fiber logger interface.
To use the fiber logger interface, users need to define how the logger will be used in
LoggerFunc.Otherwise, it falls back to use original
fmt.Sprintflogger instance.Fixes #2737
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md