[RAP-435] feat: add structured JSON logger middleware#1
Open
ajit216 wants to merge 1 commit into
Open
Conversation
Introduces JSONLogger() and JSONLoggerWithConfig() as production-ready companions to the built-in Logger() middleware. Each request emits a single JSON line with timestamp, status, latency_ms, method, path, client_ip, body_bytes and errors — suitable for Datadog, ELK and CloudWatch ingestion. Supports SkipPaths, custom Skip func, and MaskHeaders for redacting sensitive header values from error output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA new Gin middleware for structured JSON request logging is introduced, capturing request details including timestamp, HTTP status, latency, method, path, client IP, response size, and errors. Configuration options allow customizing output destination, skipping specific paths or conditionally, and masking sensitive header values. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
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: 2
🧹 Nitpick comments (1)
jsonlogger_test.go (1)
130-149: Concurrent test should assert output integrity, not just completion.Right now Line 130-Line 149 verifies goroutines finished but does not verify that 50 valid JSON lines were actually written.
Suggested enhancement
import ( "bytes" "encoding/json" "fmt" "net/http" "net/http/httptest" + "strings" "sync" "testing" @@ func TestJSONLogger_ConcurrentRequests(t *testing.T) { @@ for i := 0; i < 50; i++ { <-done } + + lines := strings.Split(strings.TrimSpace(buf.String()), "\n") + require.Len(t, lines, 50) + for _, line := range lines { + var entry JSONLogEntry + require.NoError(t, json.Unmarshal([]byte(line), &entry)) + assert.Equal(t, "/concurrent", entry.Path) + assert.Equal(t, http.StatusOK, entry.Status) + } } @@ func (s *safeBuffer) Write(p []byte) (n int, err error) { s.mu.Lock() defer s.mu.Unlock() return s.buf.Write(p) } + +func (s *safeBuffer) String() string { + s.mu.Lock() + defer s.mu.Unlock() + return s.buf.String() +}Also applies to: 151-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jsonlogger_test.go` around lines 130 - 149, Update TestJSONLogger_ConcurrentRequests to validate output integrity: after the goroutines finish, read buf.String(), split into lines, assert there are 50 non-empty lines and that each line is valid JSON (e.g., by unmarshaling into map[string]interface{}), and optionally assert each JSON contains expected keys (status, method, path). Locate the safeBuffer used as Output via JSONLoggerWithConfig(JSONLoggerConfig{Output: buf}) and add the parsing/assertions after the done channel loop; apply the same pattern to the other concurrent test block (lines 151-161) that uses a safeBuffer to ensure it verifies 50 valid JSON log entries rather than only goroutine completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jsonlogger.go`:
- Around line 27-44: The middleware currently always appends
request.URL.RawQuery into the logged "Path", risking sensitive query data; add a
config flag on JSONLoggerConfig (e.g., LogQueryString bool or IncludeQueryString
bool, default false for safety) and update the code that builds the "Path" (the
place that currently reads request.URL.RawQuery into Path) to only append
RawQuery when JSONLoggerConfig.LogQueryString is true; keep existing
Skip/SkipPaths/MaskHeaders behavior unchanged and ensure the new flag is checked
before any masking or logging of the query string.
- Around line 37-39: The comment and implementation disagree about which skip
rule wins; update the logging decision so the Skip Skipper is evaluated before
SkipPaths (i.e., call Skip(...) first and if it returns true skip logging
regardless of SkipPaths), and update the declaration comment on Skip to state
that Skip takes precedence; specifically change the conditional in the
log/handler decision logic that currently checks SkipPaths first to check Skip
first and keep SkipPaths as a fallback.
---
Nitpick comments:
In `@jsonlogger_test.go`:
- Around line 130-149: Update TestJSONLogger_ConcurrentRequests to validate
output integrity: after the goroutines finish, read buf.String(), split into
lines, assert there are 50 non-empty lines and that each line is valid JSON
(e.g., by unmarshaling into map[string]interface{}), and optionally assert each
JSON contains expected keys (status, method, path). Locate the safeBuffer used
as Output via JSONLoggerWithConfig(JSONLoggerConfig{Output: buf}) and add the
parsing/assertions after the done channel loop; apply the same pattern to the
other concurrent test block (lines 151-161) that uses a safeBuffer to ensure it
verifies 50 valid JSON log entries rather than only goroutine completion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 572bc824-3e8d-41a3-98a6-a2b94d0a7ca9
📒 Files selected for processing (2)
jsonlogger.gojsonlogger_test.go
Comment on lines
+27
to
+44
| // JSONLoggerConfig defines configuration for the JSONLogger middleware. | ||
| type JSONLoggerConfig struct { | ||
| // Output is the writer where JSON log lines are written. | ||
| // Defaults to gin.DefaultWriter (os.Stdout). | ||
| Output io.Writer | ||
|
|
||
| // SkipPaths is a list of URL paths that will not be logged. | ||
| // Useful for suppressing health-check or metrics endpoints. | ||
| SkipPaths []string | ||
|
|
||
| // Skip is an optional function that returns true when a request | ||
| // should not be logged. Takes precedence over SkipPaths. | ||
| Skip Skipper | ||
|
|
||
| // MaskHeaders is a list of header names whose values will be | ||
| // redacted to "***" in error output to prevent leaking credentials. | ||
| MaskHeaders []string | ||
| } |
There was a problem hiding this comment.
Add a query-string opt-out to prevent sensitive data leakage in logs.
Line 75-Line 78 always logs RawQuery into Path. That can expose credentials/tokens passed via query parameters in production logs.
Suggested fix
type JSONLoggerConfig struct {
@@
// Skip is an optional function that returns true when a request
// should not be logged. Takes precedence over SkipPaths.
Skip Skipper
+
+ // SkipQueryString indicates that query strings should not be included
+ // in logged paths (useful when secrets are passed via query params).
+ // Optional. Default is false.
+ SkipQueryString bool
@@
return func(c *Context) {
start := time.Now()
path := c.Request.URL.Path
- if raw := c.Request.URL.RawQuery; raw != "" {
- path = path + "?" + raw
+ if !cfg.SkipQueryString {
+ if raw := c.Request.URL.RawQuery; raw != "" {
+ path = path + "?" + raw
+ }
}Also applies to: 75-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jsonlogger.go` around lines 27 - 44, The middleware currently always appends
request.URL.RawQuery into the logged "Path", risking sensitive query data; add a
config flag on JSONLoggerConfig (e.g., LogQueryString bool or IncludeQueryString
bool, default false for safety) and update the code that builds the "Path" (the
place that currently reads request.URL.RawQuery into Path) to only append
RawQuery when JSONLoggerConfig.LogQueryString is true; keep existing
Skip/SkipPaths/MaskHeaders behavior unchanged and ensure the new flag is checked
before any masking or logging of the query string.
Comment on lines
+37
to
+39
| // Skip is an optional function that returns true when a request | ||
| // should not be logged. Takes precedence over SkipPaths. | ||
| Skip Skipper |
There was a problem hiding this comment.
Config docs and implementation order are inconsistent for skip behavior.
Line 37-Line 39 says Skip takes precedence, but Line 82-Line 86 checks SkipPaths first. Please align comment wording with actual behavior to avoid misuse.
Suggested wording update
- // Skip is an optional function that returns true when a request
- // should not be logged. Takes precedence over SkipPaths.
+ // Skip is an optional function that returns true when a request
+ // should not be logged. Evaluated in addition to SkipPaths.
Skip SkipperAlso applies to: 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jsonlogger.go` around lines 37 - 39, The comment and implementation disagree
about which skip rule wins; update the logging decision so the Skip Skipper is
evaluated before SkipPaths (i.e., call Skip(...) first and if it returns true
skip logging regardless of SkipPaths), and update the declaration comment on
Skip to state that Skip takes precedence; specifically change the conditional in
the log/handler decision logic that currently checks SkipPaths first to check
Skip first and keep SkipPaths as a fallback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="jsonlogger.go">
<violation number="1" location="jsonlogger.go:39">
P2: The doc comment says `Skip` "takes precedence over `SkipPaths`", but the implementation checks `SkipPaths` first (line 82-84) and returns before `Skip` is ever evaluated. This means `Skip` **cannot** override `SkipPaths` — the opposite of the documented behavior. Either reorder the checks so `Skip` is evaluated first, or update the comment to say they are evaluated independently (both cause skipping).</violation>
<violation number="2" location="jsonlogger.go:98">
P2: `c.Writer.Size()` returns -1 when no body has been written, producing `"body_bytes": -1` in the JSON output. For machine-parsed structured logs, clamp to 0.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| Method: c.Request.Method, | ||
| Path: path, | ||
| ClientIP: c.ClientIP(), | ||
| BodyBytes: c.Writer.Size(), |
There was a problem hiding this comment.
P2: c.Writer.Size() returns -1 when no body has been written, producing "body_bytes": -1 in the JSON output. For machine-parsed structured logs, clamp to 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At jsonlogger.go, line 98:
<comment>`c.Writer.Size()` returns -1 when no body has been written, producing `"body_bytes": -1` in the JSON output. For machine-parsed structured logs, clamp to 0.</comment>
<file context>
@@ -0,0 +1,127 @@
+ Method: c.Request.Method,
+ Path: path,
+ ClientIP: c.ClientIP(),
+ BodyBytes: c.Writer.Size(),
+ Errors: errMsg,
+ }
</file context>
|
|
||
| // Skip is an optional function that returns true when a request | ||
| // should not be logged. Takes precedence over SkipPaths. | ||
| Skip Skipper |
There was a problem hiding this comment.
P2: The doc comment says Skip "takes precedence over SkipPaths", but the implementation checks SkipPaths first (line 82-84) and returns before Skip is ever evaluated. This means Skip cannot override SkipPaths — the opposite of the documented behavior. Either reorder the checks so Skip is evaluated first, or update the comment to say they are evaluated independently (both cause skipping).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At jsonlogger.go, line 39:
<comment>The doc comment says `Skip` "takes precedence over `SkipPaths`", but the implementation checks `SkipPaths` first (line 82-84) and returns before `Skip` is ever evaluated. This means `Skip` **cannot** override `SkipPaths` — the opposite of the documented behavior. Either reorder the checks so `Skip` is evaluated first, or update the comment to say they are evaluated independently (both cause skipping).</comment>
<file context>
@@ -0,0 +1,127 @@
+
+ // Skip is an optional function that returns true when a request
+ // should not be logged. Takes precedence over SkipPaths.
+ Skip Skipper
+
+ // MaskHeaders is a list of header names whose values will be
</file context>
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
2 participants
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
JSONLogger()andJSONLoggerWithConfig()as production-ready companions to the built-inLogger()middlewaretimestamp,status,latency_ms,method,path,client_ip,body_bytes,errorsSkipPathsto suppress noisy health/metrics endpointsSkip func(*Context) boolfor dynamic skip logicMaskHeadersto redact sensitive header values from error outputMotivation
Gin's built-in
Logger()outputs colored text — unsuitable for production log aggregators (Datadog, ELK, CloudWatch). Teams currently roll their own JSON loggers inconsistently. This provides a standard, configurable solution that follows existing gin middleware patterns.Jira
RAP-435 — Structured JSON logger middleware for gin
Test plan
go build ./...passesgo test -run TestJSONLogger -v— all 7 tests passSkipPathssuppresses output for matched pathsMaskHeadersredacts header values in error messages-raceflag to check for data races🤖 Generated with Claude Code
Summary by cubic
Add structured JSON logger middleware for
gin, a production-ready companion toLogger(), that logs one JSON line per request with timestamp, status, latency_ms, method, path, client_ip, body_bytes, and errors. Addresses RAP-435.JSONLogger()andJSONLoggerWithConfig(JSONLoggerConfig)with configurableOutput.SkipPathsandSkip func(*Context) bool.MaskHeadersto hide sensitive header values.Written for commit 1817deb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests