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 AllLogger to Config #3153

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

haochunchang
Copy link

@haochunchang haochunchang commented Oct 2, 2024

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.Sprintf logger instance.

Fixes #2737

Changes introduced

  • Examples: Provide examples demonstrating the new features or changes in action.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Enhancement (improvement to existing features and functionality)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

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

Copy link

welcome bot commented Oct 2, 2024

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

Copy link
Contributor

coderabbitai bot commented Oct 2, 2024

Walkthrough

The changes in this pull request primarily enhance the Logger middleware in the Fiber framework by introducing new configuration options and methods. Key modifications include the addition of the Logger field and LoggerFunc property to the Config structure, allowing for custom logging functions and integration with various logging libraries. Additionally, a new SetFlags method has been added to both the defaultLogger struct and the ControlLogger interface, enabling the configuration of logging flags. The documentation has been updated to reflect these changes, along with new test cases and benchmarks.

Changes

File Change Summary
docs/middleware/logger.md Updated documentation for Logger middleware; added LoggerFunc and Logger properties.
log/default.go Added SetFlags(flag int) method to defaultLogger struct.
log/log.go Added SetFlags(flags int) method to ControlLogger interface.
middleware/logger/config.go Added Logger field of type fiberlog.AllLogger to Config struct.
middleware/logger/logger_test.go Added Test_Logger_Fiber_Logger test and DefaultFormatWithFiberLog benchmark.

Assessment against linked issues

Objective Addressed Explanation
Use Logger interface in logger middleware (#2737)

Possibly related PRs

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • efectn

Poem

In the forest where the loggers play,
New flags and functions brighten the day.
With Fiber's might, we leap and bound,
Custom logs and tests abound!
Hooray for changes, let them ring,
A joyful hop for everything! 🐇✨


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.

@haochunchang haochunchang force-pushed the feature/logger-interface branch 2 times, most recently from 803191e to b5aa388 Compare October 5, 2024 02:47
@haochunchang haochunchang marked this pull request as ready for review October 5, 2024 02:49
@haochunchang haochunchang requested a review from a team as a code owner October 5, 2024 02:49
@haochunchang haochunchang requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team October 5, 2024 02:49
@gaby gaby added this to the v3 milestone Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.11%. Comparing base (48e82e7) to head (3dd20b4).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
log/default.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3153      +/-   ##
==========================================
- Coverage   82.38%   82.11%   -0.28%     
==========================================
  Files         113      113              
  Lines        8466    11005    +2539     
==========================================
+ Hits         6975     9037    +2062     
- Misses       1088     1566     +478     
+ Partials      403      402       -1     
Flag Coverage Δ
unittests 82.11% <0.00%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@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: 5

🧹 Outside diff range and nitpick comments (7)
log/log.go (2)

59-59: Alignment with PR objectives

While the addition of SetFlags(flags int) to the ControlLogger interface 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:

  1. Implementing a way for users to provide their custom logger implementation.
  2. Ensuring that the custom logger can be used in place of the default logger throughout the Fiber framework.
  3. 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 steps

The change to add SetFlags(flags int) to the ControlLogger interface 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:

  1. Review other files in the PR to ensure the full implementation of custom logger integration.
  2. If not already done, implement a mechanism for users to provide and use their custom logger implementations.
  3. Update or create documentation explaining how users can implement and integrate custom loggers.
  4. 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 Logger field of type fiberlog.AllLogger is a good addition that aligns with the PR objectives. It allows users to provide custom loggers that implement the AllLogger interface.

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.AllLogger

This 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 configDefault function should be updated to handle the new Logger field. 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 nil is 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.Logger and cfg.LoggerFunc if both are provided.


Inconsistency between Config struct and ConfigDefault initialization

The Config struct in middleware/logger/config.go does not include a Logger field, yet ConfigDefault is initializing fields like LoggerFunc and BeforeHandlerFunc. This discrepancy can lead to runtime errors or unexpected behavior. Please ensure that the Config struct is updated to include the Logger field or adjust ConfigDefault to align with the existing struct definition.

🔗 Analysis chain

Line range hint 97-108: Consider updating ConfigDefault for the new Logger field.

The ConfigDefault variable has not been updated to include a default value for the new Logger field. While this might be intentional, it's worth considering either:

  1. Setting a default value (e.g., nil or a default logger instance) in ConfigDefault, or
  2. Adding a comment explaining why the Logger field 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 nil logger 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 SetFlags looks good and aligns with the PR objectives by enhancing the logger's capabilities. However, there are a couple of improvements we should make:

  1. As this is an exported method (part of the public API), please add a comment to document its purpose and usage.
  2. 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 tests

docs/middleware/logger.md (1)

111-111: Approve addition of LoggerFunc with minor suggestion.

The new LoggerFunc property 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

📥 Commits

Files that changed from the base of the PR and between 85a5fb8 and b5aa388.

📒 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 fiberlog is correctly added and aliased. This import is necessary for the new Logger field in the Config struct.


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 Logger field in the Config struct allows for custom logger integration.

To ensure a complete implementation:

  1. Verify that other files in the middleware package use this new Logger field appropriately.
  2. Update any documentation or examples to reflect this new capability.
  3. 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 Config struct is used or where the middleware is initialized, which might need updates to accommodate the new Logger field.

log/default.go (1)

213-215: Verify full implementation of PR objectives

The addition of the SetFlags method is a good enhancement to the logger's capabilities. However, the PR objectives mention using the Logger interface instead of fmt.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.Sprintf within logging functions have been successfully replaced with the Logger interface. No remaining fmt.Sprintf usages 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 tests

middleware/logger/logger_test.go (2)

18-18: Import statement for 'fiberlog' package is correctly added

The import of fiberlog package is necessary for integrating the Fiber logger. The import statement is properly added.


907-931: Benchmark 'DefaultFormatWithFiberLog' is correctly implemented

The new benchmark case DefaultFormatWithFiberLog is properly added to Benchmark_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)

