[Proposal] SLG-0003: Standardized Error Metadata via Logger Convenience

Server Development
,
1

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!

3 Likes
2

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.

6 Likes
3

Also, shouldn’t the new signatures take “some Error” not “any Error”?

4

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.

5

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:

  1. It's less discoverable; you sort of have to know it's there rather than having autocomplete toss it in your face.
  2. Logger.Metadata is actually just a typealias of [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:

  1. Composes naturally with APIs that take/provide metadata, so we don't have to remember to add it each time a new API is added. E.g. this proposal doesn't let a MetadataProvider provide an error, but if we went with this design instead it would just be available (setting aside if anyone actually wants that).
  2. If we want to add more options for how the error is recorded in the future, this provides a natural place to put them. E.g. I often add a 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).
6

And then I have a few comments about the chosen metadata keys:

  1. I think error.type is extremely useful and often forgotten. So +1 on that.
  2. I think 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.
7

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?

8

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.

9

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.