🔥 feat: Add support for contextual logs#4241
Open
ReneWerner87 wants to merge 25 commits intomainfrom
Open
Conversation
3 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a precompiled log-template system and context-tag registry, integrates context-based template rendering into the default and middleware loggers (WithContext accepts any), introduces global/per-middleware tag registration, redaction helpers, tests, and documentation updates describing the new APIs and migration notes. ChangesContext-aware logging, template engine, and logger integration
Sequence Diagram(s)sequenceDiagram
participant Handler as Request Handler
participant AppLogger as log.WithContext(c)
participant Template as Context Template
participant TagRegistry as Tag Renderer (e.g., requestid)
participant Output as Log Output
Handler->>AppLogger: WithContext(fiber.Ctx)
AppLogger->>AppLogger: retain context value
Handler->>AppLogger: .Info("message")
AppLogger->>Template: load precompiled template (atomic)
alt template present
AppLogger->>Template: Execute(ctx, data)
Template->>TagRegistry: invoke tag func (e.g., requestid)
TagRegistry->>TagRegistry: extract value from ctx (FromContext/UserValue)
TagRegistry->>Template: write sanitized bytes
Template->>AppLogger: return rendered bytes
AppLogger->>Output: write prefix + rendered context + message
else no template or ctx
AppLogger->>Output: write prefix + message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a unified log template mechanism in the internal/logtemplate package, which is now utilized by both the log package and the logger middleware. Key enhancements include the addition of log.WithContext and log.SetContextTemplate to allow for consistent, context-aware logging across the application. Documentation and generated interface files were also updated to reflect these changes and clarify behavior regarding multipart form parsing and route URL generation. A review comment suggested optimizing slice allocation in the template builder to prevent unnecessary reallocations.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4241 +/- ##
==========================================
+ Coverage 91.20% 91.27% +0.06%
==========================================
Files 127 129 +2
Lines 12551 12695 +144
==========================================
+ Hits 11447 11587 +140
- Misses 692 699 +7
+ Partials 412 409 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared internal log template engine to reuse template parsing/rendering across Fiber’s request logger middleware and the log.WithContext application logger, enabling consistent “contextual tag” rendering (e.g., request IDs).
Changes:
- Extract template parsing/execution into
internal/logtemplateand switchmiddleware/loggerto use it. - Add configurable contextual template rendering for Fiber’s default logger via
log.SetContextTemplate+log.WithContext. - Update docs and add tests for legacy logger chains, contextual tags, and
${value:key}rendering.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/logtemplate/template.go | Adds reusable template parsing + chain execution utilities. |
| internal/logtemplate/errors.go | Introduces shared template parse error(s). |
| internal/logtemplate/template_test.go | Adds unit tests for template execution and missing-parameter behavior. |
| middleware/logger/logger.go | Replaces per-middleware template compilation with logtemplate.Build(...). |
| middleware/logger/default_logger.go | Reuses shared chain executor for default logger rendering. |
| middleware/logger/config.go | Re-exports shared Buffer/Func types for logger tag functions. |
| middleware/logger/errors.go | Maps legacy logger template error to shared logtemplate error. |
| middleware/logger/template_chain.go | Removes now-superseded in-package template compilation logic. |
| middleware/logger/logger_test.go | Adds regression coverage for legacy TemplateChain/LogFuncChain behavior. |
| log/context.go | Adds SetContextTemplate and ${value:key} tag support for default logger context enrichment. |
| log/default.go | Implements contextual template rendering in default logger when bound via WithContext. |
| log/context_test.go | Adds tests for ${value:key} and custom context tags. |
| log/default_test.go | Updates caller expectations and adds coverage for contextual template output. |
| log/log.go | Updates AllLogger docs/comments to reflect context binding responsibilities. |
| docs/api/log.md | Documents SetContextTemplate, custom tags, and log.WithContext(c) behavior. |
| docs/middleware/logger.md | Documents reuse of tag/template approach for application logging. |
| docs/whats_new.md | Adds “What’s New” entry for contextual application logging templates. |
| ctx_interface_gen.go | Updates interface docs (multipart BodyLimit notes; HTML <p> escaping note; route URL comment). |
| req_interface_gen.go | Updates interface docs for multipart parsing/BodyLimit enforcement. |
| res_interface_gen.go | Updates interface docs (HTML <p> escaping note; route URL comment). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/logger/default_logger.go (1)
119-123:⚠️ Potential issue | 🟡 MinorTest coverage for the error-append path (lines 121–123) appears to be missing.
ExecuteChainscorrectly short-circuits on the first error and returns it, allowing the error string to be appended to the buffer. This matches the short-circuit semantics of the previous manual chain iteration. However, no test currently exercises the error path where a function inLogFuncChainor a buffer write withinExecuteChainsfails. The existing error output tests (Test_Logger_ErrorOutput*) exercise stream write failures only, not template execution failures. Consider adding a test case that injects a failingLogFuncor wraps the buffer to trigger an error insideExecuteChains, then verifies the error message is correctly appended to the log output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/logger/default_logger.go` around lines 119 - 123, Add a unit test that exercises the error-append path of ExecuteChains by injecting a failing LogFunc or a writer that errors during ExecuteChains and asserting the returned error string is appended to the log buffer; specifically, create a test (e.g., Test_Logger_ErrorOutput_TemplateFailure) that builds a Logger config with a LogFuncChain containing a function that returns an error (or wraps the buffer with an io.Writer whose Write returns an error), call logtemplate.ExecuteChains (via the logger flow used in existing Test_Logger_ErrorOutput* tests), capture the buffer output and the error, and assert the buffer contains err.Error() as appended text to validate the error-append behavior in default_logger.go.
🧹 Nitpick comments (2)
middleware/logger/logger.go (1)
100-101:TemplateChain/LogFuncChainare written into the pooledDataon every request — confirm no consumer mutates them.Both fields share the same underlying slices across all requests. The default logger now passes them straight to
logtemplate.ExecuteChains, which only reads, so this is safe today. Worth a brief note inData's GoDoc that customLoggerFuncimplementations must treat these slices as read-only, since mutation would corrupt every subsequent request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/logger/logger.go` around lines 100 - 101, The Data struct's TemplateChain and LogFuncChain fields are shared pooled slices and must be treated as read-only by consumers; update the GoDoc for type Data to explicitly state that TemplateChain and LogFuncChain are shared across requests and must not be mutated by custom LoggerFunc implementations (they may only read/iterate), and add a short comment near the assignment site (where data.TemplateChain and data.LogFuncChain are set) referencing logtemplate.ExecuteChains as the current reader to make the expectation clear.log/context.go (1)
35-48: Consider returning the build error instead of panicking.
SetContextTemplatepanics iflogtemplate.Buildrejects the format string. For a library setter API this is unusual — callers typically expectSet*functions to be infallible or to return an error. Panicking forces users to either validate the format themselves or wrap calls inrecover(). Since this is a brand-new API, the signature change is essentially free.♻️ Suggested change to return an error
-// SetContextTemplate configures contextual fields rendered by WithContext for Fiber's default logger. -func SetContextTemplate(config ContextConfig) { +// SetContextTemplate configures contextual fields rendered by WithContext for Fiber's default logger. +// It returns an error if config.Format cannot be parsed. +func SetContextTemplate(config ContextConfig) error { if config.Format == "" { contextTemplate.Store(nil) - return + return nil } tmpl, err := logtemplate.Build[context.Context, ContextData](config.Format, createContextTagMap(config.CustomTags)) if err != nil { - panic(err) + return err } contextTemplate.Store(tmpl) + return nil }If a panicking variant is still desirable, consider a
MustSetContextTemplatewrapper following the standard librarytext/template.Mustconvention.Please confirm whether the panic-on-error semantics is intentional (e.g., to surface misconfiguration loudly at startup) and double-check whether any of the new docs/examples in
docs/api/log.mdalready advertise this signature, since changing it would require a docs update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/context.go` around lines 35 - 48, Change SetContextTemplate to return an error instead of panicking: call logtemplate.Build inside SetContextTemplate, and if it returns an error, return that error to the caller; on success store the template into contextTemplate via contextTemplate.Store(tmpl) and return nil. Keep the current panic behavior as an optional MustSetContextTemplate wrapper (e.g., MustSetContextTemplate calls SetContextTemplate and panics on non-nil error) if you want a loud-startup variant. Update references to SetContextTemplate callers to handle the returned error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@log/default.go`:
- Around line 227-242: The writeContext function currently executes the template
directly into the provided buf which can leave a partial render if tmpl.Execute
fails; change writeContext to first load the template (contextTemplate.Load) and
execute it into a temporary/scratch buffer (local Buffer or bytes buffer)
instead of writing straight into l's buf, and only append the scratch buffer to
buf if tmpl.Execute succeeds; if Execute returns an error, avoid appending the
partial content and surface the failure once via an internal logger or write a
safe fixed fallback (e.g., omit context or write "[context error]"), referencing
the writeContext method, tmpl.Execute call, the contextTemplate.Load usage, and
ContextData so you modify the same symbols.
---
Outside diff comments:
In `@middleware/logger/default_logger.go`:
- Around line 119-123: Add a unit test that exercises the error-append path of
ExecuteChains by injecting a failing LogFunc or a writer that errors during
ExecuteChains and asserting the returned error string is appended to the log
buffer; specifically, create a test (e.g.,
Test_Logger_ErrorOutput_TemplateFailure) that builds a Logger config with a
LogFuncChain containing a function that returns an error (or wraps the buffer
with an io.Writer whose Write returns an error), call logtemplate.ExecuteChains
(via the logger flow used in existing Test_Logger_ErrorOutput* tests), capture
the buffer output and the error, and assert the buffer contains err.Error() as
appended text to validate the error-append behavior in default_logger.go.
---
Nitpick comments:
In `@log/context.go`:
- Around line 35-48: Change SetContextTemplate to return an error instead of
panicking: call logtemplate.Build inside SetContextTemplate, and if it returns
an error, return that error to the caller; on success store the template into
contextTemplate via contextTemplate.Store(tmpl) and return nil. Keep the current
panic behavior as an optional MustSetContextTemplate wrapper (e.g.,
MustSetContextTemplate calls SetContextTemplate and panics on non-nil error) if
you want a loud-startup variant. Update references to SetContextTemplate callers
to handle the returned error.
In `@middleware/logger/logger.go`:
- Around line 100-101: The Data struct's TemplateChain and LogFuncChain fields
are shared pooled slices and must be treated as read-only by consumers; update
the GoDoc for type Data to explicitly state that TemplateChain and LogFuncChain
are shared across requests and must not be mutated by custom LoggerFunc
implementations (they may only read/iterate), and add a short comment near the
assignment site (where data.TemplateChain and data.LogFuncChain are set)
referencing logtemplate.ExecuteChains as the current reader to make the
expectation clear.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 643b9135-e9a2-48a3-9006-9aef019af8fd
📒 Files selected for processing (20)
ctx_interface_gen.godocs/api/log.mddocs/middleware/logger.mddocs/whats_new.mdinternal/logtemplate/errors.gointernal/logtemplate/template.gointernal/logtemplate/template_test.golog/context.golog/context_test.golog/default.golog/default_test.golog/log.gomiddleware/logger/config.gomiddleware/logger/default_logger.gomiddleware/logger/errors.gomiddleware/logger/logger.gomiddleware/logger/logger_test.gomiddleware/logger/template_chain.goreq_interface_gen.gores_interface_gen.go
💤 Files with no reviewable changes (1)
- middleware/logger/template_chain.go
10 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 4239c1d | Previous: 3e610ae | Ratio |
|---|---|---|---|
Benchmark_Logger/WithBytesAndStatus (github.com/gofiber/fiber/v3/middleware/logger) - B/op |
72 B/op |
0 B/op |
+∞ |
Benchmark_Logger/WithBytesAndStatus (github.com/gofiber/fiber/v3/middleware/logger) - allocs/op |
3 allocs/op |
0 allocs/op |
+∞ |
Benchmark_Logger/WithTagParameter (github.com/gofiber/fiber/v3/middleware/logger) - B/op |
72 B/op |
0 B/op |
+∞ |
Benchmark_Logger/WithTagParameter (github.com/gofiber/fiber/v3/middleware/logger) - allocs/op |
3 allocs/op |
0 allocs/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/middleware/logger.md (1)
57-75: Consider adding a type assertion in the example for safety.The
requestid.FromContext(c)call at line 64 receivesc any, butrequestid.FromContextlikely expectsfiber.Ctx. While this works at runtime whencis actually afiber.Ctx, adding a type assertion makes the example clearer and more defensive.💡 Suggested improvement
CustomTags: map[string]log.ContextTagFunc{ "requestid": func(output log.Buffer, c any, _ *log.ContextData, _ string) (int, error) { - return output.WriteString(requestid.FromContext(c)) + if fiberCtx, ok := c.(fiber.Ctx); ok { + return output.WriteString(requestid.FromContext(fiberCtx)) + } + return 0, nil }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/middleware/logger.md` around lines 57 - 75, Update the CustomTags requestid tag function to perform a type assertion on the incoming c (the parameter passed to log.ContextTagFunc) before calling requestid.FromContext: inside the anonymous function in log.SetContextTemplate's CustomTags map assert c to the expected type (e.g., fiber.Ctx or *fiber.Ctx depending on your app), check the ok boolean, and call requestid.FromContext only when the assertion succeeds (returning a sensible fallback like an empty string when it fails) so requestid.FromContext is never invoked with an unexpected type.log/context.go (1)
58-77: Custom tags can silently override the built-invalue:tag.
maps.Copy(tags, customTags)overwrites entries if a user provides a custom tag named"value:". If this is intentional to allow overriding, consider documenting it inContextConfig.CustomTags. If not, consider validating or warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/context.go` around lines 58 - 77, createContextTagMap currently uses maps.Copy(tags, customTags) which lets a custom tag named TagContextValue ("value:") silently override the built-in handler; update createContextTagMap to detect if customTags contains TagContextValue and either (a) prevent/ignore that key (retain the built-in TagContextValue) or (b) log/warn the user and then decide whether to override, and reflect this behavior in ContextConfig.CustomTags docs; specifically modify the logic around maps.Copy(tags, customTags) in createContextTagMap to check for the presence of TagContextValue in customTags and handle it according to chosen policy, referencing TagContextValue, createContextTagMap, and ContextConfig.CustomTags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@log/default_test.go`:
- Around line 123-153: This test (Test_WithContextTemplate) is missing
t.Parallel() per conventions; either add t.Parallel() as the first executable
statement in the test to allow parallel execution (keep the existing t.Cleanup
that calls MustSetContextTemplate(ContextConfig{}) so global state is restored),
or if you determine shared global state (SetContextTemplate /
MustSetContextTemplate) cannot be safely parallelized, add a brief comment above
the Test_WithContextTemplate declaration explaining why it must run serially and
do not call t.Parallel(); reference SetContextTemplate, MustSetContextTemplate,
t.Cleanup and initDefaultLogger when making the change.
- Around line 155-177: Add t.Parallel() as the first line of
Test_WithContextTemplateFailureOmitsPartialContext, and remove the in-callback
test assertion; inside the ContextTagFunc implementation (the anonymous func
passed in CustomTags) replace require.NoError(t, err) with explicit error
handling that returns the write error (e.g., if err != nil { return 0, err }) so
the tag propagates the write failure instead of asserting inside the callback;
keep the rest of the test (SetContextTemplate, cleanup, SetOutput,
WithContext().Info and the final require.Equal) unchanged.
---
Nitpick comments:
In `@docs/middleware/logger.md`:
- Around line 57-75: Update the CustomTags requestid tag function to perform a
type assertion on the incoming c (the parameter passed to log.ContextTagFunc)
before calling requestid.FromContext: inside the anonymous function in
log.SetContextTemplate's CustomTags map assert c to the expected type (e.g.,
fiber.Ctx or *fiber.Ctx depending on your app), check the ok boolean, and call
requestid.FromContext only when the assertion succeeds (returning a sensible
fallback like an empty string when it fails) so requestid.FromContext is never
invoked with an unexpected type.
In `@log/context.go`:
- Around line 58-77: createContextTagMap currently uses maps.Copy(tags,
customTags) which lets a custom tag named TagContextValue ("value:") silently
override the built-in handler; update createContextTagMap to detect if
customTags contains TagContextValue and either (a) prevent/ignore that key
(retain the built-in TagContextValue) or (b) log/warn the user and then decide
whether to override, and reflect this behavior in ContextConfig.CustomTags docs;
specifically modify the logic around maps.Copy(tags, customTags) in
createContextTagMap to check for the presence of TagContextValue in customTags
and handle it according to chosen policy, referencing TagContextValue,
createContextTagMap, and ContextConfig.CustomTags.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bcdd49fc-4694-4c27-8d24-ea48ea8aa257
📒 Files selected for processing (15)
docs/api/log.mddocs/middleware/logger.mddocs/whats_new.mdinternal/logtemplate/errors.gointernal/logtemplate/template.golog/context.golog/context_test.golog/default.golog/default_test.golog/fiberlog.golog/log.gomiddleware/logger/data.gomiddleware/logger/errors.gomiddleware/logger/logger.gomiddleware/logger/logger_test.go
✅ Files skipped from review due to trivial changes (1)
- middleware/logger/data.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/logtemplate/errors.go
- log/log.go
- docs/whats_new.md
- internal/logtemplate/template.go
…mproved type safety
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@log/default.go`:
- Around line 263-270: The WithContext method on defaultLogger currently calls
newRetainedContext(ctx) but always decrements depth (depth: l.depth - 1) even
when newRetainedContext(nil) returns nil; change WithContext (and references to
newRetainedContext) to retain the original l.depth unless a non-nil retained
context is returned — i.e., obtain rc := newRetainedContext(ctx) and set ctx: rc
and depth to l.depth-1 only when rc != nil, otherwise keep depth: l.depth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
log/default_test.go (1)
33-62: Document or isolate the shared logger state in these caller tests.
Test_WithContextCallerandTest_WithContextNilCallerboth reassign package-globallogger/output, but they neither callt.Parallel()nor explain why they must stay serial. Please either add a small cleanup/reset hook or a brief serial-execution note.♻️ Suggested cleanup
func Test_WithContextCaller(t *testing.T) { + t.Cleanup(initDefaultLogger) logger = &defaultLogger{ stdlog: log.New(os.Stderr, "", log.Lshortfile), depth: 4, } @@ func Test_WithContextNilCaller(t *testing.T) { + t.Cleanup(initDefaultLogger) logger = &defaultLogger{ stdlog: log.New(os.Stderr, "", log.Lshortfile), depth: 4, }As per coding guidelines,
**/*_test.goshould invoket.Parallel()at the start of each test and subtest when safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/default_test.go` around lines 33 - 62, Tests Test_WithContextCaller and Test_WithContextNilCaller mutate package-global logger and output (logger, SetOutput) without isolating state; either mark them serial with a comment or, better, restore global state: at start of each test call t.Parallel() only if you make logger/output local or add a defer to save and restore the original logger and output after the test (capture current logger and writer, then defer restore), ensuring WithContext calls and Info assertions run against the isolated state; reference the functions/vars logger, SetOutput, WithContext, Info, Test_WithContextCaller, and Test_WithContextNilCaller when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@log/default_test.go`:
- Around line 33-62: Tests Test_WithContextCaller and Test_WithContextNilCaller
mutate package-global logger and output (logger, SetOutput) without isolating
state; either mark them serial with a comment or, better, restore global state:
at start of each test call t.Parallel() only if you make logger/output local or
add a defer to save and restore the original logger and output after the test
(capture current logger and writer, then defer restore), ensuring WithContext
calls and Info assertions run against the isolated state; reference the
functions/vars logger, SetOutput, WithContext, Info, Test_WithContextCaller, and
Test_WithContextNilCaller when applying the fix.
Address findings from a 10-perspective review (maintainer, DX, performance,
clean code, SOLID, concurrency, errors, testing, docs, security) of the
codex/reusable-log-template branch.
API & ergonomics:
- Drop log.Format / MustFormat sugar; SetContextTemplate(ContextConfig{...})
is now the canonical entry point and the parallel API confusion is gone.
- Promote tag names (TagRequestID, TagUsername, TagAPIKey, TagCSRFToken,
TagSessionID, TagRequestIDDashed) to log/ constants. KeyValueFormat,
RequestIDFormat, defaultContextTagMap and middleware/logger's stub list all
build from these so renames cascade automatically.
- Export ErrContextTagInvalid, ErrContextTagReserved, ErrTagInvalid for
errors.Is symmetry.
- SetContextTemplate now rejects an attempt to override the reserved
TagContextValue ("value:") via CustomTags, matching RegisterContextTag.
- Drop dead createContextTagMap helper.
Errors:
- Split internal/logtemplate's ambiguous ErrParameterMissing into a single
ErrUnknownTag sentinel and a typed UnknownTagError{Tag, Param, Hint}. Hint
is populated when a bare ${tag} was referenced but a parametric base of the
same name is registered, surfacing "did you mean ${tag:PARAM}?" suggestions.
- middleware/logger gains a public ErrUnknownTag + UnknownTagError surface
via translateBuildError. Replaces the fragile string-stripping that
previously rebuilt typed errors from sentinel.Error()+": "+param.
Concurrency & init:
- middleware/logger pre-registers empty stubs for the 6 known middleware tag
names at package init so a logger format that references them compiles
even when the corresponding middleware (basicauth, csrf, keyauth,
requestid, session) has not yet been initialized. The slot is filled in
on the first New() call.
DRY across middlewares:
- New helper logger.RegisterContextTag(name, extract func(any) string) fans
out to both registries. The 5 ctx-tag middlewares lose ~12 lines each of
duplicated registration boilerplate.
- New internal/redact package centralizes the keyauth/session redaction
policy (MinLength=8, PrefixLength=4, Mask="****") that previously diverged
silently between redactContextValue (<=4) and redactSessionID (<=8).
- New internal/loggertest package collapses the SetOutput/MustFormat/cleanup
boilerplate the 5 middleware tests duplicated.
Performance / API surface:
- Trim public Buffer interface from 10 methods to 3 (Write, WriteByte,
WriteString) — interface-segregation win and the only methods renderers
actually call. *bytebufferpool.ByteBuffer keeps satisfying the interface
without exposing Set/Bytes/Len/etc. through it.
- Rewrite middleware/logger.appendInt to use a 20-byte stack scratch +
strconv.AppendInt + Buffer.Write instead of Set(strconv.AppendInt(...)),
keeping the alloc-free property without needing the wide Buffer surface.
- Promote middleware/logger errPadding magic 15 to defaultErrPadding const.
Resilience:
- defaultLogger.writeContext no longer silently swallows render errors;
failures emit a "[ctx-render-error: ...] " marker via the outer buffer so
operators see misconfigured tags. Scratch buffer still isolates partial
tag writes from the live log line.
- defaultContextValueTag now sanitizes ASCII control bytes (CR, LF, NUL,
C0, DEL — tab preserved) before they reach the log line, mitigating
log-injection via attacker-controlled context values flowing through
${value:KEY}. Alloc-free fast path when input is already clean.
Tests:
- Drop brittle line-number assertions in log/default_test.go in favor of
runtime.Caller-derived line numbers — surrounding edits no longer break
caller-attribution tests.
- Add Test_ContextTemplate_ConcurrentRegistration race-stress test
exercising RegisterContextTag / SetContextTemplate / template Load
concurrently.
- Add benchmarks for logtemplate.Build, logtemplate.Execute, contextual
defaultContextValueTag and the context template execute path.
- Convert Test_Logger_TemplateParameterMissingCompatibility into a
table-driven typed-error assertion via errors.As.
Docs:
- docs/api/log.md gains a Signatures block, glossary (format/template/tag),
bidirectional cross-link to docs/middleware/logger.md, an updated
custom-tag example using SetContextTemplate, an init-order :::note, and a
:::caution describing the new ${value:KEY} sanitization policy.
- docs/whats_new.md announces the WithContext(ctx context.Context) ->
WithContext(ctx any) breaking signature change for AllLogger[T] adapters
and lists the new exported symbols across log/ and middleware/logger/.
Verified: go build, go vet, full module go test -race all green.
Benchmarks: ContextTemplate_Execute 19.7 ns/op, 0 allocs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
/gemini review |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a unified log template and tag mechanism across the log package and middleware/logger. A significant breaking change is the update of the WithContext signature to accept any instead of context.Context, enabling the logger to extract values from fiber.Ctx and other context-like objects. The update includes global tag registration, automatic tag registration for built-in middlewares (such as requestid, basicauth, csrf, keyauth, and session), and internal utilities for template parsing and sensitive data redaction. Documentation and tests have been extensively updated to cover these new features. I have no feedback to provide.
CI surfaced lint findings that go vet / go test miss: - modernize: log/context.go needsControlSanitize loop -> slices.ContainsFunc; needsControlSanitizeString loop -> strings.IndexFunc. - testifylint (error-is-as): require.True(errors.As(...)) -> require.ErrorAs across context_test.go, default_test.go, template_test.go, errors_test.go, logger_test.go. - testifylint (error-nil): require.Nil(translateBuildError(...)) -> require.NoError(...) so the linter recognises the value as an error. - testifylint (contains): require.True(strings.Contains(...)) -> require.Contains(...). - testifylint (len): require.Equal(t, len(x), len(y)) -> require.Len. - perfsprint: replace string-concat-in-loop in Test_ContextTemplate_DefaultTagsRenderEmpty with a slice + strings.Join. - errcheck: add //nolint:errcheck with reasons on the deliberately-discarded Buffer.Write fallback in writeContext, and on the race-stress test loop in Test_ContextTemplate_ConcurrentRegistration where transient errors are the point of the stress. Verified locally with golangci-lint v2.6.0 (matches CI) and modernize@latest: both report 0 issues. Race-test suite still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/logger/logger_test.go (1)
518-531:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the "WithAllTags" benchmarks in sync with this format change.
Test_Logger_Alldrops${non}here, but bothBenchmark_LoggerandBenchmark_Logger_Parallelstill build a format containing${non}later in this file. With the new eager unknown-tag validation, those benchmark setups will now panic before they can run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/logger/logger_test.go` around lines 518 - 531, Benchmark_Logger and Benchmark_Logger_Parallel still include the removed unknown tag `${non}` and will panic due to the eager unknown-tag validation; update their logger format setup to match Test_Logger_All by removing `${non}` (or otherwise replace it with a valid tag) when constructing the Config.Format string used in Benchmark_Logger and Benchmark_Logger_Parallel so the formats are consistent with Test_Logger_All and no unknown tags are present.
🧹 Nitpick comments (1)
middleware/logger/tags.go (1)
59-87: Add test cleanup for globalRegisterTagregistrations.Tests
Test_Logger_RegisteredTagandTest_Logger_CustomTagOverridesRegisteredTagcallRegisterTagwithout cleanup. While current tests use unique tag names and avoid collisions, the global registry persists across the test suite lifetime. Add an unexported reset helper and call it viat.Cleanup()to prevent future test pollution:♻️ Suggested addition
+// resetRegisteredTagsForTest resets the global tag registry. For test use only. +func resetRegisteredTagsForTest() { + registeredTags.Lock() + defer registeredTags.Unlock() + registeredTags.m = make(map[string]LogFunc) +}Then in each test that calls
RegisterTag, add:t.Cleanup(resetRegisteredTagsForTest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/logger/tags.go` around lines 59 - 87, Tests that call RegisterTag mutate the global registeredTags map without cleanup; add an unexported helper (e.g., resetRegisteredTagsForTest) that acquires registeredTags.Lock(), clears or reinitializes registeredTags.m, and releases the lock, then update each test that calls RegisterTag (like Test_Logger_RegisteredTag and Test_Logger_CustomTagOverridesRegisteredTag) to call t.Cleanup(resetRegisteredTagsForTest) so the global registry is reset after each test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@log/context.go`:
- Around line 242-249: The loop in needsControlSanitize should be replaced with
slices.ContainsFunc to satisfy the linter: add an import for "slices" and change
needsControlSanitize to return slices.ContainsFunc(p, func(b byte) bool { return
isControlByte(b) }); keep needsControlSanitizeString unchanged (per note) and
ensure you reference the existing function name needsControlSanitize and helper
isControlByte when making the change.
In `@log/default.go`:
- Around line 31-35: newRetainedContext currently treats any non-nil interface
as valid, which misses typed-nil values (e.g., (*fasthttp.RequestCtx)(nil));
update newRetainedContext to detect typed nils by using reflect.ValueOf(value)
when value != nil and set ok = false if the reflected kind is one of
Ptr/Chan/Map/Slice/Func/UnsafePointer and IsNil() returns true, otherwise ok =
true; reference the newRetainedContext function and retainedContext.value/ok
fields when making the change.
In `@middleware/basicauth/basicauth.go`:
- Around line 119-121: The UsernameFromContext tag is registered without
redaction causing plaintext usernames to be logged; update
registerLogContextTags to register the tag with partial masking by importing
internal/redact and calling logger.RegisterContextTag("username",
redact.Prefix(UsernameFromContext)) so it matches the masking behaviour used for
"api-key" and "session-id"; alternatively, if plaintext usernames are
intentionally required, add a clear comment above registerLogContextTags and
document the decision in the middleware README/godoc to make the opt-in
explicit.
In `@middleware/csrf/csrf_test.go`:
- Around line 658-674: Test_CSRFLogContextTagRedactsToken currently avoids
parallelization due to CaptureContextLog mutating global logger state; ensure
CaptureContextLog registers a t.Cleanup that fully restores the package-global
logger and context format so the mutation is reverted after the test, then add
t.Parallel() at the start of Test_CSRFLogContextTagRedactsToken to allow it to
run in parallel (if CaptureContextLog already performs the restore, simply add
t.Parallel() to the test); reference the test function
Test_CSRFLogContextTagRedactsToken and the helper loggertest.CaptureContextLog
when making the changes.
In `@middleware/csrf/csrf.go`:
- Around line 230-237: The csrf log tag currently returns the package-local
redactedKey ("[redacted]") which is inconsistent with other middleware; update
registerLogContextTags to import internal/redact and return
redact.Prefix(TokenFromContext(ctx)) (the empty-string case is handled by
redact.Prefix) instead of redactedKey, and update csrf_test.go assertions to
expect the redact.Mask suffix or otherwise validate the masked prefix rather
than the literal redactedKey.
---
Outside diff comments:
In `@middleware/logger/logger_test.go`:
- Around line 518-531: Benchmark_Logger and Benchmark_Logger_Parallel still
include the removed unknown tag `${non}` and will panic due to the eager
unknown-tag validation; update their logger format setup to match
Test_Logger_All by removing `${non}` (or otherwise replace it with a valid tag)
when constructing the Config.Format string used in Benchmark_Logger and
Benchmark_Logger_Parallel so the formats are consistent with Test_Logger_All and
no unknown tags are present.
---
Nitpick comments:
In `@middleware/logger/tags.go`:
- Around line 59-87: Tests that call RegisterTag mutate the global
registeredTags map without cleanup; add an unexported helper (e.g.,
resetRegisteredTagsForTest) that acquires registeredTags.Lock(), clears or
reinitializes registeredTags.m, and releases the lock, then update each test
that calls RegisterTag (like Test_Logger_RegisteredTag and
Test_Logger_CustomTagOverridesRegisteredTag) to call
t.Cleanup(resetRegisteredTagsForTest) so the global registry is reset after each
test run.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eda10448-2426-4506-877c-fd2a2e33af7f
📒 Files selected for processing (31)
docs/api/log.mddocs/middleware/logger.mddocs/whats_new.mdinternal/loggertest/loggertest.gointernal/logtemplate/errors.gointernal/logtemplate/template.gointernal/logtemplate/template_test.gointernal/redact/redact.gointernal/redact/redact_test.golog/context.golog/context_test.golog/default.golog/default_test.gomiddleware/basicauth/basicauth.gomiddleware/basicauth/basicauth_test.gomiddleware/csrf/csrf.gomiddleware/csrf/csrf_test.gomiddleware/keyauth/keyauth.gomiddleware/keyauth/keyauth_test.gomiddleware/logger/config.gomiddleware/logger/context_tag.gomiddleware/logger/default_logger.gomiddleware/logger/errors.gomiddleware/logger/errors_test.gomiddleware/logger/logger.gomiddleware/logger/logger_test.gomiddleware/logger/tags.gomiddleware/requestid/requestid.gomiddleware/requestid/requestid_test.gomiddleware/session/middleware.gomiddleware/session/middleware_test.go
✅ Files skipped from review due to trivial changes (2)
- middleware/basicauth/basicauth_test.go
- middleware/logger/config.go
🚧 Files skipped from review as they are similar to previous changes (3)
- middleware/logger/default_logger.go
- docs/middleware/logger.md
- docs/api/log.md
CI runs the test suite with -shuffle=on, which exposed two latent bugs in
the review-driven changes:
- log/context_test.go Test_ContextTemplate_ConcurrentRegistration relied on
a sibling test (Test_RegisterContextTagThenSetFormat) having already
registered the "tenant" tag. Without that ordering it fails immediately
with "logtemplate: unknown tag: tenant". Register the tag inline so the
test stands alone.
- middleware/logger Benchmark_Logger/WithAllTags carried a legacy ${non}
placeholder that the old permissive renderer silently passed through.
The strict logtemplate.Build path now panics on unknown tags at New()
time (the intended behavior — Test_Logger_UnknownTagPanicsWithTypedError
covers it). Drop ${non} from the benchmark format so it exercises only
registered tags.
Verified locally with go test -shuffle=on and go test -bench=Benchmark_Logger.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under -shuffle=on the test could run before any sibling test that constructs a fresh defaultLogger, in which case logger.stdlog is the zero value and SetOutput dereferences a nil *log.Logger. Calling initDefaultLogger up front (in addition to scheduling it as a Cleanup) matches the pattern used by every other test in this file that needs to capture default-logger output, and stabilises the run across shuffle seeds. Verified locally with five different -shuffle seeds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address four substantive review findings on the contextual-logs work.
1. log/default.go writeContext error sanitization (Copilot)
The render-error marker piped err.Error() through fmt.Fprintf, which
could carry CR/LF/ANSI bytes from a tag wrapping attacker-controlled
data. Route the message through writeSanitizedString — the same
sanitiser ${value:KEY} already uses — so a misconfigured tag cannot
become a log-injection vector. Test_WithContextRenderError now embeds
CR/LF in the error and asserts they are stripped.
2. docs/api/log.md KeyValueFormat consistency (Copilot)
Both the Go-block snippet and the Context Formats table referenced
the bare ${requestid} tag while the actual KeyValueFormat constant
uses the dashed ${request-id} alias. Align both to the constant.
3. log/default.go newRetainedContext typed-nil handling (CodeRabbit)
`value != nil` does not catch typed nils such as
`(*fasthttp.RequestCtx)(nil)` stored in an `any`; the interface
header carries a non-nil type descriptor and the renderer would
dereference a nil receiver. Use reflect.Value.IsNil() on the
nilable kinds (Chan, Func, Interface, Map, Pointer, Slice) to
collapse typed nils to retainedContext{}. Reflection runs once per
WithContext call (not per log line), so the cost is irrelevant to
the hot path. Test_NewRetainedContext_TypedNil locks in coverage
for untyped nil, typed-nil pointer/map/slice/chan, and non-nil
variants of the same.
4. middleware/csrf csrf-token redaction shape (CodeRabbit)
Switch the csrf-token tag from the package-local "[redacted]"
constant to redact.Prefix(token), aligning the masking shape with
keyauth and session (4-byte clear prefix + "****"). Log consumers
now see a uniform redaction format across every security-sensitive
tag the framework registers. csrf_test assertions move from exact
match against redactedKey to shape-based assertions on the
redact.Mask suffix. The package-internal redactedKey constant is
retained for storage-manager redaction (unchanged behavior).
Also document the intentional-by-design decisions:
- middleware/basicauth basicauth.go: ${username} is written in clear
text for audit-log use cases. Add a godoc block on
registerLogContextTags pointing developers to UsernameFromContext if
full usernames count as PII in their jurisdiction. Mirror the
callout in docs/api/log.md.
Verified locally with golangci-lint v2.6.0 (CI-matching), modernize@latest,
and `go test -race -shuffle=on` on the affected packages.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
/gemini review |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/logtemplate/template.go`:
- Around line 121-126: The loop in ExecuteChains accesses fixedParts[i] for each
element of funcChain without verifying lengths, which can panic; add a guard
(either a pre-check before the loop or an in-loop check) to ensure i <
len(fixedParts) before reading fixedParts[i] and return a descriptive error
(e.g., "template chain length mismatch") instead of allowing a panic; update
ExecuteChains to validate alignment of fixedParts and funcChain (referencing
ExecuteChains, fixedParts, funcChain, and output.Write) and fail fast when the
lengths are incompatible.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 879eec51-93dc-4a4a-b50c-763f7d4ff351
📒 Files selected for processing (12)
docs/api/log.mdinternal/logtemplate/template.gointernal/logtemplate/template_test.golog/context.golog/context_test.golog/default.golog/default_test.gomiddleware/basicauth/basicauth.gomiddleware/csrf/csrf.gomiddleware/csrf/csrf_test.gomiddleware/logger/errors_test.gomiddleware/logger/logger_test.go
✅ Files skipped from review due to trivial changes (5)
- internal/logtemplate/template_test.go
- log/context_test.go
- docs/api/log.md
- middleware/basicauth/basicauth.go
- log/context.go
🚧 Files skipped from review as they are similar to previous changes (4)
- middleware/logger/errors_test.go
- log/default.go
- middleware/logger/logger_test.go
- log/default_test.go
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a unified logging template and tag mechanism across Fiber's application logs and access logs. Key changes include updating the WithContext signature to accept any (allowing fiber.Ctx or context.Context), implementing a shared internal logtemplate engine for precompiled log formats, and enabling automatic tag registration for built-in middlewares like requestid, basicauth, and session. The documentation has been extensively updated to reflect these new capabilities and the breaking change in the logger interface. I have no feedback to provide as there were no review comments to assess.
gaby
reviewed
May 4, 2026
The predefined-formats table advertised a non-JSON shape
(`{time: ${time}, ...}`) while the runtime constant in
middleware/logger/format.go emits valid JSON with quoted keys
and string values. Sync the docs row to the real format.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@efectn can you check this |
gaby
approved these changes
May 5, 2026
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
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.
Replaces the approach from #4106 and fixes #3763.
This keeps the original goal of making
log.WithContext(...)useful for request-scoped application logs, but changes the design from automatic middleware extractors to an explicit, user-controlled template system similar tomiddleware/logger.logtemplatepackage.middleware/loggerwithout duplicating template parsing logic.logger.Data.TemplateChainandlogger.Data.LogFuncChaincompatible for existing custom logger integrations.log.SetContextTemplateso Fiber’s default logger can render contextual fields forlog.WithContext.WithContext(ctx any)compatible with Fiber-supported context inputs such asfiber.Ctx,*fasthttp.RequestCtx-styleUserValuecontexts, andcontext.Context.${value:key}support for ordinary context-style values.${value:key}rendering.Validation
make lintmake test