@@ -56,6 +56,7 @@ type CommonLogger interface {
type ControlLogger interface {
SetLevel(level Level)
SetOutput(w io.Writer)
SetFlags(flags int)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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 SetFlags method in log/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:

  1. 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.
  2. The flags parameter 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

@@ -108,6 +108,8 @@ Writing to os.File is goroutine-safe, but if you are using a custom Output that
| TimeZone | `string` | TimeZone can be specified, such as "UTC" and "America/New_York" and "Asia/Chongqing", etc | `"Local"` |
| TimeInterval | `time.Duration` | TimeInterval is the delay before the timestamp is updated. | `500 * time.Millisecond` |
| Output | `io.Writer` | Output is a writer where logs are written. | `os.Stdout` |
| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | You can use custom loggers with Fiber by using this field. This field is really useful if you're using Zerolog, Zap, Logrus, apex/log etc. If you don't define anything for this field, it'll use default logger of Fiber. | `see default_logger.go defaultLoggerInstance` |
| Logger | `fiberlog.AllLogger` | | |
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add description for the Logger property.

The new Logger property of type fiberlog.AllLogger has been added, which aligns with the PR objectives to enhance logging capabilities. However, the description for this property is missing, which may lead to confusion for users.

Please add a description for the Logger property that explains:

  1. The purpose of this property
  2. How it differs from or complements the LoggerFunc property
  3. A brief explanation of what fiberlog.AllLogger is and how to use it

Additionally, consider adding an example in the "Examples" section that demonstrates how to use this new Logger property.

middleware/logger/logger_test.go Outdated Show resolved Hide resolved
middleware/logger/logger_test.go Outdated Show resolved Hide resolved
customLoggerFunc := func(c fiber.Ctx, data *Data, cfg Config) error {
cfg.Logger.SetOutput(cfg.Output)
cfg.Logger.SetFlags(0)
cfg.Logger.Error(data.ChainErr.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential nil pointer dereference when accessing 'data.ChainErr.Error()'

At line 196, calling data.ChainErr.Error() without checking if data.ChainErr is nil could lead to a panic if ChainErr is nil. To prevent this, add a nil check before calling Error().

Apply this diff to safely handle a nil data.ChainErr:

 cfg.Logger.SetFlags(0)
-if data.ChainErr != nil {
-    cfg.Logger.Error(data.ChainErr.Error())
-}
+if data.ChainErr != nil {
+    cfg.Logger.Error(data.ChainErr.Error())
+} else {
+    cfg.Logger.Error("No chain error occurred")
+}
 return nil

This ensures that the logger handles cases where ChainErr might be nil.

Committable suggestion was skipped due to low confidence.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 5, 2024

@haochunchang thx for the PR, we will have a look

can you check the pre hints from our review ai
hopefully these make sense, otherwise we simply ignore them or have to fine-tune them

Copy link
Contributor

@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 (5)
docs/middleware/logger.md (2)

112-112: Enhance documentation for the Logger property

While the addition of the Logger property is a valuable enhancement, the documentation could be improved to provide more clarity and guidance for users. Consider the following suggestions:

  1. Provide a brief explanation or a link to the documentation for the AllLogger interface.
  2. Clarify the interaction between Logger and LoggerFunc when both are defined. Which one takes precedence?
  3. Add an example in the "Examples" section demonstrating how to use the Logger property with a custom logger implementing the AllLogger interface.

These improvements will help users better understand and utilize the new logging capabilities.


Line range hint 1-112: Add examples for new logging features

The documentation has been updated to include the new LoggerFunc and Logger properties, which is great. However, to fully align with the PR objectives and provide maximum value to users, consider adding specific examples that demonstrate:

  1. How to use the LoggerFunc property with a custom logging function.
  2. How to implement and use a custom logger with the Logger property.

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_Logger effectively 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

📥 Commits

Files that changed from the base of the PR and between b5aa388 and 5164018.

📒 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: New LoggerFunc property well-documented

The addition of the LoggerFunc property 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!

Copy link
Contributor

@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 (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 LoggerFunc and Logger properties 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 of LoggerFunc property!

The description of the LoggerFunc property 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: See defaultLoggerInstance in default_logger.go"


125-125: Good addition of Logger property, addressing previous feedback!

The description of the Logger property effectively explains its purpose and relationship with the LoggerFunc. This addition aligns with the PR objectives and addresses the previous review comment requesting a description for this property.

To further improve clarity:

  1. Consider adding a brief explanation of what the AllLogger interface entails.
  2. Provide a short example or link to documentation about implementing the AllLogger interface.
  3. Clarify the default value. Instead of "nil", consider stating "Default: None (nil)".
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5164018 and 8929780.

📒 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!

@haochunchang
Copy link
Author

Hi, I have addressed a few comments from the coderabbitai.
Please have a look when you have time. Thanks!

@ReneWerner87
Copy link
Member

Thx will do it tomorrow

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

@haochunchang
thank you for the effort, have not made any comments


app.Use(New(Config{
Output: buf,
Logger: fiberlog.DefaultLogger(),
Copy link
Member

Choose a reason for hiding this comment

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

please add a second test where you inject another logger and show that it receives the data

// If you don't define LoggerFunc, it'll use default logger of Fiber without using this field.
//
// Optional. Default: nil
Logger fiberlog.AllLogger
Copy link
Member

Choose a reason for hiding this comment

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

where is this used in the middleware code ?

Copy link
Member

Choose a reason for hiding this comment

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

@haochunchang @efectn
I think I understand, instead of the buffer in the default logger function another logger is used

three remarks

  1. would not add logger to the config if it is only used in the custom variant of the consumer, then it can also use the logger instance from outside, because the logger is not relevant for the existing default logging code
  2. if we get the logger instance, we should also use it in the default code, regardless of whether the custom logger function is set or not
  3. what is the difference between the logger and the output property? both define the target
    maybe you can use output for this?

what do you think about this?

Copy link
Author

Choose a reason for hiding this comment

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

I agree that we should also use the logger instance in the default code.

But I haven’t figured out how to use the methods of the AllLogger interface to achieve the same functionality of the original default logger. Especially in the part:

for i, logFunc := range data.LogFuncChain {
switch {
case logFunc == nil:
buf.Write(data.TemplateChain[i])
case data.TemplateChain[i] == nil:
_, err = logFunc(buf, c, data, "")
default:
_, err = logFunc(buf, c, data, utils.UnsafeString(data.TemplateChain[i]))
}
if err != nil {
break
}

@@ -108,6 +121,8 @@ Writing to os.File is goroutine-safe, but if you are using a custom Output that
| TimeZone | `string` | TimeZone can be specified, such as "UTC" and "America/New_York" and "Asia/Chongqing", etc | `"Local"` |
| TimeInterval | `time.Duration` | TimeInterval is the delay before the timestamp is updated. | `500 * time.Millisecond` |
| Output | `io.Writer` | Output is a writer where logs are written. | `os.Stdout` |
| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | You can use custom loggers with Fiber by using this field. This field is really useful if you're using Zerolog, Zap, Logrus, apex/log etc. If you don't define anything for this field, it'll use the default logger of Fiber. | `see default_logger.go defaultLoggerInstance` |
| Logger | `fiberlog.AllLogger` | Logger allows the use of a custom logger that implements the AllLogger interface. This field can be used in the LoggerFunc to do the logging. If you don't define LoggerFunc, this field will be ignored. | `nil` |
Copy link
Member

Choose a reason for hiding this comment

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

please add the default value of this new property to the default config

@efectn
Copy link
Member

efectn commented Oct 10, 2024

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 Output field. what do you think @haochunchang ?

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,
	}
}

@haochunchang
Copy link
Author

Cool. Then we can use this wrapper like the following:

cfg.Output = LoggerToWriter(logger, level)

Is that what you mean?

@efectn
Copy link
Member

efectn commented Oct 14, 2024

Cool. Then we can use this wrapper like the following:

cfg.Output = LoggerToWriter(logger, level)

Is that what you mean?

Yes it is

@haochunchang
Copy link
Author

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.

Copy link
Contributor

@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: 5

🧹 Outside diff range and nitpick comments (3)
log/default.go (1)

213-215: LGTM! Consider adding a comment for clarity.

The SetFlags method 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 readability

The updated documentation provides valuable information and examples for users. To further enhance its readability and organization, consider the following suggestions:

  1. Move the new custom logger example (lines 92-117) to the "Examples" section, grouping it with other usage examples.
  2. Add a subsection header for "Custom Logger Integration" before the new example to clearly separate it from other examples.
  3. Update the table of contents (if present) to reflect the new additions.
  4. 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

📥 Commits

Files that changed from the base of the PR and between 598b9d8 and 3dd20b4.

📒 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 SetFlags method 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 the AllLogger interface across the package.

docs/middleware/logger.md (1)

138-138: Address previous review comments

Two previous review comments need attention:

  1. The suggestion to add a description for the Logger property. It seems this property might have been renamed to LoggerFunc or removed. Please clarify this change and update the documentation accordingly.

  2. 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 Logger property and update the "Default Config" section to include the new LoggerFunc property?

🧰 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 integration

The new test function Test_Logger_Fiber_Logger correctly 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.

Comment on lines +213 to +215
func (l *defaultLogger) SetFlags(flag int) {
l.stdlog.SetFlags(flag)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

@@ -108,6 +135,7 @@ Writing to os.File is goroutine-safe, but if you are using a custom Output that
| TimeZone | `string` | TimeZone can be specified, such as "UTC" and "America/New_York" and "Asia/Chongqing", etc | `"Local"` |
| TimeInterval | `time.Duration` | TimeInterval is the delay before the timestamp is updated. | `500 * time.Millisecond` |
| Output | `io.Writer` | Output is a writer where logs are written. | `os.Stdout` |
| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | You can use custom loggers with Fiber by using this field. This field is really useful if you're using Zerolog, Zap, Logrus, apex/log etc. If you don't define anything for this field, it'll use the default logger of Fiber. | `see default_logger.go defaultLoggerInstance` | |
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Approve LoggerFunc addition. Fix table formatting.

The new LoggerFunc configuration option is a great addition that aligns with the PR objectives. It provides flexibility for users to integrate various logging libraries.

The table formatting needs to be adjusted to maintain consistency. Please split the description into multiple lines or consider using a list format for better readability. For example:

| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | Custom logger function. Useful for integrating logging libraries like Zerolog, Zap, Logrus, etc. If not defined, Fiber's default logger is used. | `see default_logger.go defaultLoggerInstance` |
🧰 Tools
🪛 Markdownlint

138-138: Expected: 4; Actual: 5; Too many cells, extra data will be missing
Table column count

(MD056, table-column-count)

Comment on lines +772 to +784
b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) {
app := fiber.New()
logger := fiberlog.DefaultLogger()
logger.SetOutput(io.Discard)
app.Use(New(Config{
Output: LoggerToWriter(logger),
}))
app.Get("/", func(c fiber.Ctx) error {
return c.SendString("Hello, World!")
})
benchmarkSetup(bb, app, "/")
})

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor benchmark tests to eliminate duplicate code

The benchmark cases DefaultFormatWithFiberLog in both Benchmark_Logger (lines 772-784) and Benchmark_Logger_Parallel (lines 928-940) have identical setup code. This duplication can be avoided by refactoring the common code into a helper function, enhancing maintainability and reducing potential for errors.

You can extract the common setup code into a helper function:

func setupBenchmarkWithFiberLog() (*fiber.App, func(fiber.Ctx) error) {
	app := fiber.New()
	logger := fiberlog.DefaultLogger()
	logger.SetOutput(io.Discard)
	app.Use(New(Config{
		Output: LoggerToWriter(logger),
	}))
	handler := func(c fiber.Ctx) error {
		return c.SendString("Hello, World!")
	}
	return app, handler
}

Then use this helper function in your benchmarks:

b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) {
	app, handler := setupBenchmarkWithFiberLog()
	app.Get("/", handler)
	benchmarkSetup(bb, app, "/")
})

This approach promotes code reuse and simplifies future modifications.

Also applies to: 928-940

Comment on lines +194 to +196
func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
return &dumbLogger{logger: customLogger}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate 'customLogger' parameter in 'LoggerToWriter'

The LoggerToWriter function does not check if the customLogger parameter is nil. Passing a nil customLogger will result in a dumbLogger with a nil logger, which could cause a panic when Write is called. Add a check to ensure customLogger is not nil before creating the dumbLogger.

Apply this diff to validate the parameter:

func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
+	if customLogger == nil {
+		return nil
+	}
	return &dumbLogger{logger: customLogger}
}

Alternatively, you might want to return an error or use a default logger if customLogger is nil.

📝 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 LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
return &dumbLogger{logger: customLogger}
}
func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
if customLogger == nil {
return nil
}
return &dumbLogger{logger: customLogger}
}

Comment on lines +185 to +193
type dumbLogger struct {
logger fiberlog.AllLogger
}

func (l *dumbLogger) Write(p []byte) (n int, err error) {
l.logger.Error(string(p))
return len(p), nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add nil check for 'l.logger' in 'dumbLogger.Write'

In the Write method of dumbLogger, if l.logger is nil, calling l.logger.Error(string(p)) will result in a nil pointer dereference, causing a runtime panic. To ensure robustness, add a check to verify that l.logger is not nil before attempting to log.

Apply this diff to add the nil check:

func (l *dumbLogger) Write(p []byte) (n int, err error) {
+	if l.logger == nil {
+		return 0, errors.New("logger is nil")
+	}
	l.logger.Error(string(p))
	return len(p), nil
}
📝 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
type dumbLogger struct {
logger fiberlog.AllLogger
}
func (l *dumbLogger) Write(p []byte) (n int, err error) {
l.logger.Error(string(p))
return len(p), nil
}
type dumbLogger struct {
logger fiberlog.AllLogger
}
func (l *dumbLogger) Write(p []byte) (n int, err error) {
if l.logger == nil {
return 0, errors.New("logger is nil")
}
l.logger.Error(string(p))
return len(p), nil
}

@ReneWerner87
Copy link
Member

Wrapper method sounds good
If we can put it directly into the code of the logger middleware that would be good
And then tests and examples for the documentation

Comment on lines +93 to +113
type dumbLogger struct {
logger fiberlog.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))
}
return len(p), nil
}

