feat: implement structured JSON logging for gateway requests and payments#206
feat: implement structured JSON logging for gateway requests and payments#206aayushprsingh wants to merge 5 commits into
Conversation
|
@aayushprsingh is attempting to deploy a commit to the ankanmisra's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @aayushprsingh, thanks for opening this PR. Every contribution helps MicroAI-Paygate grow. If you find the project useful, consider starring the repository — it helps others discover it. Star MicroAI-Paygate on GitHub Looking forward to reviewing this PR. |
github-actions
Bot
added
go
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds JSON logging mode, redaction for sensitive errors and recovery dumps, payment/cache status propagation in gateway context, and tests/docs for structured logging and LOG_FORMAT configuration. ChangesJSON Structured Logging with Error Sanitization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GinEngine
participant Middleware
participant GinContext
participant Stdout
Client->>GinEngine: HTTP request
GinEngine->>Middleware: run request/response logging
Middleware->>GinContext: read correlation_id, payment, cache, internal_error
Middleware->>Stdout: write JSON log line
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces structured JSON logging for the gateway (enabled via LOG_FORMAT=json), enriches request context with log metadata (cache/payment/error fields), and adds sanitization to prevent leaking sensitive values in logs.
Changes:
- Add
JSONLoggerMiddlewareemitting structured request logs and suppress plaintext logging when JSON logging is enabled - Add error-string sanitization and propagate error/payment metadata via
gin.Context - Add tests covering JSON logging output and sanitization behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/middleware.go | Adds JSON log entry struct + JSON logging middleware; suppresses correlation plaintext logging when JSON is enabled |
| gateway/main.go | Switches router setup based on LOG_FORMAT; sets payment-related context fields for logging; suppresses plaintext logs in JSON mode |
| gateway/errors.go | Adds sanitization helpers; attaches failure metadata to context; avoids plaintext internal error logging in JSON mode |
| gateway/cache.go | Records cache hit/miss in context and suppresses plaintext cache logs in JSON mode |
| gateway/main_test.go | Adds tests that capture stdout to validate JSON log shape and sanitization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| correlationID, _ := c.Get("correlation_id") | ||
| corrStr, _ := correlationID.(string) |
| if data, err := json.Marshal(entry); err == nil { | ||
| fmt.Println(string(data)) | ||
| } |
| if os.Getenv("LOG_FORMAT") != "json" { | ||
| log.Printf("Cache HIT: %s", cacheKey) | ||
| } |
| oldStdout := os.Stdout | ||
| rPipe, wPipe, _ := os.Pipe() | ||
| os.Stdout = wPipe |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/cache.go`:
- Around line 118-122: The plaintext log.Printf calls that print cache HIT/MISS
info (using cacheKey) are still executed even when LOG_FORMAT=json; update the
code paths where log.Printf is used (the cache hit/miss/error branches around
the cache handling that call log.Printf and set c.Set("cache_status", ...)) to
conditionally emit that plaintext only when os.Getenv("LOG_FORMAT") != "json"
(or replace them with the existing structured logger used elsewhere), so that
when LOG_FORMAT=json is set you suppress/plainly redact the cacheKey output and
rely on the JSON-structured logger for status; target the spots referencing
cacheKey and c.Set("cache_status", "...") and the raw log.Printf invocations for
the change.
In `@gateway/errors.go`:
- Around line 15-22: The current openRouterKeyRegex (sk-or-[a-zA-Z0-9]+) stops
at the first hyphen and can leak keys like sk-or-v1-..., so update the
openRouterKeyRegex declaration to allow the full key token (e.g., include hyphen
and dot characters such as [A-Za-z0-9_.-]+ and optionally a word-boundary) and
keep sanitizeErrorString using that regex; modify the openRouterKeyRegex symbol
so it matches complete OpenRouter keys like sk-or-v1-... to ensure full
redaction.
In `@gateway/main.go`:
- Around line 269-277: The JSON logger middleware is registered after
gin.Recovery which allows Recovery to handle panics and skip the
JSONLoggerMiddleware's post-c.Next() logging; move the JSONLoggerMiddleware()
registration to before gin.Recovery() in the gin.New() branch so
JSONLoggerMiddleware runs earlier (and still executes its post-c.Next() block
even when Recovery handles a panic); update the ordering around r = gin.New(),
r.Use(JSONLoggerMiddleware()), r.Use(gin.Recovery()), r.Use(gin.Recovery())
usage to ensure JSONLoggerMiddleware precedes gin.Recovery.
🪄 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: f8357298-eff5-47fc-a941-1099a00c883e
📒 Files selected for processing (5)
gateway/cache.gogateway/errors.gogateway/main.gogateway/main_test.gogateway/middleware.go
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review this pr |
|
@aayushprsingh which issue are you solving please refer it in the description |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4fe0ccc6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| var ( | ||
| hex64Regex = regexp.MustCompile(`\b[0-9a-fA-F]{64}\b`) |
There was a problem hiding this comment.
When an internal error contains the standard 0x-prefixed form of an Ethereum private key or signature, the leading x and first hex digit are both word characters, so this word-boundary regex does not match and the secret is emitted in internal_error. The repository documents private keys in this form, so make the prefix part of the redaction pattern and add a regression test using an actual 0x-prefixed value.
Useful? React with 👍 / 👎.
| r.Use(gin.Recovery()) | ||
| r.Use(JSONLoggerMiddleware()) |
There was a problem hiding this comment.
When any downstream handler panics, gin.Recovery() catches the panic only after the stack has unwound through JSONLoggerMiddleware, so execution never reaches the logger's post-c.Next() code and the failed request has no structured entry. Register the JSON logger before recovery, or defer its logging logic, so recovered 500 responses remain observable.
Useful? React with 👍 / 👎.
| c.Set("payment_status", "failed") | ||
| c.Set("payment_error", publicMsg) |
There was a problem hiding this comment.
When payment verification succeeds but the AI provider or receipt generation subsequently fails, handleSummarize first records payment_status=success and then respondError overwrites it with failed. This makes upstream and storage outages appear as rejected payments in the structured telemetry; only verification/payment errors should change these payment-specific fields.
Useful? React with 👍 / 👎.
| if os.Getenv("LOG_FORMAT") != "json" { | ||
| log.Printf("Cache HIT: %s", cacheKey) | ||
| } | ||
| c.Set("cache_status", "hit") |
There was a problem hiding this comment.
On a cache hit, this middleware verifies the payment and aborts before handleSummarize, but the success path only sets payment_verification and payment_context; therefore the new JSON entry reports cache_status=hit while omitting both payment_status and payer. Set those logging fields after successful cache-hit verification so payment telemetry is complete for both cached and uncached requests.
Useful? React with 👍 / 👎.
|
|
||
| var ( | ||
| hex64Regex = regexp.MustCompile(`\b[0-9a-fA-F]{64}\b`) | ||
| openRouterKeyRegex = regexp.MustCompile(`sk-or-[a-zA-Z0-9]+`) |
There was a problem hiding this comment.
For actual OpenRouter keys such as sk-or-v1-<secret>, this pattern matches only sk-or-v1 because hyphens are excluded from the character class, leaving -<secret> visible in internal_error. Extend the expression to consume the complete documented key format and test a representative sk-or-v1-... value so structured error logs do not disclose usable credential material.
Useful? React with 👍 / 👎.
| PaymentStatus string `json:"payment_status,omitempty"` | ||
| PaymentError string `json:"payment_error,omitempty"` |
There was a problem hiding this comment.
For the normal first request without payment headers, handleSummarize returns the 402 challenge without setting any context fields, so this newly added payment telemetry omits payment_status and payment_error for the most frequent x402 payment event. Record a distinct status such as required before returning the challenge so dashboards can distinguish unsigned payment challenges from unrelated HTTP 402 responses or requests with no payment flow.
Useful? React with 👍 / 👎.
|
|
||
| r := gin.Default() | ||
| var r *gin.Engine | ||
| if os.Getenv("LOG_FORMAT") == "json" { |
There was a problem hiding this comment.
The new behavior is reachable only through LOG_FORMAT=json, but that variable is absent from .env.example, the root configuration table, and gateway/README.md. Operators following the repository's documented setup therefore cannot discover or reliably configure the feature; add the variable, accepted value, default behavior, and output semantics to the synchronized configuration documentation.
Useful? React with 👍 / 👎.
| if os.Getenv("LOG_FORMAT") != "json" { | ||
| log.Printf("Failed to retrieve receipt %s: %v", id, err) | ||
| } | ||
| c.JSON(500, gin.H{"error": "Failed to retrieve receipt"}) |
There was a problem hiding this comment.
When receipt retrieval fails in JSON mode, this branch now suppresses the only log containing err and returns the response directly instead of populating internal_error, so the structured request entry contains only a generic 500 with no explanation. Use respondError or explicitly attach a sanitized internal error before returning, otherwise Redis and receipt-store outages cannot be diagnosed from gateway logs.
Useful? React with 👍 / 👎.
| // Cache MISS | ||
| log.Printf("Cache MISS: %s", cacheKey) | ||
| if os.Getenv("LOG_FORMAT") != "json" { | ||
| log.Printf("Cache MISS: %s", cacheKey) | ||
| } | ||
| c.Set("cache_status", "miss") |
There was a problem hiding this comment.
When Redis returns a connection error or a cached value is corrupt, getFromCache returns an error and this path unconditionally records cache_status=miss. That makes backend outages and invalid cache data indistinguishable from a normal absent key in the new structured telemetry; check for the Redis miss sentinel separately and record an error or bypass status for other failures.
Useful? React with 👍 / 👎.
| if os.Getenv("LOG_FORMAT") == "json" { | ||
| gin.SetMode(gin.ReleaseMode) | ||
| r = gin.New() | ||
| r.Use(gin.Recovery()) |
There was a problem hiding this comment.
When a request with X-402-Signature encounters a broken-pipe panic, Gin's default recovery path dumps the request and sanitizes only Authorization; the x402 signature, nonce, and timestamp therefore go to stderr in plaintext even in JSON mode. Configure custom recovery output that redacts these payment headers, since a signature may remain replayable until its nonce is successfully consumed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/main_test.go (1)
621-678: 💤 Low valueConsider adding ClientIP assertion for completeness.
The test verifies most
JSONLogEntryfields but omitsClientIP. Sincereq.RemoteAddris explicitly set to"127.0.0.1:12345"on line 647, adding an assertion likerequire.Equal(t, "127.0.0.1", logEntry.ClientIP)would provide more complete coverage of the middleware's output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/main_test.go` around lines 621 - 678, The test TestJSONLoggerMiddleware is missing an assertion for ClientIP even though req.RemoteAddr is set; update the test in gateway/main_test.go (inside TestJSONLoggerMiddleware) to assert that JSONLogEntry.ClientIP equals "127.0.0.1" after unmarshalling (use require.Equal with logEntry.ClientIP and "127.0.0.1") so the middleware's client IP extraction is covered — place the assertion alongside the other require.Equal checks for Status, Path, Method, etc.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/main_test.go`:
- Around line 621-678: The test TestJSONLoggerMiddleware is missing an assertion
for ClientIP even though req.RemoteAddr is set; update the test in
gateway/main_test.go (inside TestJSONLoggerMiddleware) to assert that
JSONLogEntry.ClientIP equals "127.0.0.1" after unmarshalling (use require.Equal
with logEntry.ClientIP and "127.0.0.1") so the middleware's client IP extraction
is covered — place the assertion alongside the other require.Equal checks for
Status, Path, Method, etc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 393415b7-467d-48bd-8590-3b05b5a8bae0
📒 Files selected for processing (2)
gateway/errors.gogateway/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/errors.go
Addresses the outstanding bot review on PR AnkanMisra#206: - Centralize the scattered LOG_FORMAT=="json" checks into a single package-level jsonLogging flag set once at startup (initLogFormat). - Suppress/sanitize the remaining plaintext logs that still fired in JSON mode: invalid-JSON debug, cache-hit verification error, and cached receipt failure in cache.go. - Register JSONLoggerMiddleware before gin.Recovery so panics still produce a structured entry, and route recovery through a writer that redacts X-402-Signature/Nonce/Timestamp (the default dump only scrubs Authorization; a leaked signature is replayable until its nonce burns). - Record payment_status=success + payer on cache-hit verification so cached requests carry the same telemetry as handleSummarize. - Set payment_status=required on the unsigned 402 challenge so dashboards separate it from unrelated 402s. - Preserve a sanitized internal_error in handleGetReceipt under JSON mode so receipt-store outages stay diagnosable. - Distinguish a genuine cache miss (redis.Nil) from a backend error so Redis outages aren't masked as ordinary misses. - Document LOG_FORMAT in .env.example, README.md, and gateway/README.md. Tests: redacting writer scrubs x402 headers, 402 sets required status, plus the existing redaction/payment-status regression tests. Full gateway suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/cache.go (1)
200-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not count backend cache failures as cache misses.
Line [203]-Line [206] classifies Redis/backend failures as
cache_status="error", but Line [220] still incrementscacheMissesunconditionally. That keeps masking outages as ordinary misses in metrics.Suggested fix
routePath := c.FullPath() if routePath == "" { routePath = "unknown" } - cacheMisses.WithLabelValues(routePath).Inc() + if cacheStatus == "miss" { + cacheMisses.WithLabelValues(routePath).Inc() + }As per coding guidelines, "Check per-IP/per-wallet bucket selection, cleanup, disabled-mode behavior, Redis URL validation, cache miss/fallback behavior, and concurrency safety in rate limiting/cache logic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/cache.go` around lines 200 - 221, The code treats non-nil Redis errors as cache_status="error" but still unconditionally calls cacheMisses.WithLabelValues(routePath).Inc(); change the logic so metric increments only for genuine misses: after computing cacheStatus (using errors.Is(cacheErr, redis.Nil)), call cacheMisses.WithLabelValues(routePath).Inc() only when cacheStatus == "miss" (i.e., the branch where redis.Nil was detected), while still setting c.Set("cache_status", cacheStatus) and preserving the existing logging of error vs miss (see variables cacheStatus, cacheErr, cacheMisses, c.Set, routePath, sanitizeErrorString).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@gateway/cache.go`:
- Around line 200-221: The code treats non-nil Redis errors as
cache_status="error" but still unconditionally calls
cacheMisses.WithLabelValues(routePath).Inc(); change the logic so metric
increments only for genuine misses: after computing cacheStatus (using
errors.Is(cacheErr, redis.Nil)), call
cacheMisses.WithLabelValues(routePath).Inc() only when cacheStatus == "miss"
(i.e., the branch where redis.Nil was detected), while still setting
c.Set("cache_status", cacheStatus) and preserving the existing logging of error
vs miss (see variables cacheStatus, cacheErr, cacheMisses, c.Set, routePath,
sanitizeErrorString).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12de0640-5bdd-40a7-a80b-bfe8ff82b7ed
📒 Files selected for processing (8)
.env.exampleREADME.mdgateway/README.mdgateway/cache.gogateway/errors.gogateway/main.gogateway/main_test.gogateway/middleware.go
✅ Files skipped from review due to trivial changes (3)
- gateway/README.md
- .env.example
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/errors.go
- gateway/main.go
Signed-off-by: aayushprsingh <aayushprsingh@users.noreply.github.com>
respondError unconditionally set payment_status="failed", clobbering a verified payment when a later receipt-generation step failed — corrupting the very metric this feature captures. Now it only marks failed when no prior status exists. Redaction regexes missed real key formats: 0x-prefixed private keys (the \b anchor never matched after "x") and versioned sk-or-v1-<secret> tokens (charset excluded dashes). Widened both patterns and reordered so API-key redaction runs first. Added tests covering realistic key shapes and the post-success error path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the outstanding bot review on PR AnkanMisra#206: - Centralize the scattered LOG_FORMAT=="json" checks into a single package-level jsonLogging flag set once at startup (initLogFormat). - Suppress/sanitize the remaining plaintext logs that still fired in JSON mode: invalid-JSON debug, cache-hit verification error, and cached receipt failure in cache.go. - Register JSONLoggerMiddleware before gin.Recovery so panics still produce a structured entry, and route recovery through a writer that redacts X-402-Signature/Nonce/Timestamp (the default dump only scrubs Authorization; a leaked signature is replayable until its nonce burns). - Record payment_status=success + payer on cache-hit verification so cached requests carry the same telemetry as handleSummarize. - Set payment_status=required on the unsigned 402 challenge so dashboards separate it from unrelated 402s. - Preserve a sanitized internal_error in handleGetReceipt under JSON mode so receipt-store outages stay diagnosable. - Distinguish a genuine cache miss (redis.Nil) from a backend error so Redis outages aren't masked as ordinary misses. - Document LOG_FORMAT in .env.example, README.md, and gateway/README.md. Tests: redacting writer scrubs x402 headers, 402 sets required status, plus the existing redaction/payment-status regression tests. Full gateway suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aayushprsingh
force-pushed
the
gssoc-fix-192
branch
from
5a61328 to
1142d22
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This PR adds structured JSON logging capability to the gateway service. When LOG_FORMAT=json is set, Gin is configured in ReleaseMode, default logger is bypassed, and JSONLoggerMiddleware is registered to write structured execution details to stdout. Verification errors and successful payments are recorded dynamically inside the Gin context. This update also adds cache status tracking and ensures sensitive data like API keys and private keys are redacted from internal error logs.
Summary by CodeRabbit
New Features
LOG_FORMAT) with request metadata and correlation/payment fields.payment_statusand payer address during payment processing; includes cache hit/miss status in logs.Bug Fixes
payment_status=requiredwhen payment headers are missing.Tests
Documentation
.env.exampleto documentLOG_FORMAT.