[rlog] Updates based on integration lessons learned by chris-okuda · Pull Request #39 · rotationalio/x

chris-okuda · 2026-03-26T01:28:28Z

Scope of changes

Fixing some sharp edges and bugs and adding some useful features that were brought up when doing the zerolog to rlog switchover in endeavor.

Replace per-logger SetExitFunc with process-wide SetFatalHook; os.Exit(1) when nil
Store default logger in pointer; SetDefault copies logger and calls slog.SetDefault; init wires slog default
Add Logger.With / WithGroup and package With / WithGroup
Add FanOut slog.Handler (NewFanOut, clone record per child)
Add testing helper NewCapturingTestHandler
Rename handler.go to options.go
Move tests out of rlog_test.go to other files
Add doc.go; update README

Comments for reviewer here.

Fixes SC-37788

Type of change

Author checklist

I have manually tested the change and/or added automation in the form of unit tests or integration tests
I have added new test fixtures as needed to support added tests
Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.
Are there any TODOs in this PR that should be turned into stories?

…test helpers - Replace per-logger SetExitFunc with process-wide SetFatalHook; os.Exit(1) when nil - Store default logger in atomic.Pointer; SetDefault copies logger and calls slog.SetDefault; init wires slog default - Add Logger.With / WithGroup and package With / WithGroup - Add FanOut slog.Handler (NewFanOut, clone record per child) - Add testing helpers (e.g. NewCapturingTestHandler, ParseJSONLine) - Rename handler.go to options.go; move tests to logger_test.go - Add doc.go; update README

chris-okuda

Comments for reviewer.

chris-okuda · 2026-03-26T01:29:16Z

rlog/fanout.go

+// FanOut is a [slog.Handler] that forwards each record to every child handler,
+// using a fresh [slog.Record.Clone] per child.
+type FanOut struct {
+	handlers []slog.Handler
+}


We were going to need this for Quarterdeck and other stuff. I also didn't realize G0 1.26 provides a slog.MultiHandler so maybe we can use that if we upgrade?

Oh interesting - yes, I'm happy to upgrade to 1.26 if that makes our lives easier.

rlog/logger.go

chris-okuda · 2026-03-26T18:25:20Z

rlog/logger.go

+	// Stores the global logger that is returned by [Default]
+	globalLogger atomic.Pointer[Logger]
+	// Protects reads and writes to the globalLogger
+	loggerMu sync.RWMutex


I was able to eliminate the atomic!

chris-okuda · 2026-03-26T01:32:42Z

rlog/logger.go

 func Default() *Logger {
 	loggerMu.RLock()
-	l := globalLogger
-	loggerMu.RUnlock()
-	return &l
+	defer loggerMu.RUnlock()
+	return globalLogger.Load()
 }


Removes this copy allocation

So atomic is a low level concurrency primitive and notoriously difficult to use correctly. Is that why you have it wrapped in the mutex? Is there some race condition between globalLogger.Load() and globalLogger.Store?

Never mind - saw your comment below!

chris-okuda · 2026-03-26T01:33:26Z

rlog/logger.go

 func SetDefault(logger *Logger) {
+	p := new(Logger)
+	*p = *logger
 	loggerMu.Lock()
 	defer loggerMu.Unlock()
-	globalLogger = *logger
+	globalLogger.Store(p)
+	slog.SetDefault(p.Logger)
 }


We still need the loggerMu to prevent race conditions where we might set/get the default logger in the wrong order. The globalLogger atomic is mainly to prevent having to copy the previous globalLogger.

Ok that makes perfect sense - atomic as a low level concurrency primitive is notoriously difficult to use correctly. But won't simply keeping a pointer reference to the variable allow you to prevent copies without the atomic? Or is it that you're worried about the garbage collector?

I went through a few iterations on this and there were race issues with concurrent SetDefault/Default use in that test due to both pointer issues, copies, and coordinating the default rlog logger with the default slog logger. I might be able to do something else now, but this seemed to work with this combo so I left it in the end and decided not to put more effort into figuring out if I could eliminate the atomic.

I was thinking about this last night, too, "why do I need a mutex AND an atomic value?", but I still need more experience with concurrency concepts to really grok them.

Well if it works then it works - honestly in our code setdefault only happens once at the begining of the process so only Load/ RLock is going to be called in real operations.

I removed it for a regular pointer, I guess I never tried that combination :)

rlog/options.go

chris-okuda · 2026-03-26T01:35:54Z

rlog/testing.go