func LoggerToWriter(customLogger fiberlog.AllLogger, level fiberlog.Level) io.Writer {
return &dumbLogger{
logger: customLogger,
level: level,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@haochunchang can we put that or a better generic wrapper in the code so that not every dev has to create this struct? or would that be too undynamic?

what do you think? @gaby @efectn

Copy link
Member

Choose a reason for hiding this comment

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

maybe we should recommend something like this ?

func LoggerToWriter(customLogger fiberlog.AllLogger, level fiberlog.Level) io.Writer {
	return &struct {
		Write func(p []byte) (n int, err error)
	}{
		Write: func(p []byte) (n int, err error) {
			switch level {
			case fiberlog.LevelDebug:
				customLogger.Debug(string(p))
			case fiberlog.LevelError:
				customLogger.Error(string(p))
			}
			return len(p), nil
		},
	}
}

then we really don´t need extra code in the codebase

Copy link
Author

Choose a reason for hiding this comment

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

I think we can provide the LoggerToWriter in the example to show how to use fiberlog interface in the middleware.

Copy link
Member

Choose a reason for hiding this comment

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

ok @haochunchang pls adjust it a little bit

Copy link
Author

Choose a reason for hiding this comment

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

I’ve tried to condensed into a single function wrapper that return io.Writer but it seems it still need to declare the Write method in order to satisfy the io.Writer interface.

Copy link
Member

Choose a reason for hiding this comment

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

@haochunchang yeah, but in my example its there
so can you update the PR with this code snippet ?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean adding it into the example?
Cause I failed to adopt this into the unit tests 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

🚀 [Feature]: middleware/logger: use Logger interface
4 participants