refactors HTTP server initialization, improves logging middleware, and updates documentation #1

2025-08-24T15:46:49Z

rmcguire commented

2025-08-24 15:46:49 +00:00

No description provided.

rmcguire added 1 commit 2025-08-24 15:46:49 +00:00

refactors HTTP server initialization, improves logging middleware, and updates documentation a972953040

rmcguire commented

2025-08-24 15:46:57 +00:00

/issues

rmcguire commented

2025-08-24 15:46:58 +00:00

High-Level Summary:
This pull request primarily focuses on enhancing the clarity and maintainability of the HTTP server package. It introduces comprehensive package-level documentation and refines existing function comments for better understanding. A key improvement is the optimization of HTTP response logging, where response bodies are now explicitly excluded for 204 No Content status codes, preventing unnecessary log clutter. Additionally, the comment for the Flush method has been clarified to accurately reflect its role in implementing the http.Flusher interface.

Detailed Description:

pkg/srv/http/http.go: This file now includes a package-level comment describing its functionality and the InitHTTPServer function's comment has been improved for clarity.
pkg/srv/http/http_log.go: The response logging logic has been updated to prevent logging the response body for 204 No Content status codes, and the Flush method's comment has been made more precise.

**High-Level Summary:** This pull request primarily focuses on enhancing the clarity and maintainability of the HTTP server package. It introduces comprehensive package-level documentation and refines existing function comments for better understanding. A key improvement is the optimization of HTTP response logging, where response bodies are now explicitly excluded for `204 No Content` status codes, preventing unnecessary log clutter. Additionally, the comment for the `Flush` method has been clarified to accurately reflect its role in implementing the `http.Flusher` interface. **Detailed Description:** * **pkg/srv/http/http.go:** This file now includes a package-level comment describing its functionality and the `InitHTTPServer` function's comment has been improved for clarity. * **pkg/srv/http/http_log.go:** The response logging logic has been updated to prevent logging the response body for `204 No Content` status codes, and the `Flush` method's comment has been made more precise.

rmcguire commented

2025-08-24 15:47:33 +00:00

High-Level Summary: The changes primarily enhance documentation and refine the HTTP response logging mechanism, particularly improving the handling of 204 No Content responses and empty bodies. The overall direction is positive, contributing to better maintainability and clarity.

Code Issues:

File: pkg/srv/http/http_log.go
Line: 41-57
Issue: The current logic for logging response bodies can be improved for clarity and correctness. Specifically, treating io.EOF (indicating an empty body) as an "invalid response body" error can be misleading for non-204 status codes where an empty body might be valid. Additionally, the nested if statement for the http.StatusNoContent check adds an unnecessary level of indentation, slightly impacting readability.

Suggestion: Refactor the response body logging to use an early exit for http.StatusNoContent and differentiate between io.EOF and other errors when reading the response body. This improves logical precision and reduces nesting.

// Inside the handler function in http_log.go
		// Log response with body if not 204
		if lrr.statusCode == http.StatusNoContent {
			trcLog := log.Trace().
				Str("path", r.URL.Path).
				Int("statusCode", lrr.statusCode)
			trcLog.Msg("http response (no content)") // Explicitly log 204
			return // No body to log for 204 No Content
		}

		trcLog := log.Trace().
			Str("path", r.URL.Path).
			Int("statusCode", lrr.statusCode)

		firstByte, err := lrr.body.ReadByte()
		if err != nil {
			if err == io.EOF {
				// Body is empty, which might be valid for some non-204 responses.
				trcLog.Msg("http response (empty body)")
			} else {
				// Other error reading the body. Wrap the original error for context.
				trcLog.Err(fmt.Errorf("error reading response body: %w", err)).Send()
			}
			return // No further body processing if there was an error or it was empty
		}
		lrr.body.UnreadByte() // Put the byte back for Bytes() to read

		if firstByte == '{' {
			trcLog = trcLog.RawJSON("response", lrr.body.Bytes())
		} else {
			trcLog = trcLog.Bytes("response", lrr.body.Bytes())
		}
		trcLog.Msg("http response")

File: pkg/srv/http/http_log.go
Line: 56 (new)
Issue: The log message for the response body was changed from "response payload" to "http response". While "http response" is descriptive, the initial request log uses "http request served". Maintaining a consistent naming convention (e.g., "http response served" or "http response payload") could improve clarity and make it easier to correlate request and response logs when scanning.
Suggestion: Consider aligning the response log message with the request log message for better consistency, for example, changing "http response" to "http response served" or "http response payload".
File: pkg/srv/http/http_log.go
Line: 55 (new)
Issue: The lrr.body.Bytes() call creates a copy of the entire response body in memory. For applications handling potentially very large responses, logging the full body at Trace level could lead to significant memory consumption and performance overhead, especially if Trace level logging is inadvertently enabled in production environments.
Suggestion: Evaluate the maximum expected size of response bodies. If large bodies are common, consider implementing a mechanism to truncate the logged body to a reasonable maximum size (e.g., 1KB or 5KB) or only log headers for responses exceeding a certain size threshold. Ensure clear guidelines or configurations are in place to prevent Trace level logging from being enabled in production.

**High-Level Summary:** The changes primarily enhance documentation and refine the HTTP response logging mechanism, particularly improving the handling of `204 No Content` responses and empty bodies. The overall direction is positive, contributing to better maintainability and clarity. **Code Issues:** * **File:** pkg/srv/http/http_log.go * **Line:** 41-57 * **Issue:** The current logic for logging response bodies can be improved for clarity and correctness. Specifically, treating `io.EOF` (indicating an empty body) as an "invalid response body" error can be misleading for non-204 status codes where an empty body might be valid. Additionally, the nested `if` statement for the `http.StatusNoContent` check adds an unnecessary level of indentation, slightly impacting readability. * **Suggestion:** Refactor the response body logging to use an early exit for `http.StatusNoContent` and differentiate between `io.EOF` and other errors when reading the response body. This improves logical precision and reduces nesting. ```go // Inside the handler function in http_log.go // Log response with body if not 204 if lrr.statusCode == http.StatusNoContent { trcLog := log.Trace(). Str("path", r.URL.Path). Int("statusCode", lrr.statusCode) trcLog.Msg("http response (no content)") // Explicitly log 204 return // No body to log for 204 No Content } trcLog := log.Trace(). Str("path", r.URL.Path). Int("statusCode", lrr.statusCode) firstByte, err := lrr.body.ReadByte() if err != nil { if err == io.EOF { // Body is empty, which might be valid for some non-204 responses. trcLog.Msg("http response (empty body)") } else { // Other error reading the body. Wrap the original error for context. trcLog.Err(fmt.Errorf("error reading response body: %w", err)).Send() } return // No further body processing if there was an error or it was empty } lrr.body.UnreadByte() // Put the byte back for Bytes() to read if firstByte == '{' { trcLog = trcLog.RawJSON("response", lrr.body.Bytes()) } else { trcLog = trcLog.Bytes("response", lrr.body.Bytes()) } trcLog.Msg("http response") ``` * **File:** pkg/srv/http/http_log.go * **Line:** 56 (new) * **Issue:** The log message for the response body was changed from `"response payload"` to `"http response"`. While `"http response"` is descriptive, the initial request log uses `"http request served"`. Maintaining a consistent naming convention (e.g., "http response served" or "http response payload") could improve clarity and make it easier to correlate request and response logs when scanning. * **Suggestion:** Consider aligning the response log message with the request log message for better consistency, for example, changing `"http response"` to `"http response served"` or `"http response payload"`. * **File:** pkg/srv/http/http_log.go * **Line:** 55 (new) * **Issue:** The `lrr.body.Bytes()` call creates a copy of the entire response body in memory. For applications handling potentially very large responses, logging the full body at `Trace` level could lead to significant memory consumption and performance overhead, especially if `Trace` level logging is inadvertently enabled in production environments. * **Suggestion:** Evaluate the maximum expected size of response bodies. If large bodies are common, consider implementing a mechanism to truncate the logged body to a reasonable maximum size (e.g., 1KB or 5KB) or only log headers for responses exceeding a certain size threshold. Ensure clear guidelines or configurations are in place to prevent `Trace` level logging from being enabled in production.

rmcguire added 1 commit 2025-08-24 15:50:10 +00:00

refactor: refine HTTP response body logging, explicitly handling 204 No Content, and improving error management for body reads ae11d4aa01

rmcguire merged commit cb0c87d200 into main

2025-08-24 15:50:36 +00:00

rmcguire referenced this issue from a commit

2025-08-24 15:50:38 +00:00

refactors HTTP server initialization, improves logging middleware, and updates documentation (#1)

rmcguire deleted branch fix-http-log-middleware

2025-08-24 15:50:41 +00:00

Sign in to join this conversation.