Skip to content

fix: don't panic, only accept typed log Fields #222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spikecurtis wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −98
Open

fix: don't panic, only accept typed log Fields #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions internal/entryhuman/entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ func formatValue(v interface{}) string {
case reflect.Struct, reflect.Map:
byt, err := json.Marshal(v)
if err != nil {
panic(err)
// don't panic
return "!! Error while marshalling value !!"
Copy link
Member

@johnstcn johnstcn Dec 19, 2025

Choose a reason for hiding this comment

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

While this is much more preferable to panicking, it does raise the possibility of unmarshal-able things at runtime. Could we possibly detect this in sloggers/slogtest and cause a test failure?

}
return string(byt)
case reflect.Slice:
Expand Down Expand Up @@ -334,7 +335,8 @@ func levelStyle(level slog.Level) lipgloss.Style {
case slog.LevelError, slog.LevelFatal, slog.LevelCritical:
return levelErrorStyle
default:
panic("unknown level")
// don't panic
return levelErrorStyle
}
}

Expand Down
66 changes: 7 additions & 59 deletions slog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,67 +80,40 @@ func Make(sinks ...Sink) Logger {
}

// Debug logs the msg and fields at LevelDebug.
// See Info for information on the fields argument.
func (l Logger) Debug(ctx context.Context, msg string, fields ...any) {
func (l Logger) Debug(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelDebug, msg, fields)
}

// Info logs the msg and fields at LevelInfo.
// Fields may contain any combination of key value pairs, Field, and Map.
// For example:
//
// log.Info(ctx, "something happened", "user", "alex", slog.F("age", 20))
//
// is equivalent to:
//
// log.Info(ctx, "something happened", slog.F("user", "alex"), slog.F("age", 20))
//
// is equivalent to:
//
// log.Info(ctx, "something happened", slog.M(
// slog.F("user", "alex"),
// slog.F("age", 20),
// ))
//
// is equivalent to:
//
// log.Info(ctx, "something happened", "user", "alex", "age", 20)
//
// In general, prefer using key value pairs over Field and Map, as that is how
// the standard library's slog package works.
func (l Logger) Info(ctx context.Context, msg string, fields ...any) {
func (l Logger) Info(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelInfo, msg, fields)
}

// Warn logs the msg and fields at LevelWarn.
// See Info() for information on the fields argument.
func (l Logger) Warn(ctx context.Context, msg string, fields ...any) {
func (l Logger) Warn(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelWarn, msg, fields)
}

// Error logs the msg and fields at LevelError.
// See Info() for information on the fields argument.
//
// It will then Sync().
func (l Logger) Error(ctx context.Context, msg string, fields ...any) {
func (l Logger) Error(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelError, msg, fields)
l.Sync()
}

// Critical logs the msg and fields at LevelCritical.
// See Info() for information on the fields argument.
//
// It will then Sync().
func (l Logger) Critical(ctx context.Context, msg string, fields ...any) {
func (l Logger) Critical(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelCritical, msg, fields)
l.Sync()
}

// Fatal logs the msg and fields at LevelFatal.
// See Info() for information on the fields argument.
//
// It will then Sync() and os.Exit(1).
func (l Logger) Fatal(ctx context.Context, msg string, fields ...any) {
func (l Logger) Fatal(ctx context.Context, msg string, fields ...Field) {
l.log(ctx, LevelFatal, msg, fields)
l.Sync()

Expand Down Expand Up @@ -182,32 +155,7 @@ func (l Logger) AppendSinks(s ...Sink) Logger {
return l
}

func (l Logger) log(ctx context.Context, level Level, msg string, rawFields []any) {
fields := make(Map, 0, len(rawFields))
var wipField Field
for i, f := range rawFields {
if wipField.Name != "" {
wipField.Value = f
fields = append(fields, wipField)
wipField = Field{}
continue
}
switch f := f.(type) {
case Field:
fields = append(fields, f)
case Map:
fields = append(fields, f...)
case string:
wipField.Name = f
default:
panic(fmt.Sprintf("unexpected field type %T at index %v (does it have a key?)", f, i))
}
}

if wipField.Name != "" {
panic(fmt.Sprintf("field %q has no value", wipField.Name))
}

func (l Logger) log(ctx context.Context, level Level, msg string, fields []Field) {
ent := l.entry(ctx, level, msg, fields)
l.Log(ctx, ent)
}
Expand Down
35 changes: 2 additions & 33 deletions slog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package slog_test

import (
"context"
"fmt"
"io"
"runtime"
"testing"
Expand Down Expand Up @@ -76,7 +75,7 @@ func TestLogger(t *testing.T) {

File: slogTestFile,
Func: "cdr.dev/slog_test.TestLogger.func2",
Line: 68,
Line: 67,

Fields: slog.M(
slog.F("ctx", 1024),
Expand Down Expand Up @@ -113,7 +112,7 @@ func TestLogger(t *testing.T) {

File: slogTestFile,
Func: "cdr.dev/slog_test.TestLogger.func3",
Line: 103,
Line: 102,

SpanContext: span.SpanContext(),

Expand Down Expand Up @@ -154,36 +153,6 @@ func TestLogger(t *testing.T) {
assert.Equal(t, "level", slog.LevelFatal, s.entries[5].Level)
assert.Equal(t, "exits", 1, exits)
})

t.Run("kv", func(t *testing.T) {
s := &fakeSink{}
l := slog.Make(s)

// All of these formats should be equivalent.
formats := [][]any{
{"animal", "cat", "weight", 15},
{slog.F("animal", "cat"), "weight", 15},
{slog.M(
slog.F("animal", "cat"),
slog.F("weight", 15),
)},
{slog.F("animal", "cat"), slog.F("weight", 15)},
}

for _, format := range formats {
l.Info(bg, "msg", format...)
}

assert.Len(t, "entries", 4, s.entries)

for i := range s.entries {
assert.Equal(
t, fmt.Sprintf("%v", i),
s.entries[0].Fields,
s.entries[i].Fields,
)
}
})
}

func TestLevel_String(t *testing.T) {
Expand Down
8 changes: 4 additions & 4 deletions sloggers/slogtest/t.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,25 +146,25 @@ func l(t testing.TB) slog.Logger {
}

// Debug logs the given msg and fields to t via t.Log at the debug level.
func Debug(t testing.TB, msg string, fields ...any) {
func Debug(t testing.TB, msg string, fields ...slog.Field) {
slog.Helper()
l(t).Debug(ctx, msg, fields...)
}

// Info logs the given msg and fields to t via t.Log at the info level.
func Info(t testing.TB, msg string, fields ...any) {
func Info(t testing.TB, msg string, fields ...slog.Field) {
slog.Helper()
l(t).Info(ctx, msg, fields...)
}

// Error logs the given msg and fields to t via t.Error at the error level.
func Error(t testing.TB, msg string, fields ...any) {
func Error(t testing.TB, msg string, fields ...slog.Field) {
slog.Helper()
l(t).Error(ctx, msg, fields...)
}

// Fatal logs the given msg and fields to t via t.Fatal at the fatal level.
func Fatal(t testing.TB, msg string, fields ...any) {
func Fatal(t testing.TB, msg string, fields ...slog.Field) {
slog.Helper()
l(t).Fatal(ctx, msg, fields...)
}
Expand Down