[Feedback] Server Logging API (with revisions)

[johannesweiss]

@anandabits

Thanks for working on this API! I was way too busy to provide feedback when the first draft was shared so I apologize that this is coming late in the process.

Thanks for still sending feedback. We had two discussion threads before (here and there). Tomer and I compiled an API out of that which should allow all existing solutions to migrate to this. This is not the final version of the implementation but the rough shape of this proposal is more or less final and the outcome of previous discussion threads. There are a things I'm personally not very happy with - some of which you mention in your post - but I believe logging is so fundamental that we need to come up with an API that everybody can use and adopt without too much friction. So asking everybody to re-architect would not work I believe. Therefore, we propose the current version to the community and the SSWG will decide whether to move along with this proposal or not.

But: If accepted into the SSWG's process, the resulting package will be a regular open-source SwiftPM package and the code will evolve over time. So implementation issues and details can be addressed whilst the proposed package is going through the Sandbox/Incubation stages of the SSWG process.

@anandabits

one global logger, ie. one application-wise global that's always accessible

I really don't think a library should be introducing global logger or the ability to create on on the fly. If an application wants to use scoped or local loggers it is more difficult to enforce that policy when these facilities are present and trivial to use. I recommend rethinking the approach to supporting global and ad-hoc loggers such that you make it relatively easy for an application to configure that if desired while not requiring every application using this API to make these available to all application code.

I agree with your sentiment and share your opinion. However, after talking to many people and going through two discussion threads it doesn't appear like we can force the ecosystem into some logging model without risking the API package not being widely adopted which is the main important point of an API package. One of the popular packages for example only offers a global logging facility. Other people want one logger per class and configure those with different logging levels so the ability of being able to create loggers is something we need at this stage I believe.

@anandabits

The logLevel property as well as the trace , debug , info , warning , and error methods are probably self-explanatory.

The logLevel property appears to be present to configure the level at which the logger will emit messages. Is that correct? It seems like a really bad idea to expose this as a mutable property on a logger that is passed around, and a really bad idea to expose this on a global logger. Every application I have worked on wants to configure log levels centrally (per logging context) and only allow other code to produce messages (without being able to modify the level at which the logger emits those messages).

Well, for a value-typed LogHandlers that are explicitly passed around I think this is all good. For reference-typed LogHandlers, especially global ones I do agree with your sentiment. I know we're squaring the circle here and trying to get many different ways of logging into one API but we can't ignore what's there right now and the requirements people have. We need to rely on libraries not messing with the log level etc. My hope is that over time best practices will be established and the ecosystem itself evolves and we can evolve the APIs to capture the best practices more precisely.

@anandabits

public var metadata: Logging.Metadata? { get set }

It isn't clear to me why this dictionary is optional. I think there should be pretty strong motivation for making it optional.

johannesweiss:

It's not, that's a typo in the markdown only. I'll fix later today along with other typos. In the first proposal it was indeed optional because we were toying with the idea of allowing LogHandlers to not support metadata at all. But I think that was a bad idea and therefore we removed it.

@anandabits

public subscript(metadataKey metadataKey: String) -> Logging.Metadata.Value? { get set }

It isn't clear to me why we need both the subscript and a mutable property. Given the metadataKey label on the logger it is actually shorter to just use the dictionary directly:

The dictionary is meant as the primary way of storing metadata in your program, ie. the LogHandler may store it however it wants (maybe a dictionary, maybe a tree, whatever). The property that exposes the whole metadata as a dictionary and it's there to support MDC (mapped metadata context) where the context can be saved/restored to/from a thread-local variable. Again, personally not a fan of that but it's a common request that you can use a thread-local to store the metadata and when crossing threads you'd take the metadata out, ship it over to another thread and install it there...

@anandabits

Why are these enum cases indirect ? The don't need to be since the recursive storage is already heap allocated (by the collections)

Thanks, just by accident, will fix in code.

@anandabits

protocol CustomLogStringConvertible {
    var logDescription: String { get }
}

Individual types are expected to ensure that the logDescription does not contain any sensitive data.

I think that might turn out to be a good idea. If this proposal is accepted, I'd be more than interested to work with you on getting that into the codebase before we leave the Sanbox stage. At some point obviously we'll have a public API so breaking changes will be more problematic. But in the early Sandbox stage we won't immediately declare version 1.0.0, so I'd say this is a more than acceptable change. This feels inline with the spirit of the proposal, so we don't need to propose a whole new version for that.

@anandabits

Your Logging.Metadata design currently requires all metadata to have keys. Our system initially required keys for the contextual values but that turned out to be redundant as the logDescription output is self-describing.

One way to combine these approaches would be to define a Logging.messageContextMetadataKey: String constant and provide overloads which package up the "keyless" entry metadata into a MetadataValue.list that is stored in that constant key. The other approach would be to just allow individual log entries to receive a MetadataValue instead of requiring a keyed Logging.Metadata (which is already supported by the MetadataValue enum and would still be available when desired).

That is also something I'd like to talk about should this proposal be accepted.

@anandabits

This brings to mind a couple questions about the behavior of the Logger struct more generally. Specifically, does the metadata get passed by value?

It depends on the LogHandler, squaring the circle...