Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/shared/pkg/logger/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func GRPCLogger(l Logger) logging.Logger {
}
f = f[:fieldsCount]

logger := l.WithOptions(zap.AddCallerSkip(1)).With(f...)
logger := l.WithOptions(zap.WithCaller(false)).With(f...)

methodFullName := fmt.Sprintf("%s/%s/%s",
methodFullNameMap["grpc.service"],
Expand Down
13 changes: 10 additions & 3 deletions packages/shared/pkg/logger/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,27 @@ type TracedLogger struct {
innerLogger *zap.Logger
}

func withTraceLoggerOptions(innerLogger *zap.Logger) *zap.Logger {
return innerLogger.WithOptions(
zap.AddCaller(),
zap.AddCallerSkip(1),
)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detach() leaks AddCallerSkip(1) to direct logger consumers

Low Severity

withTraceLoggerOptions embeds AddCallerSkip(1) into the inner *zap.Logger, which is designed to skip the TracedLogger wrapper. However, Detach() exposes this inner logger directly. Callers that use the detached logger without a TracedLogger wrapper (e.g., zapio.Writer in process.go) will have their reported caller shifted by one extra frame, since there's no wrapper frame to skip. Callers using .Core() are unaffected since core extraction discards options.

Additional Locations (1)
Fix in Cursor Fix in Web


func NewTracedLoggerFromCore(core zapcore.Core) Logger {
return &TracedLogger{innerLogger: zap.New(core)} //nolint:forbidigo // zap.New is used to create a new logger from a core
return &TracedLogger{innerLogger: withTraceLoggerOptions(zap.New(core))} //nolint:forbidigo // zap.New is used to create a new logger from a core
}

func NewTracedLogger(innerLogger *zap.Logger) Logger {
return &TracedLogger{innerLogger: innerLogger}
return &TracedLogger{innerLogger: withTraceLoggerOptions(innerLogger)}
}

func L() *TracedLogger {
return &TracedLogger{innerLogger: zap.L()} //nolint:forbidigo // zap.L is used to get the global logger
}

func NewNopLogger() *TracedLogger {
return &TracedLogger{innerLogger: zap.NewNop()} //nolint:forbidigo // zap.NewNop is used to create a new nop logger
return &TracedLogger{innerLogger: withTraceLoggerOptions(zap.NewNop())} //nolint:forbidigo // zap.NewNop is used to create a new nop logger
}

func NewDevelopmentLogger() (Logger, error) {
Expand Down
58 changes: 58 additions & 0 deletions packages/shared/pkg/logger/logger_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package logger

import (
"path/filepath"
"testing"

"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
)

func TestTracedLoggerCallerSkipsWrapper(t *testing.T) {
t.Parallel()

core, logs := observer.New(zap.InfoLevel)
logger := NewTracedLogger(zap.New(core)) //nolint:forbidigo // test needs a raw zap.Logger with observable core

logger.Warn(t.Context(), "wrapper skip test")

entries := logs.All()
if len(entries) != 1 {
t.Fatalf("expected 1 log entry, got %d", len(entries))
}

entry := entries[0]
if got := filepath.Base(entry.Caller.File); got != "logger_test.go" {
t.Fatalf("expected caller file logger_test.go, got %q", entry.Caller.File)
}
if entry.Caller.Line == 0 {
t.Fatal("expected caller line to be set")
}
}

func TestLAfterReplaceGlobalsCallerIsCorrect(t *testing.T) {
t.Parallel()

core, logs := observer.New(zap.InfoLevel)
logger := NewTracedLogger(zap.New(core)) //nolint:forbidigo // test needs a raw zap.Logger with observable core

undo := ReplaceGlobals(t.Context(), logger)
defer undo()

// L() wraps zap.L() which already has AddCallerSkip(1) from ReplaceGlobals.
// This test verifies L() does not double-apply the skip.
L().Info(t.Context(), "global logger test")

entries := logs.All()
if len(entries) != 1 {
t.Fatalf("expected 1 log entry, got %d", len(entries))
}

entry := entries[0]
if got := filepath.Base(entry.Caller.File); got != "logger_test.go" {
t.Fatalf("expected caller file logger_test.go, got %q (double AddCallerSkip?)", entry.Caller.File)
}
if entry.Caller.Line == 0 {
t.Fatal("expected caller line to be set")
}
}
Loading