Hi all,
The proposal SLG-0003: Standardized Error Metadata via Logger Convenience for swift-log by Samuel Murray is now up and In Review.
The review period will run until Feb 9th — please feel free to post your feedback as a reply to this thread.
Thanks!
Adding the error parameter might be fine even if a bit weird…
But record error strikes me as a bad idea. It comes from a specific usage pattern where it’s probably ok, creating a logger for a bunch of operations. But it’s most likely entirely wrong to use this if you’ve stored the logger anywhere, subsequently adding an error to every single log statement you’re making with the logger. If anything this might need to be an with… api, but honestly I don’t really see this “multiple log statements with the same error” as a useful pattern to begin with and I’m not sure why we should encourage that? You log the error at the most severe level it matters for and the remaining things are additional information — duplicating the error across many log stamanets doesn’t seem beneficial to me, it adds to log traffic, and complicates analysis “which was the first one? Did it happen many times?” Etc.
I’d argue to leave out the recordError idea.
Also, shouldn’t the new signatures take “some Error” not “any Error”?
Wouldn’t that prevent the caller from passing an “any Error” to the function? Which is typically what they would want to do, if you log in a catch block.
I agree this is a problem worth solving. I see disparities among different projects at work (same company!) on what keys are chosen.
I also agree with @ktoso that recordError is a bit funky. I think it certainly unusual (but not impossible) that I’d want to log multiple times with the same error. I also think the name sounds a lot more like it just logs the error in some standardized way.
However I'm less concerned about this ^. I think Logger being a value type (as far as metadata is concerned) makes this much harder to do by accident.
I’m not 100% sold on this idea, but I didn’t see it considered: have you thought about introducing an init(error:) on Logger.Metadata which produces the same metadata? That way the examples could be:
logger.warning("something went wrong", metadata: Logger.Metadata(error: error))
and if you wanted the recordError functionality it would compose somewhat naturally:
logger.metadata = .init(error: error)
and if you wanted to log other metadata that would work just fine too:
logger.warning("oh no", metadata: ["operation" : "upload"] + .init(error: error))
Downsides I see are:
[String : Logger.MetadataValue] so you're extending a stdlib type in the library, which is often a bit weird. I think this is mitigated though because it only applies to a dictionary with Value set to a type defined in this library.Upsides I see are:
String(reflecting: error) to my logs because it usually gives more detail than my more user-facing description. But you wouldn't always want that, so it could be init(error: any Error, reflect: Bool = false).And then I have a few comments about the chosen metadata keys:
error.type is extremely useful and often forgotten. So +1 on that.error.message is slightly misnamed. It includes the error message if the error message is implemented with CustomStringConvertible and description, but people often use Foundation’s LocalizedError and errorDescription property for this. Or Error’s localizedDescription. A better name IMO would be error.description or simply error.string.Maybe the error should rather be handed as-is to the log handler and it is up to the configured logging backend to decide on how to convert it into metadata?
This is mentioned as “Future directions”. If this proposal gets implemented, we could have a second proposal where the error is passed as-is, and the metadata serialization is a default for any LogHandlers that don’t implement this. However, I understood from the original discussions that passing the error might conflict with the intended abstraction layer between Logger and LogHandler.
Not since we have implicit existential opening, and the following works:
struct Boom: Error {
init() {}
}
let e: any Error = Boom()
square(e) // implicitly opened existential
func square(_ n: some Error) {
"\(n)"
}
Though it may be worth checking the performance overhead of it.
I'm unconvinced that just standardizing error serialization should be the way to go, the future directions of passing in the actual Error is what developers really want when making log handlers over some system that itself takes an Error instance.
For example, I recently made a log handler for the New Relic SDK that includes a recordError(error: Error, ...) function. While the SDK might just serialize the error itself, the SDK also attaches additional metadata for error or crash analysis and may present this log in a separate dashboard.
This is interesting. What additional metadata does it take? Could you point to the SDK API?
I'm a little bit torn on this one, so I'll write down my thoughts.
The original intent of swift-log was to keep it "as policy-free as possible" and make it just the mechanism or the front door to send logs without interpretation. Everything apart from that is meant to be decided by the logging backends.
Why is that desirable? Well, nobody can agree on everything so the separation of policy & mechanism really helps with that.
Making certain keys "special" is certainly a departure from the original design and usually I would argue against it strongly. In this case however, I think it's fair to say that Errors are quite special in Swift. So maybe Logger's API does deserve a special place for it.
One thing that remains unclear to me is if swift-log should decide if the Error is sent through metadata or not. There is an alternative that I think should be explored which would be to hand the any Error straight to the logging backend for the logging backend to decide where it goes and what to do with it. For example, in my team we use different metadata keys than the ones proposed. If we let the logging backend decide what to do, we could leverage this without breaking all of our triggers and telemetry.
Overall:
Logger's APIMaybe the question should be if this proposal (serializing as metadata) is an acceptable first step, with passing the Error to a LogHandler a potential extension, or if people think taking this first step is a step in the wrong direction regardless. Especially if we were to drop recordError from the proposal, I think it would be easy to extend the solution to also offer a LogHandler to get access to the original Error.
For transparency, I personally want the latter, but I think serializing as metadata is pragmatic first step.
Hypothetically, if we eventually go with Error being part of the LogHandler API, how would this API look like and will it conflict with what is proposed in this proposal, for example the error: param in log methods? In this case, as the pragmatic first step, we might go with what @willft suggested (logger.warning("oh no", metadata: ["operation" : "upload"] + .init(error: error))) and reserve the “nicer” logger.log(…, error: someError) API for the future.
I don’t think adding two different APIs in Logging is a good idea - it risks confusing people using swift-log, and forces LogHandlers to handle inputs from either use case.
My idea for how we could “migrate” from this proposal to passing Error as-is was rather that the Logger API would stay the same, but we change the implementation in swift-log. We add a new method on LogHandler that accepts an Error, and the default implementation of that is to call the existing method. And we do the serialization of metadata in LogHandler instead of in Logger. That way, the behaviour will stay the same for any LogHandlers that don’t implement the new method, but it naturally allows LogHandlers to implement the new one.
// Logging.swift
extension Logger {
// Modified existing func
package func _log(
level: Logger.Level,
_ message: @autoclosure () -> Logger.Message,
error: any Error?,
metadata: @autoclosure () -> Logger.Metadata? = nil,
source: @autoclosure () -> String? = nil,
file: String = #fileID,
function: String = #function,
line: UInt = #line
) {
self.handler.log(
level: level,
message: message(),
error: error, // Pass error as-is
metadata: metadata(),
source: source() ?? Logger.currentModule(fileID: (file)),
file: file,
function: function,
line: line
)
}
}
// LogHandler.swift
protocol LogHandler {
// New func
func log(
level: Logger.Level,
message: Logger.Message,
error: any Error?,
metadata: Logger.Metadata?,
source: String,
file: String,
function: String,
line: UInt
)
// Existing func
func log(
level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
source: String,
file: String,
function: String,
line: UInt
)
}
extension LogHandler {
// Default implementation
public func log(
level: Logger.Level,
message: Logger.Message,
error: (any Error)?,
metadata: Logger.Metadata?,
source: String,
file: String,
function: String,
line: UInt
) {
self.log(
level: level,
message: message,
metadata: merge(metadata: metadata, error: error),
source: source,
file: file,
function: function,
line: line)
}
private func merge(metadata: Logger.Metadata?, error: (any Error)?) -> Logger.Metadata? {
if let error {
var metadata = metadata ?? Logger.Metadata()
metadata[Logger.WellKnownMetadataKey.errorMessage] = "\(error)"
metadata[Logger.WellKnownMetadataKey.errorType] = "\(String(reflecting: type(of: error)))"
return metadata
} else {
return metadata
}
}
}
It’s separate questions really, the proposed surface API I think is fine, accepting an error: explicitly is useful.
Now, however we decide to put this into log handlers we’ll be stuck with because handlers will start expecting this new behavior. So I don’t think we can say “first step” to make it metadata but then eventually stop doing that. We’d have to slowly migrate etc… those are painful, so it’d be better to do the right thing right away.
We’ve added new log handler methods in the past:
@available(*, deprecated, message: "You should implement this method instead of using the default implementation")
public func log(
level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
source: String,
file: String,
function: String,
line: UInt
) {
self.log(level: level, message: message, metadata: metadata, file: file, function: function, line: line)
}
So I think this may be the route here:
protocol LogHandler: ... {
public func log( // new method
level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
error: (any Error)?, // new
source: String,
file: String,
function: String,
line: UInt
)
}
extension LogHandler {
@available(*, deprecated, message: "You should implement this method instead of using the default implementation")
public func log(
level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
error: (any Error)?,
source: String,
file: String,
function: String,
line: UInt
) {
// legacy compatibility, forward error as metadata
self.log(level: level, message: message,
metadata: metadata + [
"error.message": "\(error)",
"error.type": "\(String(reflecting: type(of: error))",
],
file: file, function: function, line: line)
}
And deprecate the existing funcs which do not take the error – that’s how we’ve added source: back then. Specifically the deprecations are important here as we nudge new implementations to the new function.
This would result in:
Thank you all for the thoughtful feedback! @samuelmurray and @ktoso came up with similar ideas, I think this is a good sign 
It seems the community converges towards passing the Error to the LogHandler and does not see that much value in only serializing Error on the Logger side. It also makes it easier to discuss any error serialization in conjunction with passing Error to the LogHandler.
I'd like to ask @samuelmurray to revise the proposal scope to address this. Rather than just defining standardized metadata keys, add Error support to the LogHandler protocol itself, with a default implementation providing the metadata serialization. I would also remove .recordError from the proposal scope, add it to the future directions and implement when swift-log has a better approach to scoped loggers.
If it feels appropriate, the implementation can happen in two phases: first, the fallback metadata serialization, then the new LogHandler interface for opting into detailed error handling.
I would suggest doing it in one go, to simplify understanding what behavior is expected from what log handler.
I'm actually +1 on the proposal as written (sorry for chiming in late), though I agree recordError might make sense to remove.
Before we add the error parameter to LogHandler, however, I think we should pause for a second and consider the consequences.
Generally, these API packages (Swift Log/Metrics/Tracing) promise moving from M * N (every library needs to support every logging backend explicitly) to M + N (every library works with every logging backend). In other words, we need to avoid creating an API that couples specific libraries to specific LogHandlers. Why? Because it breaks the ecosystem - suddenly we'll have library maintainers recommending a list of "blessed" LogHandlers in their README, with which their library works correctly. We'll lose M + N, and we'll go back to ~M * N. I think that'd be a step backwards in the ecosystem's interoperability.
When we look at what the Error protocol is - it's just a Sendable Any with special handling by the compiler. But it provides no actual methods to extract information. This proposal, as-is, took a pragmatic approach of extracting 1) the type and 2) the stringified representation (allowing errors to conform to CustomStringConvertible to take control over that output). Now, if we're saying every LogHandler will only extract these two fields, then we can just do it, as proposed, at the Logger level, and LogHandlers don't need to change anything and benefit right away.
However, it sounds like that's not what would happen; rather specific LogHandlers would dynamically cast that error parameter into various apriori known error types, and handle them specially. That would give us code like this in LogHandlers:
import Foo
import Bar
struct MyLogHandler: LogHandler {
func log(..., error: any Error, ...) {
if let fooError = error as? Foo.FooError {
// extract specific Foo.FooError properties
} else if let barError = error as? Bar.BarError {
// extract specific Bar.BarError properties
} else {
// default handling, extract type and message, at most
}
}
}
While the above might seem okay, or even desirable, it basically breaks the point of the LogHandler protocol and the N + M promise from earlier. The LogHandler suddenly needs to start directly depending on important ecosystem libraries (doesn't matter if from the toolchain or as a package dependency) just to be able to inspect their specific errors, creating a leaky abstraction that undermines type safety of Swift Log.
Passing the Error to the LogHandler is functionally equivalent to passing an opaque "context" object, like:
import Foo
import Bar
struct MyLogHandler: LogHandler {
func log(..., context: any Sendable, ...) {
if let foo = context as? Foo.FooContext {
// extract specific Foo properties
} else if let bar = error as? Bar.BarContext {
// extract specific Bar properties
} else {
// default handling, extract type and message, at most
}
}
}
But in the Swift ecosystem, we've successfully pushed back against passing void* context around, because while it appears to be flexible, it only results in coupling the two sides of the abstraction that are passing such a void * object.
So while an attractive "quick fix", I think it's the wrong model for the long term health of our API packages.
These are the reasons why I'm a strong -1 on poking a hole through LogHandler, as it removes the incentive to work on improving the abstraction itself, and instead it lets anyone pass arbitrary data through it, resulting in creating fractures in the "Swift Log ecosystem", as no longer will every library work with every LogHandler, instead there will appear these implicit, undocumented contracts between specific libraries and specific LogHandlers. And I think that would be a significant regression for the overall ecosystem.
My suggestion would be to evaluate the proposal as written on its own merits, of whether we want a utility on Logger for storing metadata about an Error in publicly documented metadata keys. The community can decide whether that's worth it or not, I'm personally a +1 on that.
Then there can be a separate proposal for poking a hole through LogHandler, however I think I've made my position clear on why I'd be a strong -1 on that. And I'd definitely recommend we don't try to turn the former into the latter, let's use a separate proposal.
I am neutral on the proposal. I think it is really common to log errors and having well defined metadata keys + a convenience methods makes sense to me. However, I do agree with the two points that were brought up so far:
recordError holds its weight. In my opinion, it is enough to have all the log methods that take a new error parameter. (NIT: That parameter should be some Error and not any Error.LogHandler discussion into a future proposal. We can always do that down the line without breaking API but it requires lot more discussion around what concrete log handlers would actually do with the error besides casting it to specific error types.