remove error argument from logging API

[tomerd]

motivation: since Error in swift does not carry any additional information (such as stack traces) that can be useful in formatting decision the LogHandler may need to do, there is no real reason to treat it as special in the API, and it can just be "strigified" at the calling site

changes: remove error params from all APIs

[tomerd]

@ktoso i have been sleeping on this... since unlike java, the Error in swift does not carry any additional information (such as stack traces) that can be useful in formatting decision the LogHandler may need to do, i now think there is no real reason to treat it as special in the API, and it can just be "strigified" at the calling site. wdyt?

[tomerd]

/cc @weissi

[tomerd]

needs https://github.com/weissi/swift-server-logging-api-proposal/pull/31 first

[ktoso]

Hm hm... So with with the addition of stringConvertible(CustomStringConvertible) from https://github.com/weissi/swift-server-logging-api-proposal/pull/25 the Error would not have to be stringified if users did not want to in the first place... We can carry the Error in metadata without rendering it (since an error COULD contain heavy stuff, it could even contain a backtrace... normally it does not yes, but it could hm; one could say it is abusing the Error type perhaps but it definitely can be useful to do so)...

Having that said I'm not entirely comfortable with this change mostly for reasons that it feels a bit weird to users perhaps. In reality it means that we'd perhaps have to have implicit contracts that "the error key in metadata should contain Error if there was one"... Not sure that's so nice, hard to say; in reality if you'd have a centralized log gathering system perhaps it does not matter much, though for the hello worlds it makes it look very weird so I'm more worried about that than the performance aspect.

[tomerd]

@ktoso i much prefer having it in the api over some convention in metadata. the main argument to remove it is that it normally does not carry expensive data (eg stacktraces), but if its a common practice to put heavy data on it than a descrete argument continues to make sense

@weissi wdyt?

[ktoso]

if its a common practice to put heavy data on it than a descrete argument continues to make sense

It probably isn't common but could happen in some cases (we did so in some places); In general the convention about a "error" key is the nasty part I guess.

[weissi]

@ktoso / @tomerd what's the state here?

[ktoso]

AFAICS we do want to keep the error parameter after all so this proposal would remain unmerged.

[weissi]

I think we should remove the error parameters. I don't think they hold their weight and I don't think os_log would want to add them but we can chat with them.

[ktoso]

Hmm... I see that Vapor also doesn't have such param... https://api.vapor.codes/console/latest/Logging/Protocols/Logger.html

I was wondering if in real it wouldn't cause people to end up carrying errors in metadata then or put it into strings directly -- though it seems the put it into strings directly seems to be the thing people want to do... Without backtraces in (most) errors I see why this might be fine.

Not a deal breaker for me I guess, for errors with known "heavy to render" payloads they can be carried in metadata (though that's the pattern I'm a bit worried of to be honest); For my use cases I could do some proxy log handler which would take care of avoiding the rendering etc.

Your call I guess; worth checking with os_log?

[tomerd]

@ktoso @weissi i suggest we remove for the initial release then bring up with the community in the forums as follow. easier to add later than remove later

[ktoso]

Sounds good 👍

[tomerd]

@weissi @ktoso rebased

[hartbit]

For what it’s worth, I agree with this decision. Error is so application specific that generic LogHandler will never really know what to do with it. Best to leave its stringification to the user for now.

[weissi]

sorry @tomerd , I made this conflict :(

[tomerd]

@weissi rebased again