Skip to content
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.

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

slogr: restore original backend when converting back and forth #210

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Aug 23, 2023

This works both ways by checking whether the backend is one of our own wrappers. There's one exception: when the verbosity level was modified, we cannot use the original slog.Handler because it doesn't know about that modification.

The downside is that we have to adjust the stack unwinding each time a record is handled, because we need the original sink without that adjustment. This only gets done when actually emitting a log record, so that overhead should be okay. It will disappear entirely once sinks directly support writing a slog.Event.

A second copy would work, but then that copy would need to be updated alongside the other one in WithAttrs, which causes overhead along a callchain also when the log record does not get emitted. The code also would be more complex.

This works both ways by checking whether the backend is one of our own
wrappers. There's one exception: when the verbosity level was modified, we
cannot use the original slog.Handler because it doesn't know about that
modification.

The downside is that we have to adjust the stack unwinding each time a record
is handled, because we need the original sink without that adjustment. This
only gets done when actually emitting a log record, so that overhead should be
okay. It will disappear entirely once sinks directly support writing a
slog.Event.

A second copy would work, but then that copy would need to be updated alongside
the other one in WithAttrs, which causes overhead along a callchain also when
the log record does not get emitted. The code also would be more complex.
@thockin thockin merged commit 358130c into go-logr:master Aug 23, 2023
@thockin
Copy link
Contributor

thockin commented Aug 23, 2023

ok

@pohly pohly mentioned this pull request Aug 25, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants