🔥 feat: Add Skip function to logger middleware #3333
Conversation
WalkthroughThe pull request modifies the logger middleware configuration in the Fiber framework by renaming the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 83.66% 83.67%
=======================================
Files 118 118
Lines 11711 11718 +7
=======================================
+ Hits 9798 9805 +7
Misses 1485 1485
Partials 428 428
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…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. |
gaby
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
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.
|
@JIeJaitt I updated the logic per recommendation from |
gaby
changed the title
|
Renamed:
These now match the official expressjs middleware https://github.com/expressjs/morgan |
|
👍 LGTM |
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