Introduce task local logger #315
Conversation
FranzBusch
force-pushed
the
fb-task-local-logger
branch
2 times, most recently
from
13be449 to
0e33370
Compare
There was a problem hiding this comment.
I’m currently offline for a week and would really want to have the time to dive deep on this. I don’t think it’s an obvious great idea and needs some proper debate and plans before merging. I’m especially concerned what this means for the library ecosystem and will have to think this through more.
Requesting changes to mark this intent. Thanks for your patience
Sources/Logging/Logging.swift
Outdated
| @@ -43,6 +43,9 @@ import WASILibc | |||
| /// logger.info("Hello World!") | |||
| /// ``` | |||
| public struct Logger { | |||
| @available(macOS 10.15, *) | |||
| @TaskLocal | |||
| public static var logger: Logger = Logger(label: "NoOp", SwiftLogNoOpLogHandler()) | |||
There was a problem hiding this comment.
I'd suggest a name like current or inherited so that it reads more naturally than Logger.logger (which sounds like a factory method that creates a brand new logger instead)
There was a problem hiding this comment.
Agree! Logger.current or Logger.inherited both sound good to me but open to other stuff too
Sources/Logging/Logging.swift
Outdated
| @@ -43,6 +43,9 @@ import WASILibc | |||
| /// logger.info("Hello World!") | |||
| /// ``` | |||
| public struct Logger { | |||
| @available(macOS 10.15, *) | |||
| @TaskLocal | |||
| public static var logger: Logger = Logger(label: "NoOp", SwiftLogNoOpLogHandler()) | |||
There was a problem hiding this comment.
Agree! Logger.current or Logger.inherited both sound good to me but open to other stuff too
There was a problem hiding this comment.
Overall LGTM I have concerns about it being enforced and not opt-in (though I guess one could pin to the old version). It's good to force performance improvements in the runtime but with such a performance hit being shown I wonder if it's worth offering a way of not using it - ignore me, it's entirely opt in if you just create an instance of the logger and pass that around 🤦 don't review code at 3AM 😆
|
|
||
| var logger = Logger.logger | ||
| logger[metadataKey: "MetadataKey1"] = "Value1" | ||
| Logger.$logger.withValue(logger) { |
There was a problem hiding this comment.
Is this syntax required for passing metadata through to child tasks? I thought that was automatic. E.g.:
logger[metadataKey: "MetadataKey1"] = "Value1"
try await someFunction()
func someFunction() async throws {
// `MetadataKey1` is available here
}Definitely agree with renaming logger here - the API doesn't come across great with the double logger in this example
There was a problem hiding this comment.
Logger is a value type, so the modified copy needs to be written back into the task local to be propagated to child tasks.
# Motivation Structured logging and structured concurrency are a match made in heaven. Using task locals for loggers enables easier passing of metadata for log messages and removes the cumbersome passing around of loggers. # Modification This PR adds a proposal, the implementation and benchmarks for a task local logger. # Result Using a task local for logging makes it incredible ergonomic to carry around metadata. To be transparent the benchmarks show that this is around 20x slower than a bare no-op log. A no-op log takes around 2ns whereas a task local logger no-op log takes around 40ns. 95% of that time is spent in accessing the task local since that is missing some `@inlinable` in the runtime and goes through the C interface. Instruction wise it is 20 vs 290 instructions. Now I still propose that we go forward with this since the ergonomics far outweigh the performance initially. Furthermore, this acts as a great forcing function for performance improvements in the runtime since logging is often done in hot paths.
FranzBusch
force-pushed
the
fb-task-local-logger
branch
from
0e33370 to
565e5fc
Compare
Motivation
Structured logging and structured concurrency are a match made in heaven. Using task locals for loggers enables easier passing of metadata for log messages and removes the cumbersome passing around of loggers.
Modification
This PR adds a proposal, the implementation and benchmarks for a task local logger.
Result
Using a task local for logging makes it incredible ergonomic to carry around metadata. To be transparent the benchmarks show that this is around 20x slower than a bare no-op log. A no-op log takes around 2ns whereas a task local logger no-op log takes around 40ns. 95% of that time is spent in accessing the task local since that is missing some
@inlinablein the runtime and goes through the C interface. Instruction wise it is 20 vs 290 instructions.Now I still propose that we go forward with this since the ergonomics far outweigh the performance initially. Furthermore, this acts as a great forcing function for performance improvements in the runtime since logging is often done in hot paths.