🐛 bug: Use Content-Length for bytesReceived and bytesSent tags in Logger Middleware #3066
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes primarily focus on resolving an Out-Of-Memory (OOM) issue in the Go-Fiber logger middleware. The updates include enhancements to the test suite in Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3066 +/- ##
==========================================
- Coverage 83.12% 83.12% -0.01%
==========================================
Files 115 115
Lines 8332 8331 -1
==========================================
- Hits 6926 6925 -1
Misses 1076 1076
Partials 330 330
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- middleware/logger/logger_test.go (3 hunks)
- middleware/logger/tags.go (2 hunks)
Additional comments not posted (6)
middleware/logger/tags.go (2)
91-91: Approved: Correctly uses strconv.Itoa for conversionThe
TagBytesReceivedfunction correctly retrieves the content length from the request header and usesstrconv.Itoafor conversion.
94-94: Approved: Correctly uses strconv.Itoa for conversionThe
TagBytesSentfunction correctly retrieves the content length from the response header and usesstrconv.Itoafor conversion.middleware/logger/logger_test.go (4)
413-416: Approved: Correctly tests response body loggingThe
Test_Response_Bodyfunction correctly tests both GET and POST requests and verifies the response body logging.
418-442: Approved: Correctly tests bytes received and sent loggingThe
Test_Request_Bodyfunction correctly sets up a POST request with a body and verifies the logging of bytes received and sent.
461-475: Approved: Correctly tests bytes received and sent logging with different response content lengthsThe
Test_Logger_AppendUintfunction correctly sets up GET requests and verifies the logging of bytes received and sent with different response content lengths.
658-660: Approved: Correctly tests bytes sent logging with streaming responsesThe
Test_Logger_ByteSent_Streamingfunction correctly sets up a GET request with a streaming response and verifies the logging of bytes sent.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- middleware/logger/default_logger.go (2 hunks)
- middleware/logger/tags.go (1 hunks)
Additional comments not posted (3)
middleware/logger/default_logger.go (1)
153-154: LGTM! Verify the function usage in the codebase.The change to use
strconv.AppendIntinstead offasthttp.AppendUintis approved.However, ensure that all function calls to
appendIntare consistent with the new implementation.Verification successful
Verify the context around the
appendIntcalls inmiddleware/logger/tags.goto ensure they are consistent with the new implementation.The
appendIntfunction is used in three places inmiddleware/logger/tags.go. We need to confirm that these usages are consistent with the new implementation usingstrconv.AppendInt.
The function calls to
appendIntare consistent with the new implementation.The usage of
appendIntinmiddleware/logger/tags.gois correct and consistent with the new implementation usingstrconv.AppendInt.
TagBytesReceived:appendInt(output, c.Request().Header.ContentLength())TagBytesSent:appendInt(output, c.Response().Header.ContentLength())TagStatus:appendInt(output, c.Response().StatusCode())Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `appendInt` are consistent with the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new implementation. rg --type go -A 5 $'appendInt'Length of output: 1562
Script:
#!/bin/bash # Description: Verify the context around the `appendInt` calls in `middleware/logger/tags.go`. # Test: Search for the function usage and include additional context lines. rg --type go -A 10 -B 10 $'appendInt' middleware/logger/tags.goLength of output: 2067
middleware/logger/tags.go (2)
93-93: LGTM! Verify the function usage in the codebase.The change to use the
ContentLengthheader for calculating the number of bytes sent is approved.However, ensure that all function calls to
TagBytesSentare consistent with the new implementation.
90-90: LGTM! Verify the function usage in the codebase.The change to use the
ContentLengthheader for calculating the number of bytes received is approved.However, ensure that all function calls to
TagBytesReceivedare consistent with the new implementation.Verification successful
No issues found with the updated implementation of
TagBytesReceived.The function
TagBytesReceivedis not used elsewhere in the codebase, so there are no inconsistencies to verify with the new implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `TagBytesReceived` are consistent with the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new implementation. rg --type go -A 5 $'TagBytesReceived'Length of output: 873
Script:
#!/bin/bash # Description: Verify all function calls to `TagBytesReceived` are consistent with the new implementation. # Test: Search for the function usage across the entire codebase. rg 'TagBytesReceived' --type goLength of output: 210
…ger Middleware (#3066) * logger: Use Content-Length header for BytesReceived and BytesSent tags * Use strconv.AppendInt instead of fasthttp.AppendUint
Description
Content-Lengthheader when calculating thebytesSentandbytesReceivedtags in the Logger Middleware.Test_Request_Bodyfor testing thebytesReceivedtag.SetContentLengthFixes #3060
Type of change