Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Pull Request Test Coverage Report for Build 22088253765Details
💛 - Coveralls |
![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
🧹 Nitpick comments (6)
log/event.go (2)
183-195: Consider using a set for reserved key lookup.
slices.Containsis O(n) per key. For events with many custom attributes, this could add overhead. Amap[string]struct{}would provide O(1) lookups.♻️ Proposed optimization
- customAttrKeys := make([]string, 0, len(e.attrs)) - for key := range e.attrs { - if slices.Contains(reservedAttrKeys, key) { - continue - } - - customAttrKeys = append(customAttrKeys, key) - } + reservedSet := make(map[string]struct{}, len(reservedAttrKeys)) + for _, k := range reservedAttrKeys { + reservedSet[k] = struct{}{} + } + + customAttrKeys := make([]string, 0, len(e.attrs)) + for key := range e.attrs { + if _, reserved := reservedSet[key]; reserved { + continue + } + customAttrKeys = append(customAttrKeys, key) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/event.go` around lines 183 - 195, The loop building customAttrKeys uses slices.Contains against reservedAttrKeys causing O(n) checks per attribute; convert reservedAttrKeys into a map[string]struct{} (e.g., reservedAttrSet) once before the loop and replace slices.Contains(reservedAttrKeys, key) with a constant-time lookup like "if _, ok := reservedAttrSet[key]; ok { continue }", leaving the rest of the logic (customAttrKeys slice, sort.Strings(customAttrKeys), and appending attrs with slog.Any) unchanged and referenced by the same symbols (e.attrs, customAttrKeys, reservedAttrKeys, attrs, slog.Any).
200-209: JSON tags on internal record types are unused.The
stepRecordanderrorRecordtypes have JSON struct tags, buttoAttrs()manually constructsmap[string]anyrepresentations (lines 149-153, 158-161) rather than marshaling these structs. Consider removing the tags to avoid misleading future maintainers, or use the structs directly withslog.Any.Option: Use structs directly
- steps := make([]map[string]any, 0, len(e.steps)) - for _, step := range e.steps { - steps = append(steps, map[string]any{ - "timestamp": step.Timestamp, - "level": step.Level.String(), - "name": step.Name, - }) - } + steps := make([]stepRecord, 0, len(e.steps)) + for _, step := range e.steps { + steps = append(steps, step) + }Note: This would require adjusting the
Levelfield serialization sinceslog.Leveldoesn't serialize to string by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/event.go` around lines 200 - 209, The JSON struct tags on stepRecord and errorRecord are misleading because toAttrs() builds maps manually; either remove the `json:"..."` tags from the type definitions (stepRecord, errorRecord) to avoid confusion, or refactor to use the structs directly in toAttrs() by returning slog.Any("step", stepRecord{/*...*/}) / slog.Any("error", errorRecord{/*...*/}) and ensure slog-friendly serialization for stepRecord.Level (e.g., add a string field or implement a method to convert slog.Level to its string before attaching), updating the toAttrs() code paths that currently construct maps to use the chosen approach.demo-app/cmd/wide-events/main.go (1)
14-19: Consider adjustingkeepHTTPStatusAtLeastfor a more illustrative demo.The
keepHTTPStatusAtLeast: 200means all HTTP responses would be kept since 200 is the baseline success code. For a demo showcasing tail-sampling, a value like400or500(to keep only errors) would better illustrate the sampling behavior.Note: This doesn't affect the current demo since
WriteEventis called directly without HTTP context, but it may confuse readers trying to understand the sampler's purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo-app/cmd/wide-events/main.go` around lines 14 - 19, The sampler creation uses log.NewDefaultSampler with keepHTTPStatusAtLeast set to 200 which preserves all HTTP responses; update the NewDefaultSampler invocation to use a higher threshold (e.g., keepHTTPStatusAtLeast = 400 or 500) so the demo shows tail-sampling decisions (only error responses kept) — modify the call site where NewDefaultSampler is constructed (the logger := log.NewWideEventLogger(...) block) to pass the new threshold value.docs/package.json (1)
6-7: Minor inconsistency betweendevandstartscripts.The
devscript now includes--hostwhilestartdoes not. If both scripts serve similar purposes (local development), consider aligning them, or add a comment explaining the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/package.json` around lines 6 - 7, The package.json scripts show an inconsistency: "dev" uses the --host flag while "start" does not; decide whether both should behave the same and update accordingly—either add --host to the "start" script to match "dev" or document the difference with a comment in package.json (or README) clarifying that "dev" exposes the dev server on the network while "start" intentionally does not; update the "dev" and/or "start" entries to reflect the chosen behavior (scripts "dev" and "start").log/sampler.go (1)
29-35: Clamp or validaterandomKeepRateto [0,1].Out-of-range values silently change sampling behavior (always keep or always drop). Consider clamping or panicking on invalid configuration.
🧰 Suggested clamp
func NewDefaultSampler(slowThreshold time.Duration, keepHTTPStatusAtLeast int, randomKeepRate float64) *DefaultSampler { + if randomKeepRate < 0 { + randomKeepRate = 0 + } else if randomKeepRate > 1 { + randomKeepRate = 1 + } return &DefaultSampler{ slowThreshold: slowThreshold, keepHTTPStatusAtLeast: keepHTTPStatusAtLeast, randomKeepRate: randomKeepRate, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/sampler.go` around lines 29 - 35, NewDefaultSampler currently accepts randomKeepRate without bounds, letting out-of-range values alter sampling; modify NewDefaultSampler to validate and clamp randomKeepRate into [0,1] before storing it on the returned *DefaultSampler (e.g., if randomKeepRate < 0 set to 0, if > 1 set to 1) so sampling behavior is deterministic; reference the NewDefaultSampler function and the DefaultSampler.randomKeepRate field when making this change (use simple conditionals or math.Min/Max to clamp).log/wideevent_middleware.go (1)
80-103: Optional: Delegatehttp.ResponseWriteroptional interfaces for better middleware compatibility.While no handlers in the codebase currently use these interfaces, properly delegating
Flusher,Hijacker, andPusheris a best practice for response writer wrappers. This prevents potential issues if future handlers rely on streaming, SSE, or WebSocket support. The embeddedhttp.ResponseWriteralready provides these methods, but explicit delegation with type-checks ensures they work correctly when the underlying writer supports them.Consider adding:
Flush()delegation forhttp.FlusherHijack()delegation forhttp.HijackerPush()delegation forhttp.Pusher- Optional
Unwrap()method to expose the wrapped writer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/wideevent_middleware.go` around lines 80 - 103, The statusResponseWriter wrapper should explicitly delegate optional interfaces so wrapped handlers that expect streaming/SSE/WebSocket/HTTP/2 push still work: add methods on statusResponseWriter that type-assert the embedded ResponseWriter and forward calls — implement Flush() to call http.Flusher, Hijack() returning (net.Conn, *bufio.ReadWriter, error) to call http.Hijacker, Push(target string, opts *http.PushOptions) error to call http.Pusher, and an Unwrap() http.ResponseWriter method that returns the underlying ResponseWriter; each method should return appropriate errors if the underlying writer does not support the interface and otherwise call-through to the underlying implementation to preserve behavior for WriteHeader/Write.
🤖 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/wideevent_logger.go`:
- Around line 17-40: The constructor NewWideEventLogger may leave the sampler
field nil causing a panic in WriteEvent when calling l.sampler.ShouldSample;
ensure NewWideEventLogger checks if the provided Sampler s is nil and if so
assigns a default “keep-all” sampler implementation (or return an error/fail
fast) before storing it on the WideEventLogger, so WriteEvent can safely call
l.sampler.ShouldSample; update the sampler assignment in NewWideEventLogger (and
add a lightweight keep-all sampler type or factory if not present) and ensure
WideEventLogger.sampler is never nil.
In `@log/wideevent_middleware.go`:
- Around line 42-47: The middleware is currently logging raw client IPs via the
"request.remoteAddr" attribute in the NewEvent -> event.AddAttrs call, which
exposes PII; change this to either redact or pseudonymize the value before
adding it (e.g., replace with a masked string, hash/HMAC it using a server-side
secret, or omit it) and make the behavior controlled by a configuration flag
(e.g., EnablePIILogging or AllowRemoteAddrLogging) so it's opt-in; update the
code that sets "request.remoteAddr" in wideevent_middleware.go (where NewEvent
and event.AddAttrs are used) to perform the chosen redaction/hashing and respect
the config flag.
- Around line 23-36: NewWideEventMiddleware currently assigns a nil
WideEventLogger directly, which will cause a panic when WideEventMiddleware
methods call m.logger.WriteEvent; update NewWideEventMiddleware to guard against
a nil logger (parameter logger) and either return/fail-fast or substitute a
no-op logger instance. Specifically, inside NewWideEventMiddleware check if
logger == nil and set logger to a no-op WideEventLogger (implement a minimal
no-op writer) or return an error/panic as your API policy prefers so that
WideEventMiddleware (and calls to WriteEvent) never nil-dereference; reference
the constructor NewWideEventMiddleware, type WideEventMiddleware, method
WriteEvent, and symbols WideEventLogger, WideEventKey, defaultWideEventName when
making the change.
---
Nitpick comments:
In `@demo-app/cmd/wide-events/main.go`:
- Around line 14-19: The sampler creation uses log.NewDefaultSampler with
keepHTTPStatusAtLeast set to 200 which preserves all HTTP responses; update the
NewDefaultSampler invocation to use a higher threshold (e.g.,
keepHTTPStatusAtLeast = 400 or 500) so the demo shows tail-sampling decisions
(only error responses kept) — modify the call site where NewDefaultSampler is
constructed (the logger := log.NewWideEventLogger(...) block) to pass the new
threshold value.
In `@docs/package.json`:
- Around line 6-7: The package.json scripts show an inconsistency: "dev" uses
the --host flag while "start" does not; decide whether both should behave the
same and update accordingly—either add --host to the "start" script to match
"dev" or document the difference with a comment in package.json (or README)
clarifying that "dev" exposes the dev server on the network while "start"
intentionally does not; update the "dev" and/or "start" entries to reflect the
chosen behavior (scripts "dev" and "start").
In `@log/event.go`:
- Around line 183-195: The loop building customAttrKeys uses slices.Contains
against reservedAttrKeys causing O(n) checks per attribute; convert
reservedAttrKeys into a map[string]struct{} (e.g., reservedAttrSet) once before
the loop and replace slices.Contains(reservedAttrKeys, key) with a constant-time
lookup like "if _, ok := reservedAttrSet[key]; ok { continue }", leaving the
rest of the logic (customAttrKeys slice, sort.Strings(customAttrKeys), and
appending attrs with slog.Any) unchanged and referenced by the same symbols
(e.attrs, customAttrKeys, reservedAttrKeys, attrs, slog.Any).
- Around line 200-209: The JSON struct tags on stepRecord and errorRecord are
misleading because toAttrs() builds maps manually; either remove the
`json:"..."` tags from the type definitions (stepRecord, errorRecord) to avoid
confusion, or refactor to use the structs directly in toAttrs() by returning
slog.Any("step", stepRecord{/*...*/}) / slog.Any("error", errorRecord{/*...*/})
and ensure slog-friendly serialization for stepRecord.Level (e.g., add a string
field or implement a method to convert slog.Level to its string before
attaching), updating the toAttrs() code paths that currently construct maps to
use the chosen approach.
In `@log/sampler.go`:
- Around line 29-35: NewDefaultSampler currently accepts randomKeepRate without
bounds, letting out-of-range values alter sampling; modify NewDefaultSampler to
validate and clamp randomKeepRate into [0,1] before storing it on the returned
*DefaultSampler (e.g., if randomKeepRate < 0 set to 0, if > 1 set to 1) so
sampling behavior is deterministic; reference the NewDefaultSampler function and
the DefaultSampler.randomKeepRate field when making this change (use simple
conditionals or math.Min/Max to clamp).
In `@log/wideevent_middleware.go`:
- Around line 80-103: The statusResponseWriter wrapper should explicitly
delegate optional interfaces so wrapped handlers that expect
streaming/SSE/WebSocket/HTTP/2 push still work: add methods on
statusResponseWriter that type-assert the embedded ResponseWriter and forward
calls — implement Flush() to call http.Flusher, Hijack() returning (net.Conn,
*bufio.ReadWriter, error) to call http.Hijacker, Push(target string, opts
*http.PushOptions) error to call http.Pusher, and an Unwrap()
http.ResponseWriter method that returns the underlying ResponseWriter; each
method should return appropriate errors if the underlying writer does not
support the interface and otherwise call-through to the underlying
implementation to preserve behavior for WriteHeader/Write.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
log/event.go (1)
137-199: Optional: stabilize custom attribute order for deterministic logs/tests.Map iteration order is random; sorting keys yields consistent output.
♻️ Suggested tweak
customAttrKeys := make([]string, 0, len(e.attrs)) for key := range e.attrs { if _, reserved := reservedSet[key]; reserved { continue } customAttrKeys = append(customAttrKeys, key) } + slices.Sort(customAttrKeys) for _, key := range customAttrKeys { attrs = append(attrs, slog.Any(key, e.attrs[key])) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@log/event.go` around lines 137 - 199, The iteration over map keys in Event.toAttrs produces non-deterministic ordering; to stabilize logs/tests, sort the customAttrKeys slice before appending attributes. Inside the toAttrs method (which builds reservedAttrKeys, reservedSet and customAttrKeys from e.attrs and wideEventBuiltinAttrKeys), after populating customAttrKeys call sort.Strings(customAttrKeys) (importing sort) and then iterate that sorted slice when appending slog.Any entries for each key; no other logic changes are needed.
🤖 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/event.go`:
- Around line 48-54: Add a nil-guard and lazy init for the Event attrs map
inside the AddAttrs method: before calling maps.Copy, acquire the lock (already
done), check if e.attrs is nil and if so assign a new map (e.attrs =
make(map[string]any)), then call maps.Copy(e.attrs, attrs) only when attrs is
non-nil; keep using e.mu.Lock() / defer e.mu.Unlock() as currently implemented.
Ensure you reference Event.AddAttrs, e.attrs and maps.Copy when making the
change.
In `@log/wideevent_logger.go`:
- Around line 48-55: The WriteEvent method can panic when called with a nil
event because e.Finish() is invoked unguarded; update WideEventLogger.WriteEvent
to first check if e is nil and return early (no-op) if so, then call e.Finish()
and proceed to the existing sampling/logging logic (sampler.ShouldSample and
logger.LogAttrs) to prevent nil dereference. Ensure the nil check is done before
calling e.Finish() and references the WideEventLogger.WriteEvent, Event.Finish,
sampler.ShouldSample, and logger.LogAttrs symbols so the change is applied in
the right place.
---
Duplicate comments:
In `@log/wideevent_middleware.go`:
- Around line 45-84: The middleware currently logs raw r.RemoteAddr (PII);
update WideEventMiddleware.Wrap to avoid logging it directly by making redaction
opt-in and pseudonymizing when enabled: add a bool option/field like
RedactRemoteAddr on WideEventMiddleware (or use existing config), and when
building the attrs call use a local variable remote := r.RemoteAddr, strip the
port if present, then if m.RedactRemoteAddr compute a stable pseudonym (e.g.
hash the stripped IP) and set "request.remoteAddr" to that value else set it to
the original; keep the rest of the event.AddAttrs call and use the same
event.AddAttrs map to insert the redacted/pseudonymized value so consumers get
no raw PII unless the flag is explicitly false.
---
Nitpick comments:
In `@log/event.go`:
- Around line 137-199: The iteration over map keys in Event.toAttrs produces
non-deterministic ordering; to stabilize logs/tests, sort the customAttrKeys
slice before appending attributes. Inside the toAttrs method (which builds
reservedAttrKeys, reservedSet and customAttrKeys from e.attrs and
wideEventBuiltinAttrKeys), after populating customAttrKeys call
sort.Strings(customAttrKeys) (importing sort) and then iterate that sorted slice
when appending slog.Any entries for each key; no other logic changes are needed.
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 by CodeRabbit
New Features
Documentation
Chores