Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds two wide-event logging implementations (log2 and log3) with event lifecycle, sampling, and HTTP middleware; introduces demo programs; tweaks dev tooling and trace-id middleware imports; minor lint and test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as HTTP Middleware
participant Handler as App Handler
participant Event as Event
participant Sampler as Sampler
participant Logger as Logger
Client->>Middleware: HTTP Request
activate Middleware
Middleware->>Event: Start(ctx, "http_request")
Middleware->>Handler: call next handler (ctx contains event)
activate Handler
Handler-->>Middleware: Response (status, bytes)
deactivate Handler
Middleware->>Event: Add(request attrs, response attrs)
Middleware->>Event: Finish()
activate Event
Event->>Sampler: ShouldSample(EventView)
activate Sampler
Sampler-->>Event: SamplingDecision{Keep/Drop, Reason}
deactivate Sampler
alt Keep
Event->>Logger: emit(eventPayload)
activate Logger
Logger->>Logger: Build slog.Record and write
deactivate Logger
end
Event-->>Middleware: done
Middleware-->>Client: Final Response
deactivate Middleware
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Pull Request Test Coverage Report for Build 22071736233Details
💛 - 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: 6
🤖 Fix all issues with AI agents
In `@log2/logger.go`:
- Around line 29-34: The handler creation never applies the requested
ReplaceAttr to remove the default time attribute; update both
slog.NewTextHandler and slog.NewJSONHandler calls to pass a HandlerOptions that
sets ReplaceAttr (in addition to Level) and implement ReplaceAttr to filter out
attributes where a.Key == slog.TimeKey (returning the zero/empty Attr to drop
it), using the existing symbols handler, slog.NewTextHandler,
slog.NewJSONHandler, slog.TimeKey and slog.HandlerOptions so the log output no
longer contains the default time field.
In `@log3/event.go`:
- Around line 64-74: The AddError method on Event may dereference a nil error
because it calls err.Error(); update Event.AddError to guard against nil: check
if err == nil and either return early (no-op) or record a sentinel string (e.g.,
"<nil error>") instead of calling err.Error(); ensure this logic is applied
before calling e.setLevelNoLock or appending to e.errors (referencing
Event.AddError, e.mu, setLevelNoLock, and the errorRecord slice).
- Around line 1-8: Add a package-level doc comment at the top and add Go doc
comments for each exported symbol (Event, NewEvent, SetLevel, AddAttrs, AddStep,
AddError, Finish, ToAttrs) describing purpose and usage to satisfy linters;
place a short package comment immediately above "package log3" and add brief
sentences starting with the symbol name above each exported type or function
(e.g., "Event ...", "NewEvent ..." etc.), keeping comments idiomatic and
complete sentences to resolve the missing doc comment lint failures.
In `@log3/logger.go`:
- Around line 11-22: The Logger struct declares an unused w io.Writer field
which is not set in NewWideEventLogger; either remove the w field from the
Logger type or assign it when constructing a new Logger. Update the Logger
struct or the constructor accordingly: for the former, delete the w field; for
the latter, set w: w in the return value of NewWideEventLogger so the writer is
stored alongside sampler and logger (referencing Logger and NewWideEventLogger).
In `@log3/sampler.go`:
- Around line 42-54: The HTTP status comparison erroneously treats a missing or
zero status as matching when s.keepHttpStatusAtLeast is 0; change the check so
you only compare when a non-zero status was actually present: after extracting
statusFromMap and casting into httpStatus (using e.attrs["request.status"] and
statusFromMap.(int)), ensure you verify that exists is true and httpStatus != 0
before evaluating httpStatus >= s.keepHttpStatusAtLeast; treat httpStatus == 0
as "no status" and skip/return false for this branch so absent statuses are not
sampled when the threshold is 0.
- Around line 56-58: The sampling branch in sampler.go is inverted: change the
condition that uses rand.Float64() so we keep events with probability
s.randomKeepRate rather than keep ~1-randomKeepRate; replace the
`s.randomKeepRate <= rand.Float64()` check with the correct comparison (e.g. use
`s.randomFloat() < s.randomKeepRate` or `rand.Float64() < s.randomKeepRate`) in
the method that decides to drop/keep events (referencing s.randomKeepRate and
the current rand.Float64() usage), and if you keep math/rand for probabilistic
sampling add a `//nolint:gosec` with a short justification comment.
🧹 Nitpick comments (8)
Taskfile.dist.yml (1)
27-27: Switching to Bun requires Bun to be installed.This change introduces an implicit dependency on the Bun runtime. Ensure team members and CI environments have Bun installed, or document this requirement in the project's setup instructions.
docs/package.json (1)
6-7: Inconsistency betweendevandstartscripts.The
devscript now binds to all interfaces with--host, whilestartstill runsastro devwithout it. Consider aligning them or documenting when to use each:
dev→ external access (network-accessible)start→ local-only (localhost)If the distinction is intentional, this is fine. Otherwise, consider renaming or consolidating.
log3/event.go (2)
76-81: Duration is calculated in bothFinish()andToAttrs().Both methods compute
e.duration = time.Since(e.timestamp). IfFinish()is called beforeToAttrs(), the duration is overwritten. IfToAttrs()is called withoutFinish(), it still works. This creates ambiguity about the intended usage pattern.Consider either:
- Only compute duration in
Finish()and haveToAttrs()use the stored value- Remove
Finish()ifToAttrs()is always expected to finalize the eventAlso applies to: 83-87
120-126:stepRecord.LogValue()appears unused.The
LogValue()method implementsslog.LogValuer, butToAttrs()manually constructsmap[string]anyslices instead of using this method. Either remove the unused method or refactorToAttrs()to leverage it.log2/config.go (1)
29-42:ContextKeysis not initialized inDefaultConfig().The
Config.ContextKeysfield is not set, so it defaults tonil. If downstream code (e.g.,log.New()in the log package) expects a non-nil map, this could cause issues. Consider initializing it to an empty map if a nil map could cause problems:func DefaultConfig() Config { return Config{ Writer: os.Stdout, Format: defaultFormat, Level: slog.LevelInfo, + ContextKeys: map[string]any{}, Sampler: NewDefaultTailSampler(DefaultTailSamplerConfig{If
nilis explicitly acceptable downstream, this is fine as-is.log3/logger.go (1)
24-30: Consider checking for nil sampler.If
l.sampleris nil (e.g., ifNewWideEventLoggeris called with a nil sampler), line 27 will panic. Consider adding a nil check or defaulting to always-sample behavior:func (l *Logger) WriteEvent(ctx context.Context, e *Event) { e.Finish() - if l.sampler.ShouldSample(ctx, e) { + if l.sampler == nil || l.sampler.ShouldSample(ctx, e) { l.logger.LogAttrs(ctx, e.level, "", e.ToAttrs()...) } }log2/middleware.go (1)
83-99:statusRecordermay break streaming, WebSocket, and SSE handlers.Wrapping
http.ResponseWriterwithout implementing optional interfaces (http.Flusher,http.Hijacker,http.CloseNotifier,http.Pusher) causes type assertions in downstream handlers to fail. For example, SSE handlers callingw.(http.Flusher).Flush()will panic.♻️ Proposed fix to support http.Flusher
type statusRecorder struct { http.ResponseWriter status int bytes int } +func (r *statusRecorder) Flush() { + if f, ok := r.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +func (r *statusRecorder) Unwrap() http.ResponseWriter { + return r.ResponseWriter +}Consider also implementing
http.Hijackerif WebSocket support is needed, or use a library likehttpsnoopthat handles this transparently.log2/event.go (1)
140-147: UsetoIntfor consistent type coercion.Status extraction uses a direct
.(int)type assertion, buttoInt(defined at lines 293-335) handlesint,int8-64,uint,uint8-64,float32/64, andstring. UsingtoInthere would be more robust ifrequest.statusis set from JSON unmarshaling (oftenfloat64) or other sources.♻️ Proposed fix
status := 0 - statusFromAttrs, ok := e.attrs["request.status"] - if ok { - status, ok = statusFromAttrs.(int) - if !ok { - status = 0 - } + if statusFromAttrs, ok := e.attrs["request.status"]; ok { + status, _ = toInt(statusFromAttrs) }
| var handler slog.Handler | ||
| if cfg.Format == "text" { | ||
| handler = slog.NewTextHandler(writer, &slog.HandlerOptions{Level: cfg.Level}) | ||
| } else { | ||
| handler = slog.NewJSONHandler(writer, &slog.HandlerOptions{Level: cfg.Level}) | ||
| } |
There was a problem hiding this comment.
PR objective not implemented: slog.TimeKey not removed.
The PR description explicitly requests removing the default time attribute from log output using ReplaceAttr, but the handler options only set Level. The suggested code from the PR description should be applied here.
🐛 Proposed fix per PR objectives
+ opts := &slog.HandlerOptions{
+ Level: cfg.Level,
+ ReplaceAttr: func(_ []string, a slog.Attr) slog.Attr {
+ if a.Key == slog.TimeKey {
+ return slog.Attr{} // remove default slog "time"
+ }
+ return a
+ },
+ }
+
var handler slog.Handler
if cfg.Format == "text" {
- handler = slog.NewTextHandler(writer, &slog.HandlerOptions{Level: cfg.Level})
+ handler = slog.NewTextHandler(writer, opts)
} else {
- handler = slog.NewJSONHandler(writer, &slog.HandlerOptions{Level: cfg.Level})
+ handler = slog.NewJSONHandler(writer, opts)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var handler slog.Handler | |
| if cfg.Format == "text" { | |
| handler = slog.NewTextHandler(writer, &slog.HandlerOptions{Level: cfg.Level}) | |
| } else { | |
| handler = slog.NewJSONHandler(writer, &slog.HandlerOptions{Level: cfg.Level}) | |
| } | |
| opts := &slog.HandlerOptions{ | |
| Level: cfg.Level, | |
| ReplaceAttr: func(_ []string, a slog.Attr) slog.Attr { | |
| if a.Key == slog.TimeKey { | |
| return slog.Attr{} // remove default slog "time" | |
| } | |
| return a | |
| }, | |
| } | |
| var handler slog.Handler | |
| if cfg.Format == "text" { | |
| handler = slog.NewTextHandler(writer, opts) | |
| } else { | |
| handler = slog.NewJSONHandler(writer, opts) | |
| } |
🤖 Prompt for AI Agents
In `@log2/logger.go` around lines 29 - 34, The handler creation never applies the
requested ReplaceAttr to remove the default time attribute; update both
slog.NewTextHandler and slog.NewJSONHandler calls to pass a HandlerOptions that
sets ReplaceAttr (in addition to Level) and implement ReplaceAttr to filter out
attributes where a.Key == slog.TimeKey (returning the zero/empty Attr to drop
it), using the existing symbols handler, slog.NewTextHandler,
slog.NewJSONHandler, slog.TimeKey and slog.HandlerOptions so the log output no
longer contains the default time field.
| package log3 | ||
|
|
||
| import ( | ||
| "log/slog" | ||
| "maps" | ||
| "sync" | ||
| "time" | ||
| ) |
There was a problem hiding this comment.
Add missing package and export documentation to fix lint failures.
Static analysis reports missing doc comments on the package and all exported symbols. Add documentation to resolve these lint failures:
Suggested documentation additions
+// Package log3 provides wide-event logging with sampling support.
package log3Then add doc comments above each exported symbol (Event, NewEvent, SetLevel, AddAttrs, AddStep, AddError, Finish, ToAttrs).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package log3 | |
| import ( | |
| "log/slog" | |
| "maps" | |
| "sync" | |
| "time" | |
| ) | |
| // Package log3 provides wide-event logging with sampling support. | |
| package log3 | |
| import ( | |
| "log/slog" | |
| "maps" | |
| "sync" | |
| "time" | |
| ) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 1-1:
package-comments: should have a package comment (revive)
🤖 Prompt for AI Agents
In `@log3/event.go` around lines 1 - 8, Add a package-level doc comment at the top
and add Go doc comments for each exported symbol (Event, NewEvent, SetLevel,
AddAttrs, AddStep, AddError, Finish, ToAttrs) describing purpose and usage to
satisfy linters; place a short package comment immediately above "package log3"
and add brief sentences starting with the symbol name above each exported type
or function (e.g., "Event ...", "NewEvent ..." etc.), keeping comments idiomatic
and complete sentences to resolve the missing doc comment lint failures.
| func (e *Event) AddError(err error) { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
|
|
||
| e.setLevelNoLock(slog.LevelError) | ||
|
|
||
| e.errors = append(e.errors, errorRecord{ | ||
| Timestamp: time.Now(), | ||
| Error: err.Error(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference if err is nil.
AddError calls err.Error() without checking if err is nil. This will panic if a caller passes a nil error.
Suggested fix
func (e *Event) AddError(err error) {
+ if err == nil {
+ return
+ }
+
e.mu.Lock()
defer e.mu.Unlock()
e.setLevelNoLock(slog.LevelError)
e.errors = append(e.errors, errorRecord{
Timestamp: time.Now(),
Error: err.Error(),
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (e *Event) AddError(err error) { | |
| e.mu.Lock() | |
| defer e.mu.Unlock() | |
| e.setLevelNoLock(slog.LevelError) | |
| e.errors = append(e.errors, errorRecord{ | |
| Timestamp: time.Now(), | |
| Error: err.Error(), | |
| }) | |
| } | |
| func (e *Event) AddError(err error) { | |
| if err == nil { | |
| return | |
| } | |
| e.mu.Lock() | |
| defer e.mu.Unlock() | |
| e.setLevelNoLock(slog.LevelError) | |
| e.errors = append(e.errors, errorRecord{ | |
| Timestamp: time.Now(), | |
| Error: err.Error(), | |
| }) | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 64-64:
exported: exported method Event.AddError should have comment or be unexported (revive)
🤖 Prompt for AI Agents
In `@log3/event.go` around lines 64 - 74, The AddError method on Event may
dereference a nil error because it calls err.Error(); update Event.AddError to
guard against nil: check if err == nil and either return early (no-op) or record
a sentinel string (e.g., "<nil error>") instead of calling err.Error(); ensure
this logic is applied before calling e.setLevelNoLock or appending to e.errors
(referencing Event.AddError, e.mu, setLevelNoLock, and the errorRecord slice).
| type Logger struct { | ||
| w io.Writer | ||
| sampler Sampler | ||
| logger *slog.Logger | ||
| } | ||
|
|
||
| func NewWideEventLogger(w io.Writer, s Sampler, loggerType string, contextKeys map[string]any) *Logger { | ||
| return &Logger{ | ||
| sampler: s, | ||
| logger: log.New(w, loggerType, slog.LevelDebug, contextKeys), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unused field w in Logger struct.
The w io.Writer field (line 12) is declared but never assigned in NewWideEventLogger. The writer is passed directly to log.New() but not stored. Either remove the unused field or store it if needed for future use.
Suggested fix
type Logger struct {
- w io.Writer
sampler Sampler
logger *slog.Logger
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Logger struct { | |
| w io.Writer | |
| sampler Sampler | |
| logger *slog.Logger | |
| } | |
| func NewWideEventLogger(w io.Writer, s Sampler, loggerType string, contextKeys map[string]any) *Logger { | |
| return &Logger{ | |
| sampler: s, | |
| logger: log.New(w, loggerType, slog.LevelDebug, contextKeys), | |
| } | |
| } | |
| type Logger struct { | |
| sampler Sampler | |
| logger *slog.Logger | |
| } | |
| func NewWideEventLogger(w io.Writer, s Sampler, loggerType string, contextKeys map[string]any) *Logger { | |
| return &Logger{ | |
| sampler: s, | |
| logger: log.New(w, loggerType, slog.LevelDebug, contextKeys), | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@log3/logger.go` around lines 11 - 22, The Logger struct declares an unused w
io.Writer field which is not set in NewWideEventLogger; either remove the w
field from the Logger type or assign it when constructing a new Logger. Update
the Logger struct or the constructor accordingly: for the former, delete the w
field; for the latter, set w: w in the return value of NewWideEventLogger so the
writer is stored alongside sampler and logger (referencing Logger and
NewWideEventLogger).
| httpStatus := 0 | ||
| statusFromMap, exists := e.attrs["request.status"] | ||
| if exists { | ||
| success := false | ||
| httpStatus, success = statusFromMap.(int) | ||
| if !success { | ||
| httpStatus = 0 | ||
| } | ||
| } | ||
|
|
||
| if httpStatus >= s.keepHttpStatusAtLeast { | ||
| return true | ||
| } |
There was a problem hiding this comment.
HTTP status check triggers on status 0 when keepHttpStatusAtLeast is 0.
When keepHttpStatusAtLeast is set to 0 (or left uninitialized), and no request.status attribute exists (causing httpStatus to remain 0), the condition httpStatus >= s.keepHttpStatusAtLeast evaluates to 0 >= 0, which is true—forcing every event without an HTTP status to be sampled.
Consider whether status 0 should be treated as "no status" and excluded from the comparison, or require a minimum sensible threshold.
🤖 Prompt for AI Agents
In `@log3/sampler.go` around lines 42 - 54, The HTTP status comparison erroneously
treats a missing or zero status as matching when s.keepHttpStatusAtLeast is 0;
change the check so you only compare when a non-zero status was actually
present: after extracting statusFromMap and casting into httpStatus (using
e.attrs["request.status"] and statusFromMap.(int)), ensure you verify that
exists is true and httpStatus != 0 before evaluating httpStatus >=
s.keepHttpStatusAtLeast; treat httpStatus == 0 as "no status" and skip/return
false for this branch so absent statuses are not sampled when the threshold is
0.
| if s.randomKeepRate <= rand.Float64() { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Inverted random sampling logic.
The condition s.randomKeepRate <= rand.Float64() keeps events when the random value is greater than or equal to the keep rate. This is inverted—if randomKeepRate is 0.1 (10%), this keeps ~90% of events instead of 10%.
Compare with log2/sampling.go line 111 which correctly uses s.randomFloat() < s.randomKeepRate.
🐛 Proposed fix
- if s.randomKeepRate <= rand.Float64() {
+ if rand.Float64() < s.randomKeepRate {
return true
}Regarding the gosec G404 warning: math/rand/v2 is acceptable here since this is probabilistic sampling, not a security-sensitive operation. Consider adding a //nolint:gosec directive with justification if suppression is preferred over switching to crypto/rand.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if s.randomKeepRate <= rand.Float64() { | |
| return true | |
| } | |
| if rand.Float64() < s.randomKeepRate { | |
| return true | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 56-56: G404: Use of weak random number generator (math/rand) detected by gosec. Use crypto/rand instead.
🪛 GitHub Check: lint
[failure] 56-56:
G404: Use of weak random number generator (math/rand or math/rand/v2 instead of crypto/rand) (gosec)
🤖 Prompt for AI Agents
In `@log3/sampler.go` around lines 56 - 58, The sampling branch in sampler.go is
inverted: change the condition that uses rand.Float64() so we keep events with
probability s.randomKeepRate rather than keep ~1-randomKeepRate; replace the
`s.randomKeepRate <= rand.Float64()` check with the correct comparison (e.g. use
`s.randomFloat() < s.randomKeepRate` or `rand.Float64() < s.randomKeepRate`) in
the method that decides to drop/keep events (referencing s.randomKeepRate and
the current rand.Float64() usage), and if you keep math/rand for probabilistic
sampling add a `//nolint:gosec` with a short justification comment.
2 participants
wide-eventsusesslogunder the hood, and the standardtimefield is added by the handler.Drop it with
HandlerOptions.ReplaceAttr:Then use
optsin both handlers:Summary by CodeRabbit
New Features
Chores