Merged
Conversation
…ggers Adds `Always()` method to Verbose interface to force logging regardless of level, `WithValues()` for structured logging support, and fixes flag parsing to handle unknown short flags correctly without consuming subsequent arguments. Also adds "secret" to sensitive keys for sanitization.
Add optional verbose parameter to TraceToStdout and NewLogger to enable verbosity levels. Implement dual-mode logging: JSON-structured logs when enabled, with request/response body sanitization to strip sensitive data. Improve trace config parsing to distinguish between request and response traces.
Updated hx fixture test configuration to set correct working directory and build path. Upgraded gomplate and is-healthy dependencies. Commented out local replacement for commons.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
WalkthroughRefactors HTTP logging middleware to add verbosity-aware logging and JSON-structured output, updates trace parsing, adds body/header sanitization, extends the Verbose interface with Always/WithValues, changes flag parsing and tests, and updates Go module and fixture harness configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as Logger Middleware
participant Tracer as Tracer (json/pretty)
participant Sanitizer as Sanitizer
participant Backend as Logger Backend
Client->>Middleware: NewLogger(config, verbose...)
Middleware->>Tracer: newContextLogger(config, verbose)
alt JSON logs enabled
Tracer->>Tracer: jsonLogger path (structured fields)
else
Tracer->>Tracer: prettyLogger path (pretty format)
end
Client->>Middleware: Send HTTP request
Middleware->>Sanitizer: readBody() / sanitizeBody() / sanitize headers
Middleware->>Tracer: logAt/jsonLogAt(verbose, sanitized data)
Tracer->>Backend: emit log (redacted)
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 5
🧹 Nitpick comments (1)
go.mod (1)
192-192: Remove the commentedreplacedirective at line 192.The commented
replace github.com/flanksource/clicky => ../clickyis stale development code. Sincego.workis not used in this repository, this directive should be removed entirely rather than left as a confusing comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 192, Remove the stale commented replace directive "// replace github.com/flanksource/clicky => ../clicky" from go.mod; locate the exact commented line containing the replace directive and delete it so the go.mod no longer contains that leftover development comment (no other changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hx/fixtures/hx.md`:
- Around line 2-4: The build step in the fixture uses `cd $GIT_ROOT_DIR` then
runs `go build -o hx .`, which targets the repository root (no Go files) and
fails; update the build command so it builds the CLI package instead (either
change the build target to the package path for the CLI, e.g. ./cmd/hx, or `cd`
into cmd/hx before running `go build -o hx .`) — modify the line containing `go
build -o hx .` (refer to the build/exec block that references GIT_ROOT_DIR and
CWD) to point to the correct package.
In `@cmd/hx/go.mod`:
- Line 176: Uncomment the replace directive in cmd/hx/go.mod so the module uses
the local commons sources; specifically restore the line replacing
github.com/flanksource/commons with ../.. (undo the commented "// replace
github.com/flanksource/commons => ../..") so cmd/hx (which imports commons
subpackages in cmd/hx/root.go like output, parse, har, http, middlewares) will
build and test against the PR's local commons changes.
In `@http/middlewares/logger.go`:
- Around line 27-38: The jsonFormatter.Format currently unmarshals into
map[string]any which fails for JSON arrays; change the logic in
jsonFormatter.Format to unmarshal into any, switch on the resulting type, and
handle both map[string]any and []any cases: for objects call
logger.StripSecretsFromMap(m) as before, and for arrays either implement and
call a new logger.StripSecretsFromSlice([]any) that recursively strips secrets
from each element or inline iterate the []any and apply the same
secret-stripping logic to any nested maps/slices before marshalling; ensure the
final variable passed to json.MarshalIndent is the sanitized value and preserve
the existing api.CodeBlock formatting and error handling in Format.
- Around line 93-102: The readBody function currently returns an empty string on
io.ReadAll error but still replays the partial data, causing logs to not reflect
the actual request body; change readBody so that when io.ReadAll returns an
error it returns the partial data as the string (string(data)) and still returns
io.NopCloser(bytes.NewReader(data)) so the logged value and the replayed body
are consistent (update the error branch in readBody to return string(data),
io.NopCloser(bytes.NewReader(data)) and optionally preserve or surface the error
where appropriate).
In `@logger/buffered.go`:
- Around line 341-352: bufferedVerbose.WithValues currently returns the same
instance and drops structured key/value context; change it to capture and
propagate those pairs by adding a field on bufferedVerbose (e.g., values
[]interface{}) and have WithValues append/merge the incoming kv into that field
(returning a new bufferedVerbose preserving logger, enabled, filters and the
combined values) so subsequent methods like Infof/Info/Error that flush buffers
include those stored values; ensure merging logic respects existing filters and
mirror how slogVerbose.WithValues is used in http/middlewares/logger.go so
request/response metadata passed via WithValues(kv...) is not lost.
---
Nitpick comments:
In `@go.mod`:
- Line 192: Remove the stale commented replace directive "// replace
github.com/flanksource/clicky => ../clicky" from go.mod; locate the exact
commented line containing the replace directive and delete it so the go.mod no
longer contains that leftover development comment (no other changes required).
🪄 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: b2949a2c-f690-4fff-b2e3-bec250246bf5
⛔ Files ignored due to path filters (2)
cmd/hx/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/hx/fixtures/hx.mdcmd/hx/go.modgo.modhttp/client.gohttp/middlewares/logger.gohttp/middlewares/trace.gologger/buffered.gologger/default.gologger/log.gologger/logger_test.gologger/sanitize.gologger/slog.go
Comment on lines
+2
to
+4
| build: echo $GIT_ROOT_DIR==$CWD && pwd && cd $GIT_ROOT_DIR && pwd && go build -o hx . | ||
| cwd: cmd/hx | ||
| exec: $GIT_ROOT_DIR/hx --har "{{ .name | strings.Slug }}.har" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where the CLI main package lives and whether root package is main.
# Expected: main package under cmd/hx; if root is not main, current build command is incorrect.
echo "== package declarations (root and cmd/hx) =="
rg -n '^\s*package\s+main\b|^\s*package\s+' *.go cmd/hx/*.go cmd/hx/**/*.go
echo
echo "== current fixture build/exec lines =="
rg -n '^(build|cwd|exec):' cmd/hx/fixtures/hx.mdRepository: flanksource/commons
Length of output: 686
🏁 Script executed:
#!/bin/bash
# Check if root has any Go files
echo "== Go files at repo root =="
ls -la *.go 2>/dev/null || echo "(No Go files at root)"
echo
echo "== Directory structure around cmd/hx =="
tree -L 2 -I 'node_modules|.git|vendor' cmd/hx/ 2>/dev/null || find cmd/hx -maxdepth 2 -type f -name '*.go' | head -20Repository: flanksource/commons
Length of output: 417
Build target is incorrect and will fail to produce the CLI binary.
After cd $GIT_ROOT_DIR, the command go build -o hx . attempts to build the root directory, which contains no Go files. The CLI entrypoint is in cmd/hx/ (main.go and root.go are both package main). This build command must target the correct package.
Fix
-build: echo $GIT_ROOT_DIR==$CWD && pwd && cd $GIT_ROOT_DIR && pwd && go build -o hx .
+build: cd "$GIT_ROOT_DIR" && go build -o hx ./cmd/hx
cwd: cmd/hx
exec: $GIT_ROOT_DIR/hx --har "{{ .name | strings.Slug }}.har"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hx/fixtures/hx.md` around lines 2 - 4, The build step in the fixture uses
`cd $GIT_ROOT_DIR` then runs `go build -o hx .`, which targets the repository
root (no Go files) and fails; update the build command so it builds the CLI
package instead (either change the build target to the package path for the CLI,
e.g. ./cmd/hx, or `cd` into cmd/hx before running `go build -o hx .`) — modify
the line containing `go build -o hx .` (refer to the build/exec block that
references GIT_ROOT_DIR and CWD) to point to the correct package.
| ) | ||
|
|
||
| replace github.com/flanksource/commons => ../.. | ||
| // replace github.com/flanksource/commons => ../.. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that cmd/hx imports commons packages while local replace is disabled.
# Expected: imports exist + replace is commented, confirming potential mismatch risk.
echo "== Imports from github.com/flanksource/commons in cmd/hx =="
rg -n 'github\.com/flanksource/commons(/|")' cmd/hx/*.go cmd/hx/**/*.go
echo
echo "== commons dependency and replace directive state in cmd/hx/go.mod =="
rg -n 'github\.com/flanksource/commons v|^\s*replace github\.com/flanksource/commons|^\s*//\s*replace github\.com/flanksource/commons' cmd/hx/go.modRepository: flanksource/commons
Length of output: 615
Re-enable the commons replace directive in cmd/hx/go.mod.
The replace at line 176 is commented out, causing cmd/hx to resolve github.com/flanksource/commons from the published v1.47.2 (line 8) instead of local sources. However, cmd/hx/root.go imports multiple commons subpackages (output, parse, har, http, middlewares), so disabling the replace means any changes to commons in this PR won't be tested by cmd/hx builds or tests. Uncomment the replace directive to ensure cmd/hx uses the PR's local commons code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hx/go.mod` at line 176, Uncomment the replace directive in cmd/hx/go.mod
so the module uses the local commons sources; specifically restore the line
replacing github.com/flanksource/commons with ../.. (undo the commented "//
replace github.com/flanksource/commons => ../..") so cmd/hx (which imports
commons subpackages in cmd/hx/root.go like output, parse, har, http,
middlewares) will build and test against the PR's local commons changes.
Comment on lines
27
to
38
| 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 | ||
| } | ||
| var m map[string]any | ||
| if err := json.Unmarshal(src, &m); err != nil { | ||
| return err | ||
| } | ||
| var indented bytes.Buffer | ||
| if err := json.Indent(&indented, src, "", " "); err != nil { | ||
| sanitized := logger.StripSecretsFromMap(m) | ||
| b, err := json.MarshalIndent(sanitized, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprint(w, api.CodeBlock("json", indented.String()).ANSI()) | ||
| fmt.Fprint(w, api.CodeBlock("json", string(b)).ANSI()) | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if StripSecretsFromSlice exists or needs to be added
echo "=== Checking for StripSecretsFromSlice in logger package ==="
rg -n "StripSecretsFromSlice" logger/Repository: flanksource/commons
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Check for StripSecretsFromMap and understand the logger package structure
echo "=== Checking for StripSecretsFromMap ==="
rg -n "StripSecretsFromMap" logger/ -A 5
echo -e "\n=== Full logger package file structure ==="
fd -e go logger/ | head -20
echo -e "\n=== Look for logger.go or similar main files ==="
find logger -name "*.go" -type fRepository: flanksource/commons
Length of output: 2225
🏁 Script executed:
#!/bin/bash
# Check the actual jsonFormatter.Format function context and surrounding code
echo "=== Full context of jsonFormatter in http/middlewares/logger.go ==="
cat -n http/middlewares/logger.go | head -50
echo -e "\n=== Check if there's any array handling elsewhere in the codebase ==="
rg -n "StripSecrets" --type go -A 2 -B 2Repository: flanksource/commons
Length of output: 6201
🏁 Script executed:
#!/bin/bash
# Verify if JSON arrays are actually processed by this middleware
echo "=== Check how this middleware is used ==="
rg -n "jsonFormatter\|Format.*json" http/ --type go -B 3 -A 3
echo -e "\n=== Look for test files that might show usage patterns ==="
fd -e test.go http/middlewares/Repository: flanksource/commons
Length of output: 555
🏁 Script executed:
#!/bin/bash
# Check how jsonFormatter is used
echo "=== Search for jsonFormatter usage ==="
rg -n "jsonFormatter" --type go
echo -e "\n=== Check logger/sanitize.go fully for array handling ==="
cat -n logger/sanitize.goRepository: flanksource/commons
Length of output: 5743
🏁 Script executed:
#!/bin/bash
# Check for any tests of jsonFormatter that might show expected input
echo "=== Look for middleware tests ==="
find . -path "./http/middlewares/*test*.go" -type f
# Check if there are any integration tests showing array responses
echo -e "\n=== Search for response formatting with arrays ==="
rg -n "\\[.*\\]" http/middlewares/logger.go -A 2 -B 2 || echo "No array patterns found"
# Check the full http/middlewares/logger.go file
echo -e "\n=== Full http/middlewares/logger.go file ==="
wc -l http/middlewares/logger.goRepository: flanksource/commons
Length of output: 2696
JSON arrays in responses are not handled and will cause formatting to fail.
Unmarshaling into map[string]any will fail for JSON array payloads (e.g., [{"id":1}]), which are valid JSON responses. The jsonFormatter.Format method needs to handle both objects and arrays. Note that logger.StripSecretsFromSlice() does not currently exist in the logger package, so a complete solution would either need to:
- Implement
StripSecretsFromSlice()in the logger package to handle array elements recursively, or - Unmarshal to
anyand switch on the type, handling arrays with appropriate secret stripping logic
🤖 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 - 38, The jsonFormatter.Format
currently unmarshals into map[string]any which fails for JSON arrays; change the
logic in jsonFormatter.Format to unmarshal into any, switch on the resulting
type, and handle both map[string]any and []any cases: for objects call
logger.StripSecretsFromMap(m) as before, and for arrays either implement and
call a new logger.StripSecretsFromSlice([]any) that recursively strips secrets
from each element or inline iterate the []any and apply the same
secret-stripping logic to any nested maps/slices before marshalling; ensure the
final variable passed to json.MarshalIndent is the sanitized value and preserve
the existing api.CodeBlock formatting and error handling in Format.
Comment on lines
+93
to
+102
| func readBody(body io.ReadCloser) (string, io.ReadCloser) { | ||
| if body == nil { | ||
| return "", nil | ||
| } | ||
| data, err := io.ReadAll(body) | ||
| if err != nil { | ||
| return "", io.NopCloser(bytes.NewReader(data)) | ||
| } | ||
| return string(data), io.NopCloser(bytes.NewReader(data)) | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: returns empty string but replays partial data.
When io.ReadAll fails, the function returns an empty string for logging but still creates a reader from the partial data. This means the request body is replayed with partial content while the log shows nothing was read.
🛡️ Proposed fix to return consistent data
func readBody(body io.ReadCloser) (string, io.ReadCloser) {
if body == nil {
return "", nil
}
data, err := io.ReadAll(body)
if err != nil {
- return "", io.NopCloser(bytes.NewReader(data))
+ // Return whatever was read (may be partial) for both logging and replay
+ return string(data), io.NopCloser(bytes.NewReader(data))
}
return string(data), io.NopCloser(bytes.NewReader(data))
}📝 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
| func readBody(body io.ReadCloser) (string, io.ReadCloser) { | |
| if body == nil { | |
| return "", nil | |
| } | |
| data, err := io.ReadAll(body) | |
| if err != nil { | |
| return "", io.NopCloser(bytes.NewReader(data)) | |
| } | |
| return string(data), io.NopCloser(bytes.NewReader(data)) | |
| } | |
| func readBody(body io.ReadCloser) (string, io.ReadCloser) { | |
| if body == nil { | |
| return "", nil | |
| } | |
| data, err := io.ReadAll(body) | |
| if err != nil { | |
| // Return whatever was read (may be partial) for both logging and replay | |
| return string(data), io.NopCloser(bytes.NewReader(data)) | |
| } | |
| return string(data), io.NopCloser(bytes.NewReader(data)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@http/middlewares/logger.go` around lines 93 - 102, The readBody function
currently returns an empty string on io.ReadAll error but still replays the
partial data, causing logs to not reflect the actual request body; change
readBody so that when io.ReadAll returns an error it returns the partial data as
the string (string(data)) and still returns io.NopCloser(bytes.NewReader(data))
so the logged value and the replayed body are consistent (update the error
branch in readBody to return string(data), io.NopCloser(bytes.NewReader(data))
and optionally preserve or surface the error where appropriate).
Comment on lines
+341
to
352
| func (v *bufferedVerbose) Always() Verbose { | ||
| return &bufferedVerbose{ | ||
| logger: v.logger, | ||
| enabled: v.enabled, | ||
| filters: v.filters, | ||
| always: true, | ||
| } | ||
| } | ||
|
|
||
| func (v *bufferedVerbose) WithValues(_ ...interface{}) Verbose { | ||
| return v | ||
| } |
There was a problem hiding this comment.
WithValues silently discards structured logging context.
The bufferedVerbose.WithValues is a no-op that returns the same instance without preserving the key-value pairs. This creates an inconsistency with slogVerbose.WithValues, which properly propagates structured context.
In http/middlewares/logger.go:143-149, WithValues(kv...) is called to attach request/response metadata. When using BufferedLogger, this metadata is silently discarded, meaning HTTP trace logs will lack the expected structured context.
Consider either:
- Documenting this limitation clearly
- Storing the keysAndValues and including them in log output
📝 Suggested implementation to preserve values
type bufferedVerbose struct {
logger *BufferedLogger
enabled bool
filters []string
always bool
+ values []interface{}
}
func (v *bufferedVerbose) WithValues(keysAndValues ...interface{}) Verbose {
- return v
+ newValues := make([]interface{}, len(v.values)+len(keysAndValues))
+ copy(newValues, v.values)
+ copy(newValues[len(v.values):], keysAndValues)
+ return &bufferedVerbose{
+ logger: v.logger,
+ enabled: v.enabled,
+ filters: v.filters,
+ always: v.always,
+ values: newValues,
+ }
}Then format values in Infof:
func (v *bufferedVerbose) Infof(format string, args ...interface{}) {
if !v.always && !v.enabled {
return
}
message := fmt.Sprintf(format, args...)
+ if len(v.values) > 0 {
+ message = fmt.Sprintf("%s %v", message, v.values)
+ }
if !v.isFiltered(message) {
v.logger.Infof("%s", message)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@logger/buffered.go` around lines 341 - 352, bufferedVerbose.WithValues
currently returns the same instance and drops structured key/value context;
change it to capture and propagate those pairs by adding a field on
bufferedVerbose (e.g., values []interface{}) and have WithValues append/merge
the incoming kv into that field (returning a new bufferedVerbose preserving
logger, enabled, filters and the combined values) so subsequent methods like
Infof/Info/Error that flush buffers include those stored values; ensure merging
logic respects existing filters and mirror how slogVerbose.WithValues is used in
http/middlewares/logger.go so request/response metadata passed via
WithValues(kv...) is not lost.
moshloop
force-pushed
the
pr/http-logging
branch
from
088e380 to
1fa1831
Compare
|
🎉 This PR is included in version 1.50.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores