[SLG-0006]: task-local logger implementation#459
Conversation
kukushechkin
force-pushed
the
SLG-0006-task-local-logger-implementation
branch
from
c5471e9 to
16bce4e
Compare
kukushechkin
changed the title
kukushechkin
force-pushed
the
SLG-0006-task-local-logger-implementation
branch
from
16bce4e to
80e5dfd
Compare
kukushechkin
force-pushed
the
SLG-0006-task-local-logger-implementation
branch
from
80e5dfd to
0ed85c1
Compare
| #### `Logger.current` | ||
|
|
||
| Returns the logger bound by the nearest enclosing `withLogger` scope. If none has been set | ||
| up, returns a fallback logger: the globally bootstrapped handler if |
There was a problem hiding this comment.
Hmm, we globally bootstrap a factory to make a handler, not actually a handler.
And that's a problem for this. I don't think we should invoke the factory for every access even if you didn't withLogger { ... } anything up. Why isn't there a single, global fallback Logger that gets created lazily by invoking the factory once on first use?
|
|
||
| Returns the logger bound by the nearest enclosing `withLogger` scope. If none has been set | ||
| up, returns a fallback logger: the globally bootstrapped handler if | ||
| `LoggingSystem.bootstrap` has been called, otherwise a silent `SwiftLogNoOpLogHandler`. |
There was a problem hiding this comment.
I like making the default silent but in the original SwiftLog design we decided that that would be too unexpected for users. I think we should keep the original default: If you bootstrap you get that, otherwise you get the default no frills stdio logger.
| first time it is taken so a user who to wrap their entry point in `withLogger` | ||
| (or forgot to call `LoggingSystem.bootstrap`) doesn't see logs silently disappear with no diagnostic. | ||
|
|
||
| The bootstrapped branch invokes `LoggingSystem.factory` on every access. `Logger.current` is |
There was a problem hiding this comment.
Why not a single, lazy global fallback Logger instead of calling the factory many times?
| /// from `withLogger { logger in ... }` — instead of re-reading the task-local | ||
| /// on every call. | ||
| @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) | ||
| public static var current: Logger { get } |
There was a problem hiding this comment.
I'm somewhat sceptical of calling it just current. It's definitely convenient and reads nicely but it feels pretty magic. Logger.taskLocal.debug("hello") or something similar may be better because it explains nicely where it comes from.
In some systems, there will be multiple loggers to choose from and I think adding more clarity here would help.
| /// parameter so the body does not need to re-read ``Logger/current``. | ||
| /// - Returns: The value returned by `operation`. | ||
| @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) | ||
| public func withLogger<Result>( |
There was a problem hiding this comment.
Why not Logger.withLogger(...) { ... }? reads nicer than the free withLogger
2 participants
This proposal adds task-local logger storage so that metadata can accumulate across async
call stacks without threading a
Loggerthrough every function signature.Motivation:
Metadata propagation requires threading a logger through every layer. Every function on
the path from request ingress to the bottom of the call stack has to accept, mutate, and
forward a
Logger. Libraries must choose between polluting their public API with alogger:parameter andlosing all the caller's context. There is no third option today.
Modifications:
@TaskLocalLoggerinstance added.Logger.currentAPI.Result:
Users can now automatically propagate accumulated metadata into libraries without explicitly passing loggers through the API boundaries.