feat(http): enhance http logging with context-aware access logs and formatters#304
Merged
feat(http): enhance http logging with context-aware access logs and formatters#304
Conversation
…ormatters Adds context-aware HTTP logging via request context, new access log middleware with single-line output, form URL encoded formatter for request bodies, NO_COLOR environment variable support, and improved request/response logging configuration through TraceConfigFromString for flexible trace levels.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChanges across console output, context logger propagation, HTTP client OAuth transport, HTTP request/response logging, and logger formatting. Introduces NO_COLOR environment variable support, context-aware logger selection, improved OAuth token transport wrapping, refactored HTTP logging architecture with access log mode, and enhanced HTTP body formatters. Changes
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
http/middlewares/logger.go (1)
27-39: Redundant JSON validation logic.The check on lines 28-31 is logically questionable: if
json.Valid(src)returnsfalse,json.Unmarshalwill also fail since both use the same underlying validation. The unmarshal attempt appears to serve no purpose.♻️ Proposed simplification
func (j *jsonFormatter) Format(w io.Writer, src []byte) error { - if !json.Valid(src) { - if err := json.Unmarshal(src, &json.RawMessage{}); err != nil { - return err - } + if !json.Valid(src) { + return fmt.Errorf("invalid JSON") } var indented bytes.Buffer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@http/middlewares/logger.go` around lines 27 - 39, The pre-validation using json.Valid and the subsequent json.Unmarshal in jsonFormatter.Format is redundant and should be removed; instead, let json.Indent handle invalid JSON and return its error directly. Edit the jsonFormatter.Format function to remove the if !json.Valid(...) { ... } block (the json.Unmarshal call) so the function simply attempts json.Indent into the indented buffer and returns any error from json.Indent, then writes the formatted output with api.CodeBlock(...).ANSI().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/console.go`:
- Around line 54-75: The NO_COLOR check is only applied to console.isTTY via the
package-level isTTY variable, but logger/slog.go uses its own detection (isTTY,
flags.color, tint.Options.NoColor), so NO_COLOR is not globally honored; fix by
centralizing the no-color decision: expose and use console.IsNoColor (or provide
a shared function like console.NoColorEnabled) from logger/slog.go and ensure
logger's color logic consults that before applying colors (i.e., short-circuit
flags.color/tint.Options.NoColor when IsNoColor() returns true), or move the
isTTY/no-color detection into a shared utility used by both console.isTTY and
logger/slog.go (referencing IsNoColor, isTTY, flags.color, and
tint.Options.NoColor to locate the related logic).
In `@http/middlewares/accesslog.go`:
- Around line 3-20: Replace direct fmt.Printf usage in NewAccessLog /
RoundTripperFunc with the request-scoped logger obtained from req.Context()
(e.g., logger := logger.FromContext(req.Context()) or the project's equivalent)
and emit access entries via that logger at the appropriate level; also sanitize
the URI before logging by removing userinfo and sensitive query parameters (or
use a redacted URL representation) instead of logging req.URL verbatim, and
include elapsed time and resp.StatusCode or error in the structured log entry.
In `@http/middlewares/logger.go`:
- Around line 134-137: The log statements in logger.go use
console.Bluef(req.Method) which treats req.Method as a format string and can
panic if it contains '%' — change both calls to use a constant format string
like "%s" and pass req.Method as an argument (e.g., console.Bluef("%s",
req.Method)), doing the same for console.Yellowf("%s", req.URL) and the
statusColor(resp.StatusCode) call so all formatting uses constant format strings
(keep the surrounding log.Infof calls intact).
- Around line 81-113: The shared bytes.Buffer `buf` created once for the
middleware causes a data race across concurrent RoundTrip calls; fix by
allocating a new buffer per request and wiring it into the logger before
invoking the inner RoundTripper — e.g., inside the RoundTripperFunc create a
local `bytes.Buffer`, call `l.SetOutput(&buf)` on a per-request copy of the
logger if necessary (or otherwise obtain a fresh logger instance), then use that
local msg for the timing/suffix logic and call `getLogger(req).Infof(...)`;
alternatively, if reusing buffers is desired for performance, use a `sync.Pool`
to borrow/return buffers per request instead of sharing a single `buf`.
In `@http/middlewares/trace.go`:
- Around line 88-142: TraceConfigFromString currently returns early for exact
string cases (e.g., "access", "debug"/"headers", "body",
"trace"/"all"/"response") which skips the property-based overrides; refactor so
the function first selects a base TraceConfig (use traceHeaders, traceAll, or
TraceConfig literals depending on the switch) but does not return immediately,
then apply the property checks using properties.On(false, "http.body.disabled")
and properties.On(false, "http.headers.disabled") to mutate the chosen config
(clearing Body/Response and Headers/ResponseHeaders as appropriate), and finally
return the modified config; keep references to traceHeaders, traceAll and
properties.On in your changes and ensure TraceConfigFromString always applies
those global overrides.
In `@logger/httpretty/printer.go`:
- Around line 619-629: The code currently falls back to a hardcoded "https" when
req.URL.Scheme is empty, which can mislabel HTTP requests; change the fallback
to inspect req.TLS and set scheme = "https" if req.TLS != nil, otherwise scheme
= "http" (keeping the existing logic that uses req.Host / req.Header.Get("Host")
and builds uri via fmt.Sprintf). Update the block that assigns uri (referencing
uri, req.URL.Scheme, req.TLS, req.Host) so the scheme is determined from req.TLS
instead of always using "https".
---
Nitpick comments:
In `@http/middlewares/logger.go`:
- Around line 27-39: The pre-validation using json.Valid and the subsequent
json.Unmarshal in jsonFormatter.Format is redundant and should be removed;
instead, let json.Indent handle invalid JSON and return its error directly. Edit
the jsonFormatter.Format function to remove the if !json.Valid(...) { ... }
block (the json.Unmarshal call) so the function simply attempts json.Indent into
the indented buffer and returns any error from json.Indent, then writes the
formatted output with api.CodeBlock(...).ANSI().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10eb9467-2256-44a3-a3e0-e38657bf8b9e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
console/console.gocontext/context.gohttp/client.gohttp/middlewares/accesslog.gohttp/middlewares/logger.gohttp/middlewares/trace.gologger/default.gologger/httpretty/httpretty.gologger/httpretty/printer.gologger/slog.go
Comment on lines
+54
to
+75
| isTTY = is.TTY() && !IsNoColor() | ||
| ) | ||
|
|
||
| func IsNoColor() bool { | ||
| if os.Getenv("NO_COLOR") != "" { | ||
| return true | ||
| } | ||
| if v := strings.ToLower(os.Getenv("COLOR")); v == "no" || v == "false" { | ||
| return true | ||
| } | ||
| if os.Getenv("TERM") == "dumb" { | ||
| return true | ||
| } | ||
|
|
||
| // also scan os.Args for --no-color in case env vars are not set but the flag is used (e.g., in tests) | ||
| for _, arg := range os.Args[1:] { | ||
| if arg != "--no-color=false" && (arg == "--no-color" || arg == "-no-color") { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
NO_COLOR only disables the console helpers right now.
Line 54 gates console.isTTY, but the main slog path still decides color independently in logger/slog.go via its own isTTY, flags.color, and tint.Options.NoColor. That means normal logger output can still be colored even when NO_COLOR is set, so the feature is only partially implemented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/console.go` around lines 54 - 75, The NO_COLOR check is only applied
to console.isTTY via the package-level isTTY variable, but logger/slog.go uses
its own detection (isTTY, flags.color, tint.Options.NoColor), so NO_COLOR is not
globally honored; fix by centralizing the no-color decision: expose and use
console.IsNoColor (or provide a shared function like console.NoColorEnabled)
from logger/slog.go and ensure logger's color logic consults that before
applying colors (i.e., short-circuit flags.color/tint.Options.NoColor when
IsNoColor() returns true), or move the isTTY/no-color detection into a shared
utility used by both console.isTTY and logger/slog.go (referencing IsNoColor,
isTTY, flags.color, and tint.Options.NoColor to locate the related logic).
http/middlewares/accesslog.go
Outdated
Comment on lines
+3
to
+20
| import ( | ||
| "fmt" | ||
| "net/http" | ||
| "time" | ||
| ) | ||
|
|
||
| func NewAccessLog() Middleware { | ||
| return func(rt http.RoundTripper) http.RoundTripper { | ||
| return RoundTripperFunc(func(req *http.Request) (*http.Response, error) { | ||
| start := time.Now() | ||
| resp, err := rt.RoundTrip(req) | ||
| elapsed := time.Since(start) | ||
| if err != nil { | ||
| fmt.Printf("%s %s error %s\n", req.Method, req.URL, elapsed.Truncate(time.Millisecond)) | ||
| return nil, err | ||
| } | ||
| fmt.Printf("%s %s %d %s\n", req.Method, req.URL, resp.StatusCode, elapsed.Truncate(time.Millisecond)) | ||
| return resp, nil |
There was a problem hiding this comment.
Use the request-scoped logger for access logs.
Lines 16-19 write straight to stdout, so these entries bypass logger level/JSON/color settings and any logger attached to req.Context(). They also log the URL verbatim, which can leak userinfo or query-token data. Please emit this through the same logger path used elsewhere and sanitize the URI before logging.
Proposed direction
import (
- "fmt"
"net/http"
"time"
+
+ commonsctx "github.com/flanksource/commons/context"
)
func NewAccessLog() Middleware {
return func(rt http.RoundTripper) http.RoundTripper {
return RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
start := time.Now()
resp, err := rt.RoundTrip(req)
- elapsed := time.Since(start)
+ elapsed := time.Since(start).Truncate(time.Millisecond)
+ log := commonsctx.LoggerFromContext(req.Context())
+ uri := req.URL.Redacted()
if err != nil {
- fmt.Printf("%s %s error %s\n", req.Method, req.URL, elapsed.Truncate(time.Millisecond))
+ log.Infof("%s %s error %s err=%v", req.Method, uri, elapsed, err)
return nil, err
}
- fmt.Printf("%s %s %d %s\n", req.Method, req.URL, resp.StatusCode, elapsed.Truncate(time.Millisecond))
+ log.Infof("%s %s %d %s", req.Method, uri, resp.StatusCode, elapsed)
return resp, nil
})
}
}As per coding guidelines, "Use the logger package for all logging needs; avoid fmt.Print statements."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import ( | |
| "fmt" | |
| "net/http" | |
| "time" | |
| ) | |
| func NewAccessLog() Middleware { | |
| return func(rt http.RoundTripper) http.RoundTripper { | |
| return RoundTripperFunc(func(req *http.Request) (*http.Response, error) { | |
| start := time.Now() | |
| resp, err := rt.RoundTrip(req) | |
| elapsed := time.Since(start) | |
| if err != nil { | |
| fmt.Printf("%s %s error %s\n", req.Method, req.URL, elapsed.Truncate(time.Millisecond)) | |
| return nil, err | |
| } | |
| fmt.Printf("%s %s %d %s\n", req.Method, req.URL, resp.StatusCode, elapsed.Truncate(time.Millisecond)) | |
| return resp, nil | |
| import ( | |
| "net/http" | |
| "time" | |
| commonsctx "github.com/flanksource/commons/context" | |
| ) | |
| func NewAccessLog() Middleware { | |
| return func(rt http.RoundTripper) http.RoundTripper { | |
| return RoundTripperFunc(func(req *http.Request) (*http.Response, error) { | |
| start := time.Now() | |
| resp, err := rt.RoundTrip(req) | |
| elapsed := time.Since(start).Truncate(time.Millisecond) | |
| log := commonsctx.LoggerFromContext(req.Context()) | |
| uri := req.URL.Redacted() | |
| if err != nil { | |
| log.Infof("%s %s error %s err=%v", req.Method, uri, elapsed, err) | |
| return nil, err | |
| } | |
| log.Infof("%s %s %d %s", req.Method, uri, resp.StatusCode, elapsed) | |
| return resp, nil | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@http/middlewares/accesslog.go` around lines 3 - 20, Replace direct fmt.Printf
usage in NewAccessLog / RoundTripperFunc with the request-scoped logger obtained
from req.Context() (e.g., logger := logger.FromContext(req.Context()) or the
project's equivalent) and emit access entries via that logger at the appropriate
level; also sanitize the URI before logging by removing userinfo and sensitive
query parameters (or use a redacted URL representation) instead of logging
req.URL verbatim, and include elapsed time and resp.StatusCode or error in the
structured log entry.
Remove standalone access log middleware and consolidate into context logger. Move logger initialization inside request handler to avoid state reuse. Simplify trace config parsing with early returns. Detect request scheme dynamically instead of defaulting to https.
|
🎉 This PR is included in version 1.49.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…
Adds context-aware HTTP logging via request context, new access log middleware with single-line output, form URL encoded formatter for request bodies, NO_COLOR environment variable support, and improved request/response logging configuration through TraceConfigFromString for flexible trace levels.
Summary by CodeRabbit
New Features
Improvements