Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a wide-event logging subsystem (Event type, sampling, WideEventLogger, middleware), relocates TraceID middleware into the Changes
Sequence DiagramsequenceDiagram
actor Client
participant Middleware as WideEventMiddleware
participant Event as Event
participant Handler as NextHandler
participant Sampler as Sampler
participant Logger as WideEventLogger
Client->>Middleware: HTTP Request
Middleware->>Event: NewEvent("http.request") / AddAttrs(method,path,remote_addr)
Middleware->>Handler: Call next handler
Handler->>Handler: Process request
alt panic
Handler-->>Middleware: panic()
Middleware->>Event: AddError(panic) / set status 500 if needed
else normal
Handler-->>Middleware: returns (status)
Middleware->>Event: AddAttrs(request.status)
end
Middleware->>Event: Finish()
Middleware->>Sampler: ShouldSample(ctx, Event)
alt sample == true
Sampler-->>Logger: allow
Middleware->>Logger: WriteEvent(ctx, Event)
Logger->>Logger: emit via slog (JSON/Text)
else sample == false
Sampler-->>Middleware: drop
end
Middleware->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 22087197604Details
💛 - 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
🤖 Fix all issues with AI agents
In `@log/sampler.go`:
- Around line 39-57: DefaultSampler.ShouldSample currently reads Event fields
(e.errors, e.duration, e.attrs) without synchronization causing a data race; add
thread-safe getters on Event (e.g., HasErrors() bool, Duration() time.Duration,
Attr(key string) (any, bool)) that lock e.mu, return the requested values, and
then update DefaultSampler.ShouldSample to call e.HasErrors(), e.Duration(), and
e.Attr("request.status") instead of accessing fields directly so all reads are
protected by the Event mutex.
- Around line 55-57: The current check "if httpStatus >= s.keepHTTPStatusAtLeast
{ return true }" treats a zero httpStatus (absent) as a match when
keepHTTPStatusAtLeast is 0; update the guard in the sampler (the code
referencing httpStatus and s.keepHTTPStatusAtLeast) to explicitly treat 0 as
"absent" by requiring httpStatus > 0 before comparing (or alternatively document
the current behavior); change the condition to require httpStatus > 0 &&
httpStatus >= s.keepHTTPStatusAtLeast so absent statuses are not accidentally
sampled, and add a brief comment explaining the intent.
In `@log/wideevent_logger.go`:
- Around line 41-46: The WriteEvent method reads e.level unsafely after calling
e.Finish(); to fix, add a synchronized getter on Event (e.g., func (e *Event)
Level() slog.Level that locks e.mu, returns e.level) and replace direct access
to e.level in WideEventLogger.WriteEvent with e.Level(), or alternatively modify
Event.ToAttrs() to return the level alongside attrs and use that return value in
WideEventLogger.WriteEvent; update references in WriteEvent to use the new
getter/return value (Event.Level or the new ToAttrs signature) so no field is
accessed without the mutex.
🧹 Nitpick comments (4)
log/wideevent_middleware.go (2)
1-1: Static analysis: package name conflicts with Go stdliblog.The linter flags that
package logshadows the standard library package. This is a package-level design decision affecting all files. Users of this package will need to use import aliases (as seen inlog/traceid_test.gowithplatformalog).If renaming isn't desired, consider documenting the recommended import alias in package documentation.
80-103: ImplementUnwrap()method to expose underlyingResponseWritercapabilities.
statusResponseWriterwrapshttp.ResponseWriterbut does not implementUnwrap(). This breaks functionality for handlers usinghttp.Flusher(SSE),http.Hijacker(WebSockets), orhttp.Pusher(HTTP/2 push). AddingUnwrap()allowshttp.ResponseControllerto access the underlying writer's capabilities:type statusResponseWriter struct { http.ResponseWriter statusCode int wroteHeader bool } +// Unwrap returns the underlying ResponseWriter for http.ResponseController. +func (w *statusResponseWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +}log/event.go (2)
82-88: Duration calculated in bothFinish()andToAttrs()- intentional?
Finish()setse.duration = time.Since(e.timestamp), butToAttrs()recalculates it (line 95). This means:
- If
ToAttrs()is called withoutFinish(), it still works- If both are called, the duration reflects
ToAttrs()call time, notFinish()timeIf
Finish()is meant to snapshot the duration, line 95 should be removed or guarded.💡 Only calculate duration if not already set
func (e *Event) ToAttrs() []slog.Attr { e.mu.Lock() defer e.mu.Unlock() - e.duration = time.Since(e.timestamp) + if e.duration == 0 { + e.duration = time.Since(e.timestamp) + }Also applies to: 90-95
124-141: Unused code:stepRecord.LogValue()and JSON struct tags.
stepRecordimplementsLogValue()butToAttrs()manually converts steps to[]map[string]anyinstead of using it. The JSON struct tags on bothstepRecordanderrorRecordalso appear unused since serialization is handled manually.Consider either:
- Removing the unused
LogValue()method and JSON tags- Or refactoring
ToAttrs()to useslog.Any("steps", e.steps)which would invokeLogValue()
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.golangci.yml (1)
75-78: Consider using revive configuration instead of text-based exclusion for stronger stability.The
textmatcher relies on exact diagnostic wording, which can shift across golangci-lint or revive version updates. Instead, configure thevar-namingrule directly inlinters-settingsto disable the package-name collision check:linters-settings: revive: rules: - name: var-naming arguments: - [] - [] - skip-package-name-collision-with-go-std: trueThis targets the rule configuration directly (stable across versions) rather than matching prose output, and makes the intent clearer to future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@log/event.go`:
- Around line 69-80: Add a nil-check to avoid calling err.Error() when no error
is provided: in AddError, return early if err == nil before taking the mutex so
you don't escalate level or append an error record for a nil value. Keep using
e.setLevelNoLock and appending to e.errors/errorRecord only when err is non-nil
(call err.Error() after the nil guard).
🧹 Nitpick comments (2)
log/event.go (2)
116-148:ToAttrsmutates state unexpectedly.Line 121 sets
e.duration = time.Since(e.timestamp), which is a mutation inside what appears to be a read-only conversion method. This creates potential confusion:
- If
Finish()was already called,ToAttrs()will overwrite the recorded duration.- Calling
ToAttrs()multiple times yields different durations.- The method name suggests a pure conversion, not a state mutation.
Consider either removing the mutation (relying on
Finish()being called first) or renaming/documenting the side effect explicitly.♻️ Option A: Remove mutation, use existing duration
func (e *Event) ToAttrs() []slog.Attr { e.mu.Lock() defer e.mu.Unlock() - e.duration = time.Since(e.timestamp) + duration := e.duration + if duration == 0 { + duration = time.Since(e.timestamp) + } steps := make([]map[string]any, 0, len(e.steps)) // ... rest unchanged ... return []slog.Attr{ slog.String("name", e.name), slog.Time("timestamp", e.timestamp), - slog.Duration("duration", e.duration), + slog.Duration("duration", duration), slog.Any("attrs", e.attrs), slog.Any("steps", steps), slog.Any("errors", eventErrors), } }
150-167:stepRecord.LogValue()is unused.The
LogValue()method onstepRecordimplementsslog.LogValuer, butToAttrs()manually constructsmap[string]anyslices instead of using it. Either remove the unused method or refactorToAttrs()to leverage it for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@log/event.go`:
- Around line 124-155: To prevent concurrent map access panics, clone the
Event.attrs map inside Event.ToAttrs() while still holding e.mu and return the
cloned map in the slog.Attr instead of the original e.attrs; locate the ToAttrs
method on type Event and replace slog.Any("attrs", e.attrs) with a shallow copy
(new map[string]any populated from e.attrs) so that after unlocking the original
e.attrs can be mutated by AddAttrs without impacting the logged data consumed
asynchronously by LogAttrs.
2 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores