fix(logging): only log GET requests at debug level #3237
Conversation
WalkthroughThe changes in this pull request focus on simplifying the logging behavior in the HTTP server's router. Modifications were made to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPRouter
participant FilteredLoggingHandler
participant Logger
Client->>HTTPRouter: Send HTTP Request
HTTPRouter->>FilteredLoggingHandler: Handle Request
FilteredLoggingHandler->>Logger: Check Logging Level
alt Logging Level < DebugLevel
FilteredLoggingHandler-->>HTTPRouter: Serve Request without Logging
else Logging Level >= DebugLevel
FilteredLoggingHandler->>Logger: Log Request
FilteredLoggingHandler-->>HTTPRouter: Serve Request with Logging
end
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (1)
util/util.go (1)
Line range hint
478-482: Consider improving readability with early return.The logic is correct but could be more readable with an early return pattern.
Consider this alternative structure:
-if logrus.GetLevel() < logrus.DebugLevel { - h.handler.ServeHTTP(w, req) - return -} +if logrus.GetLevel() >= logrus.DebugLevel { + h.loggingHandler.ServeHTTP(w, req) + return +} +h.handler.ServeHTTP(w, req)This makes it clearer that debug-level logging is the special case, while non-logging is the default behavior for GET requests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/daemon.go (1 hunks)
- util/util.go (2 hunks)
🔇 Additional comments (2)
app/daemon.go (1)
257-257: LGTM! Verify logging behavior in util.FilteredLoggingHandler.The simplified logging setup aligns with the PR objective to suppress GET request logs unless debug level is enabled.
Let's verify the FilteredLoggingHandler implementation:
✅ Verification successful
Implementation correctly handles GET request logging based on log level
The FilteredLoggingHandler implementation in util.go aligns perfectly with the PR objectives:
- GET requests are only logged when log level is debug or higher
- All other HTTP methods are logged normally
- Implementation is clean and straightforward using logrus log levels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the FilteredLoggingHandler implementation in util package # Expected: Should find the new implementation that handles GET request logging based on log level # Check the FilteredLoggingHandler implementation rg -A 10 "func FilteredLoggingHandler" "util/"Length of output: 523
Script:
#!/bin/bash # Let's get more context around the GET request handling logic rg -A 15 'case "GET":' "util/"Length of output: 737
util/util.go (1)
467-467: LGTM! Function signature simplified.The removal of the
filteredPathsparameter simplifies the API while maintaining functionality through log level control.
james-munson
force-pushed
the
6702-rest-logging
branch
from
3cafe9e
to
d0572cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
util/util.go (1)
478-481: LGTM! Consider adding a comment explaining the logging behavior.The implementation correctly suppresses GET request logging unless debug level is enabled. Consider adding a comment to explain this behavior for future maintainers.
switch req.Method { case "GET": + // Only log GET requests when debug logging is enabled to reduce log volume if logrus.GetLevel() < logrus.DebugLevel { h.handler.ServeHTTP(w, req) return }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/daemon.go (1 hunks)
- util/util.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/daemon.go
🔇 Additional comments (1)
util/util.go (1)
467-467: Verify the impact of signature change.The removal of
filteredPathsparameter is a breaking change that aligns with the PR objective of using log levels instead of path filtering.Let's verify the impact on existing callers:
✅ Verification successful
Breaking change detected but impact is minimal
The signature change of
FilteredLoggingHandleris a breaking change, but the impact is contained:
- There is only one direct usage in
app/daemon.gowhich already uses the new signature format (without thefilteredPathsparameter)- The old implementation exists in the vendor directory (
vendor/github.com/longhorn/longhorn-engine/pkg/util/util.go) but this is a vendored dependency and won't be affected by local changes- No test files are using this function
The change is safe to proceed with as the only caller has already been updated to match the new signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing calls to FilteredLoggingHandler rg "FilteredLoggingHandler\(" -A 2Length of output: 698
Script:
#!/bin/bash # Let's check if there are any other files in the repository that might be importing and using this function rg "github\.com.*util\.FilteredLoggingHandler" -l # Also check for any tests that might be affected rg "FilteredLoggingHandler" --type=testLength of output: 120
Script:
#!/bin/bash # Let's check for test files specifically fd ".*_test\.go$" --exec rg "FilteredLoggingHandler" {} # Let's also check the content of app/daemon.go to understand the usage context cat app/daemon.goLength of output: 10598
Signed-off-by: James Munson <james.munson@suse.com>
derekbit
force-pushed
the
6702-rest-logging
branch
from
d0572cd
to
38f5fde
Compare
|
@mergify backport v1.7.x v1.6.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Part of longhorn/longhorn#6702
What this PR does / why we need it:
Suppress logging of incoming REST GET queries unless log level is debug or more. They can overwhelm all other logging.
Special notes for your reviewer:
this should backport to 1.6 and 1.7.
Additional documentation or context