+// CapturingTestHandler is a [slog.Handler] that records each [slog.Record] and
+// each rendered JSON line (one line per Handle call). All derived handlers
+// (via WithAttrs/WithGroup) share the same capture buffers. Construct with
+// [NewCapturingTestHandler]; pass a non-nil [testing.TB] to also log each line
+// via [testing.TB.Log].
+type CapturingTestHandler struct {
+	state    *captureState
+	topAttrs []slog.Attr
+	segments []captureGroupSegment
+	tb       testing.TB
+}


I was creating this sort of thing with various features all over the place; this one can be used for all the different situations we need it for.

Good pattern.

bbengfort

Very well thought out and designed library package!

bbengfort · 2026-03-26T14:49:38Z

rlog/fanout.go

+// FanOut is a [slog.Handler] that forwards each record to every child handler,
+// using a fresh [slog.Record.Clone] per child.
+type FanOut struct {
+	handlers []slog.Handler
+}


Oh interesting - yes, I'm happy to upgrade to 1.26 if that makes our lives easier.

bbengfort · 2026-03-26T14:50:08Z

rlog/logger.go

 }

-// New returns a new [Logger] using the given [slog.Logger].
+// New returns a new [Logger] wrapping the given [slog.Logger].


What happens if logger is nil? Does it just use the default logger?

That is a good point; I guess if it's nil that causes an issue! I'll make a nil guard here, even though I really hope the user wouldn't do that.

bbengfort · 2026-03-26T14:51:20Z

rlog/logger.go

 )

-// Initializes the global logger and level once. Is a no-op if already initialized.
+// Initializes the global logger to be a console JSON logger with level [slog.LevelInfo].


Good default!

bbengfort · 2026-03-26T14:53:00Z

rlog/logger.go

 func Default() *Logger {
 	loggerMu.RLock()
-	l := globalLogger
-	loggerMu.RUnlock()
-	return &l
+	defer loggerMu.RUnlock()
+	return globalLogger.Load()
 }


So atomic is a low level concurrency primitive and notoriously difficult to use correctly. Is that why you have it wrapped in the mutex? Is there some race condition between globalLogger.Load() and globalLogger.Store?

Never mind - saw your comment below!

bbengfort · 2026-03-26T14:54:36Z

rlog/logger.go

 func SetDefault(logger *Logger) {
+	p := new(Logger)
+	*p = *logger
 	loggerMu.Lock()
 	defer loggerMu.Unlock()
-	globalLogger = *logger
+	globalLogger.Store(p)
+	slog.SetDefault(p.Logger)
 }


Ok that makes perfect sense - atomic as a low level concurrency primitive is notoriously difficult to use correctly. But won't simply keeping a pointer reference to the variable allow you to prevent copies without the atomic? Or is it that you're worried about the garbage collector?

bbengfort · 2026-03-26T14:55:11Z

rlog/testing.go

+// CapturingTestHandler is a [slog.Handler] that records each [slog.Record] and
+// each rendered JSON line (one line per Handle call). All derived handlers
+// (via WithAttrs/WithGroup) share the same capture buffers. Construct with
+// [NewCapturingTestHandler]; pass a non-nil [testing.TB] to also log each line
+// via [testing.TB.Log].
+type CapturingTestHandler struct {
+	state    *captureState
+	topAttrs []slog.Attr
+	segments []captureGroupSegment
+	tb       testing.TB
+}


Good pattern.

…default logger if the user provides a nil slog logger in New

chris-okuda self-assigned this Mar 26, 2026

chris-okuda commented Mar 26, 2026

View reviewed changes

chris-okuda marked this pull request as ready for review March 26, 2026 01:37

This comment was marked as resolved.

Sign in to view

chris-okuda added 2 commits March 25, 2026 15:48

remove old test stuff

97980e7

fix bug in fanout handler

5534eda

bbengfort approved these changes Mar 26, 2026

View reviewed changes

chris-okuda added 3 commits March 26, 2026 08:09

note to use the slog multihandler in go 1.26 or later and return the …

449351e

…default logger if the user provides a nil slog logger in New

we can get rid of the atomic if I do things correctly with a pointer

e8ac89d

add with/withgroup test

6f362b0

chris-okuda merged commit 0596351 into main Mar 26, 2026
3 checks passed

chris-okuda deleted the chrisokuda/sc-37788/rlog_updates branch March 26, 2026 18:31

Conversation

chris-okuda commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope of changes

Type of change

Author checklist

Reviewer(s) checklist

Uh oh!

chris-okuda left a comment

Choose a reason for hiding this comment

Uh oh!

chris-okuda Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-okuda Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

bbengfort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chris-okuda commented Mar 26, 2026 •

edited

Loading

chris-okuda Mar 26, 2026 •

edited

Loading

chris-okuda Mar 26, 2026 •

edited

Loading