🔥 feat: Add Filter option to logger middleware #3333
Conversation
WalkthroughThe pull request introduces a new logging filter feature to the Fiber middleware. A Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (5)
✨ Finishing Touches
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3333 +/- ##
=======================================
Coverage 84.21% 84.21%
=======================================
Files 116 116
Lines 11578 11584 +6
=======================================
+ Hits 9750 9756 +6
Misses 1397 1397
Partials 431 431
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…fiber into jiejaitt-feature/filter-logger
|
@ReneWerner87 hi, there |
did you also run this local with the “-race” flag ? |
I just ran them locally with |
Using |
Pretty sure this is related to re-using the same |
|
ok, unit-tests are passing now. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/middleware/logger.md (1)
161-161: Fix missing colon in the default configuration.There's a syntax issue in the ConfigDefault example - the colon is missing after Filter.
- Filter nil, + Filter: nil,middleware/logger/logger_test.go (1)
183-187: Consider adding more diverse test cases for the Filter functionality.The current tests only verify filtering based on status codes. Consider adding tests for other scenarios, such as filtering based on request path, method, or headers.
middleware/logger/config.go (1)
23-27: Documentation for the Filter property is clear, but has a minor inaccuracy.The comment describes the filter function as returning true to write the log, but looking at the implementation in default_logger.go, the condition is
!cfg.Filter(c)meaning it skips logging if the filter returns false.To make this clearer and consistent with the implementation:
- // Filter is a function that is called before writing the log string. - // If it returns true, the log will be written; otherwise, it will be skipped. + // Filter is a function that is called before writing the log string. + // If it returns false, the log will be skipped; otherwise, it will be written.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/logger.md(2 hunks)middleware/logger/config.go(3 hunks)middleware/logger/default_logger.go(1 hunks)middleware/logger/logger_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
middleware/logger/default_logger.go (1)
93-96: Conditional logging filter implementation looks good.The filtering logic is correctly implemented with proper nil check and logical flow. This allows for selective logging based on custom criteria, which is a useful feature to reduce noise in logs.
docs/middleware/logger.md (1)
142-142: Documentation for the new Filter property is clear and well-formatted.The new Filter property is properly documented with a clear description of its purpose and behavior.
middleware/logger/config.go (2)
107-107: ConfigDefault correctly includes the new Filter field.The default setting of nil is appropriate for this optional functionality.
136-138: Proper initialization of Filter in configDefault function.The Filter property is correctly initialized with the default value if not provided.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@JIeJaitt Add a section in docs/whats_new.md should be ready to merge after that. |
|
@gaby I thought about it and you are right. The filter() function can focus on the decision-making logic without having to care about the specific processing details of the log string. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/whats_new.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: Compare
- GitHub Check: repeated
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/whats_new.md (1)
943-961:⚠️ Potential issueCritical issue: Inconsistent filter example logic.
The documentation states that when the Filter function returns true the log will be written, and when it returns false the log will be skipped. In the provided examples, the second example (logging only for status 200) is consistent with this behavior. However, in the first example the comment indicates that logging should be skipped for 404 requests, but the code returns:
return c.Response().StatusCode() == fiber.StatusNotFoundThis condition returns true when the status code is 404, meaning a 404 response would be logged rather than skipped. To align the logic with the intended behavior (i.e., skip logging for 404 responses), consider inverting the condition. For example:
- return c.Response().StatusCode() == fiber.StatusNotFound + return c.Response().StatusCode() != fiber.StatusNotFoundThis change would ensure that the log is written only when the status code is not 404, thereby skipping 404 responses as the comment suggests.
🧹 Nitpick comments (1)
docs/middleware/logger.md (1)
142-142: New Filter Property Documentation Addition
The configuration table now includes theFilterproperty with the function signaturefunc(fiber.Ctx) booland a clear description. This documentation clearly explains that the function is invoked before writing the log output and will skip logging if it returns false. Consider adding a brief usage example in the "Examples" section to illustrate how to leverage this filter in a real scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/middleware/logger.md(2 hunks)docs/whats_new.md(1 hunks)middleware/logger/config.go(3 hunks)middleware/logger/default_logger.go(1 hunks)middleware/logger/logger_test.go(1 hunks)
🔇 Additional comments (7)
middleware/logger/default_logger.go (1)
18-21: Filter implementation looks correct.This implementation adds a filter capability to the logger, allowing conditional logging based on request context. The placement at the beginning of the function is efficient as it avoids unnecessary processing for requests that should be skipped.
middleware/logger/logger_test.go (1)
174-224: Test structure looks good and effectively validates the filter functionality.The test implementation properly verifies the filter functionality with two distinct test cases:
- Verifying that logs are written for 404 status codes when filtered to only log those
- Verifying that logs are not written for 200 status codes with the same filter
The tests correctly use a single middleware instance with both the filter and output capture, which accurately tests the filter's behavior.
middleware/logger/config.go (4)
23-27: Well-defined and properly documented feature addition.The new
Filterfunction field is well-designed and appropriately documented. It provides a clean extension point for conditionally logging requests based on custom logic, which addresses issue #3300 as mentioned in the PR objectives.
107-107: Appropriate default value for the new Filter field.Setting the default value to
nilis appropriate as it:
- Maintains backward compatibility
- Makes the feature optional
- Follows the pattern of other optional function fields in the Config struct
136-138: Proper implementation of default value handling.The implementation correctly follows the established pattern for handling default values in the
configDefaultfunction. This ensures consistent behavior when users don't explicitly provide a filter function.
23-27:✅ Verification successful
Verify usage examples are provided in documentation.
The implementation looks good, but users might benefit from usage examples in the documentation showing how to implement filter functions for common scenarios (e.g., filtering by status code, path, etc.).
🏁 Script executed:
#!/bin/bash # Check if examples are provided in the logger middleware documentation # Look for Filter examples in docs echo "Checking for Filter usage examples in documentation..." grep -A 10 "Filter" docs/middleware/logger.md || echo "No Filter examples found in logger documentation" # Check if the new feature is mentioned in what's new document echo "Checking for mention in what's new document..." grep -A 5 "Filter" docs/whats_new.md || echo "No mention of Filter in what's new document"Length of output: 3951
Documentation Examples Verified
The Filter usage examples are clearly present in both
docs/middleware/logger.mdanddocs/whats_new.md, demonstrating common scenarios like filtering based on status codes. No further action is required.docs/middleware/logger.md (1)
161-161: Default Config Update: 'Filter' Initialization
The default configuration now initializes theFilterproperty tonil, which preserves the existing logging behavior when no filter is provided. This is consistent with the documented default in the configuration table.
| app.Use(logger.New(logger.Config{ | ||
| Filter: func(c fiber.Ctx) bool { | ||
| // Skip logging for 404 requests | ||
| return c.Response().StatusCode() == fiber.StatusNotFound |
There was a problem hiding this comment.
Shouldn't this be != ?
If its 404 it returns True, which the defaultLogger negates into False and doesn't filter
| @@ -171,6 +171,58 @@ func Test_Logger_Done(t *testing.T) { | |||
| require.Positive(t, buf.Len(), 0) | |||
| } | |||
|
|
|||
| func Test_Logger_Filter(t *testing.T) { | |||
There was a problem hiding this comment.
I'm confused about how this is working.
If the condition is True, it will become False in defaultLogger. But we dont check for that in the test?
There was a problem hiding this comment.
@gaby Thanks for the heads up. I've just checked and found a few things that need to be refined, it's almost midnight here, I'll submit a more complete unit test and documentation note tomorrow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/whats_new.md (1)
921-925: Fix inconsistency between comment and code.The comment states "log status code >= 400" but the code actually checks for status codes >= 404 (StatusNotFound). For accurate documentation, you should either update the comment or modify the condition.
- // log status code >= 400 + // log status code >= 404 return c.Response().StatusCode() >= fiber.StatusNotFoundAlternatively, to match the comment:
// log status code >= 400 - return c.Response().StatusCode() >= fiber.StatusNotFound + return c.Response().StatusCode() >= fiber.StatusBadRequestmiddleware/logger/logger_test.go (1)
174-176: Test function documentation could be improved.Consider adding a documentation comment to explain the purpose of this test function.
+// Test_Logger_Filter tests the Filter functionality of the logger middleware. +// It verifies that logs are written or skipped based on the filter condition. func Test_Logger_Filter(t *testing.T) { t.Parallel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/middleware/logger.md(2 hunks)docs/whats_new.md(1 hunks)middleware/logger/logger_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (7)
docs/whats_new.md (3)
915-916: Filter function documentation looks good.The description clearly explains the purpose of the Filter function: it acts as a condition to determine whether a request should be logged.
935-940: Ensure this filter works as expected.The comment and implementation align correctly. When the filter returns true (status != 200), the log will be written, which means logs will be skipped for 200 status codes.
928-932:❓ Verification inconclusive
Inconsistent filtering logic.
This example has a potential issue with the filter logic. The comment says "log status code == 404", but the filter is returning true when the status is 404. The documentation at line 915-916 states that when the filter returns true, the log will be written. However, there seems to be confusion in past review comments about how this interacts with the defaultLogger.
Let's verify the implementation to ensure this works as expected:
🏁 Script executed:
#!/bin/bash # Check how the Filter is used in the logger middleware implementation # Search for places where the Filter is checked rg -A 3 "if c\.Filter" middleware/logger/Length of output: 43
Action Required: Verify Logger Filter Usage Consistency
The documentation snippet in
docs/whats_new.md(lines 928–932) shows a logger configuration with aFilterfunction that returns true when the response status is equal tofiber.StatusNotFound. According to the docs (lines 915–916), returning true should trigger logging. However, our initial search for usage ofc.Filterin the logger middleware (usingrg -A 3 "if c\.Filter" middleware/logger/) did not produce any output.Please manually verify the logger middleware implementation—especially within the
logger.Newconfiguration in themiddleware/logger/directory—to ensure that the Filter logic is applied as expected and interacts correctly with the default logger behavior.middleware/logger/logger_test.go (2)
177-198: Test implementation for 404 filters looks good.The test correctly sets up a logger middleware with a filter that only logs 404 responses, and then verifies that the output contains "404". This confirms the filter behavior works as expected.
200-225: Test implementation for 200 filter looks good.The test correctly sets up a filter that logs everything except 200 responses, and verifies that the output does not contain "200". This confirms that the filter successfully skips logging OK responses.
docs/middleware/logger.md (2)
142-142: Filter property documentation looks good.The description clearly explains what the Filter function does and how it affects logging behavior.
161-161:⚠️ Potential issueFix syntax error in ConfigDefault.
There's a missing comma after
nilin the ConfigDefault declaration, which would cause a compilation error.- Filter nil, + Filter: nil,Likely an incorrect or invalid review comment.
Description
The
Filteris a function that is called before the log string for a request is written to Output. If it returns true, the log will be written; otherwise, it will be skipped.Fixes #3300