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

[samuelmurray]

Add standardized way of attaching data from Error instances to log posts.

Motivation:

Addresses #291

Modifications:

The "SLG-0003" proposal doc added.

Result:

The proposal is ready for review.

[samuelmurray]

I have some thoughts:

1. I’m not sure if it’s more appropriate to add extension methods, or modify the existing methods to take an Error? that defaults to nil, i.e. func log(…, error: Error? = nil, …)
2. Concerning the serialization:
   1. Is there reason to add ”error.*” as part of the API (to future proof/discourage other usage) or only ”error.message" and ”error.type”
   2. Is it more sensible to serialize as [”error.message”: …, “error.type”: ...], or [”error”: [“message”: …, “type”: …]]

[AllDmeat]

It would be great if LogHandler could accept Error objects directly. That would let us forward non-fatal errors to Crashlytics (or similar services) and improve monitoring in apps.

[samuelmurray]

I agree, however that's out of scope for this proposal. That was my original idea in #384, but as per suggested in #384 (comment) we split it in two phases to help moving forward. That is, if/when this is implemented, I might add a follow-up proposal, though from the discussion in my previous PR it’s not obvious if that’s a direction swift-log wants to go in.

[czechboy0]

I think we should also add to this proposal a way to set the error metadata on a Logger:

extension Logger {
    public mutating func recordError(_ error: any Error) {
        self[metadataKey: "error.type"] = "\(String(reflecting: type(of: error)))"
        self[metadataKey: "error.message"] = "\(error)"
    }
}

This means the user can do any of the following:

let error: any Error = ...
let logger: Logger = ...

// Option 1:
logger.info("Stuff went wrong, error: error)

// Option 2:
logger.recordError(error)
logger.info("Stuff went wrong")

We generally want to allow adopters to attach metadata on the logger as well, to make it easier to pass loggers with appropriate metadata to be passed into nested functions.

[samuelmurray]

Should I add it already at this stage, or would that be part of the review?

[kukushechkin]

Thank you for taking time to prepare the proposal! I have left a few suggestions and a few questions to discuss before starting the formal review process.

[kukushechkin]

We usually put all the public API changes in the Detailed Design section, as they would be in the code with the accompanying doc strings.

[samuelmurray]

I’m not sure what this comment suggests - these are in Detailed design. Did you mean to say Proposed solution?

[kukushechkin]

Oh, sorry for being unclear. I suggest adding the new methods and their docc comments, as this is also part of the "public interface" of the package.

[samuelmurray]

I see, added it now! Takes quite a bit of space with all the different methods 😅

[kukushechkin]

@samuelmurray

1. I’m not sure if it’s more appropriate to add extension methods, or modify the existing methods to take an Error? that defaults to nil, i.e. func log(…, error: Error? = nil, …)

Please, keep them as separate methods. Changing the method signature is strictly speaking an API breakage, even if default value keeps code compilable.

2. Concerning the serialization:

   1. Is there reason to add ”error.*” as part of the API (to future proof/discourage other usage) or only ”error.message" and ”error.type”
   2. Is it more sensible to serialize as [”error.message”: …, “error.type”: ...], or [”error”: [“message”: …, “type”: …]]

error.message and error.type are easier to communicate as the API surface. I am not quite sure about the performance implications of parsing extra nested json vs parsing dot-separated keys in the external systems, might be a good question for the broader audience during the review, but worst case scenario a Log Handler specific for an intake system that cares can combine construct nested dicts, while we keep the API smaller.

[samuelmurray]

Thank you for taking time to prepare the proposal! I have left a few suggestions and a few questions to discuss before starting the formal review process.

@kukushechkin thanks for all your suggestions, really appreciate the help!

[kukushechkin]

There is a good chance this enum will be extended with more keys in the future, so it should be up for backward compatible evolution. One option is @nonexhaustive, but this limits the supported Swift versions. Or a struct with static properties similar to this. I would lean towards the second option as LogHandler would not need to match over all the special keys supported by the Logger, but would rather reference a few interesting ones.

What do you think @czechboy0?