From bd81afe5a502bf0e2d03c30d0f5199a532cc4c62 Mon Sep 17 00:00:00 2001 From: Ernesto Alejandro Santana Hidalgo Date: Thu, 24 Apr 2025 10:22:49 +0200 Subject: [PATCH] log/slog: update Source method to return nil for unavailable locations Following proposal https://github.com/golang/go/issues/70280 Signed-off-by: Ernesto Alejandro Santana Hidalgo --- api/next/70280.txt | 1 + doc/next/6-stdlib/99-minor/log/slog/70280.md | 1 + src/log/slog/handler.go | 6 ++- src/log/slog/handler_test.go | 40 +++++++++++++++++++- src/log/slog/logger_test.go | 3 ++ src/log/slog/record.go | 11 ++++-- src/log/slog/record_test.go | 21 +++++++--- 7 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 api/next/70280.txt create mode 100644 doc/next/6-stdlib/99-minor/log/slog/70280.md diff --git a/api/next/70280.txt b/api/next/70280.txt new file mode 100644 index 0000000000..f2dd74af48 --- /dev/null +++ b/api/next/70280.txt @@ -0,0 +1 @@ +pkg log/slog, method (Record) Source() *Source #70280 diff --git a/doc/next/6-stdlib/99-minor/log/slog/70280.md b/doc/next/6-stdlib/99-minor/log/slog/70280.md new file mode 100644 index 0000000000..7f1b734d4f --- /dev/null +++ b/doc/next/6-stdlib/99-minor/log/slog/70280.md @@ -0,0 +1 @@ +[Record] now has a Source() method, returning its source location or nil if unavailable. diff --git a/src/log/slog/handler.go b/src/log/slog/handler.go index 27cc6d169c..e56be5f494 100644 --- a/src/log/slog/handler.go +++ b/src/log/slog/handler.go @@ -299,7 +299,11 @@ func (h *commonHandler) handle(r Record) error { } // source if h.opts.AddSource { - state.appendAttr(Any(SourceKey, r.Source())) + src := r.Source() + if src == nil { + src = &Source{} + } + state.appendAttr(Any(SourceKey, src)) } key = MessageKey msg := r.Message diff --git a/src/log/slog/handler_test.go b/src/log/slog/handler_test.go index ded177ae01..445f43f1f5 100644 --- a/src/log/slog/handler_test.go +++ b/src/log/slog/handler_test.go @@ -547,7 +547,11 @@ func TestJSONAndTextHandlers(t *testing.T) { }, } { r := NewRecord(testTime, LevelInfo, "message", callerPC(2)) - line := strconv.Itoa(r.Source().Line) + source := r.Source() + if source == nil { + t.Fatal("source is nil") + } + line := strconv.Itoa(source.Line) r.AddAttrs(test.attrs...) var buf bytes.Buffer opts := HandlerOptions{ReplaceAttr: test.replace, AddSource: test.addSource} @@ -634,6 +638,40 @@ func TestHandlerEnabled(t *testing.T) { } } +func TestJSONAndTextHandlersWithUnavailableSource(t *testing.T) { + // Verify that a nil source does not cause a panic. + // and that the source is empty. + var buf bytes.Buffer + opts := &HandlerOptions{ + ReplaceAttr: removeKeys(LevelKey), + AddSource: true, + } + + for _, test := range []struct { + name string + h Handler + want string + }{ + {"text", NewTextHandler(&buf, opts), "source=:0 msg=message"}, + {"json", NewJSONHandler(&buf, opts), `{"msg":"message"}`}, + } { + t.Run(test.name, func(t *testing.T) { + buf.Reset() + r := NewRecord(time.Time{}, LevelInfo, "message", 0) + err := test.h.Handle(t.Context(), r) + if err != nil { + t.Fatal(err) + } + + want := strings.TrimSpace(test.want) + got := strings.TrimSpace(buf.String()) + if got != want { + t.Errorf("\ngot %s\nwant %s", got, want) + } + }) + } +} + func TestSecondWith(t *testing.T) { // Verify that a second call to Logger.With does not corrupt // the original. diff --git a/src/log/slog/logger_test.go b/src/log/slog/logger_test.go index 8669bc1428..558aecaf6e 100644 --- a/src/log/slog/logger_test.go +++ b/src/log/slog/logger_test.go @@ -191,6 +191,9 @@ func TestCallDepth(t *testing.T) { const wantFile = "logger_test.go" wantLine := startLine + count*2 got := h.r.Source() + if got == nil { + t.Fatal("got nil source") + } gotFile := filepath.Base(got.File) if got.Function != wantFunc || gotFile != wantFile || got.Line != wantLine { t.Errorf("got (%s, %s, %d), want (%s, %s, %d)", diff --git a/src/log/slog/record.go b/src/log/slog/record.go index 5a06f4f8fd..53ecc67cc8 100644 --- a/src/log/slog/record.go +++ b/src/log/slog/record.go @@ -211,11 +211,14 @@ func (s *Source) group() Value { return GroupValue(as...) } -// Source returns a Source for the log event. -// If the Record was created without the necessary information, -// or if the location is unavailable, it returns a non-nil *Source -// with zero fields. +// Source returns a new Source for the log event using r's PC. +// If the PC field is zero, meaning the Record was created without the necessary information +// or the location is unavailable, then nil is returned. func (r Record) Source() *Source { + if r.PC == 0 { + return nil + } + fs := runtime.CallersFrames([]uintptr{r.PC}) f, _ := fs.Next() return &Source{ diff --git a/src/log/slog/record_test.go b/src/log/slog/record_test.go index 575c7221f0..939dc34ac8 100644 --- a/src/log/slog/record_test.go +++ b/src/log/slog/record_test.go @@ -39,17 +39,18 @@ func TestRecordAttrs(t *testing.T) { } func TestRecordSource(t *testing.T) { - // Zero call depth => empty *Source. + // Zero call depth => nil *Source. for _, test := range []struct { depth int wantFunction string wantFile string wantLinePositive bool + wantNil bool }{ - {0, "", "", false}, - {-16, "", "", false}, - {1, "log/slog.TestRecordSource", "record_test.go", true}, // 1: caller of NewRecord - {2, "testing.tRunner", "testing.go", true}, + {0, "", "", false, true}, + {-16, "", "", false, true}, + {1, "log/slog.TestRecordSource", "record_test.go", true, false}, // 1: caller of NewRecord + {2, "testing.tRunner", "testing.go", true, false}, } { var pc uintptr if test.depth > 0 { @@ -57,6 +58,16 @@ func TestRecordSource(t *testing.T) { } r := NewRecord(time.Time{}, 0, "", pc) got := r.Source() + if test.wantNil { + if got != nil { + t.Errorf("depth %d: got non-nil Source, want nil", test.depth) + } + continue + } + if got == nil { + t.Errorf("depth %d: got nil Source, want non-nil", test.depth) + continue + } if i := strings.LastIndexByte(got.File, '/'); i >= 0 { got.File = got.File[i+1:] }