[Feedback] Server Logging API (with revisions)

[anandabits (Matthew Johnson)]

@johannesweiss

I guess we can now for sure say that this is a good argument for an immutable API.
Irregardless of what languages a programmer is familiar with, reasoning about the immutable API is easy as it requires no distinction between value and reference types.

My understanding from our prior discussion is that the semantics of Logger depend on the semantics of the configured handler, so Logger doesn’t necessarily have value semantics or reference semantics. It has unspecified semantics. That is one of the reasons I argued for using immutable derived loggers with clearly specified semantics that leave the original longer untouched.

Secondarily, as I also mentioned upthread in detail, it isn’t at all clear to me that value semantics makes sense for a logger which by necessity has reference semantics with respect to the log handler. The best you can do is hybrid semantics where the metadata has value semantics and the handler has reference semantics. It seems to me like the most clear and consistent model is reference semantics with immutable metadata.

[Mordil (Nathan Harris)]

@johannesweiss

• Implementation: https://github.com/weissi/swift-server-logging-api-proposal , if accepted, a fresh repository will be created under github.com/apple

Just a quick side question - what was the decision process behind the repo being under the Apple org, and not Swift-Server?

[johannesweiss]

@xwu

What would a no-tradeoffs design look like? I think the community would be very interested in that.

I reckon there would be about four different — incompatible — no-tradeoff designs which would still get us no further.

[xwu (Xiaodi Wu)]

@johannesweiss

I reckon there would be about four different — incompatible — no-tradeoff designs which would still get us no further.

But as the expert heading this up, having synthesized the whole discussion, what is your ultimate no-tradeoff design?

[johannesweiss]

@anandabits

That is one of the reasons I argued for using immutable derived loggers with clearly specified semantics that leave the original longer untouched.

I don’t think that’s a good argument. The reason that technically the semantics are undefined is not because it’s a mutable API. If we wanted we could also ‘enforce’ value semantics by doing protocol LogHandler: AnyObject { ...; func copy() -> LogHandler } and implement CoW in Logger.

The choice mutable API vs. immutable+mutator-factories is a purely syntactical one.

The idea is: We recommend and document value-typed LogHandler implementations and nothing else. But if someone really thinks they need to have a global log level for all LogHandlers, they have an escape hatch.

Btw, even with what you call ‘immutable derived loggers’, implementors can still violate the rules. You can return a fresh instance and still mutate the already existing instances.

[anandabits (Matthew Johnson)]

@johannesweiss

I don’t think that’s a good argument.

You ignored the main thrust of my argument which is that loggers inherently have reference semantics with respect to the output of the logger. It is only the metadata that can be treated with value semantics. Thus the semantics of the logger when used as you recommend is a somewhat strange hybrid of value and reference semantics.

@johannesweiss

Btw, even with what you call ‘immutable derived loggers’, implementors can still violate the rules. You can return a fresh instance and still mutate the already existing instances.

This is only true because the current design defers metadata handling to the LogHandler instead of handling metadata itself, such that it could provide consistent, well-defined semantics for metadata.

[johannesweiss]

@anandabits

Secondarily, as I also mentioned upthread in detail, it isn’t at all clear to me that value semantics makes sense for a logger which by necessity has reference semantics with respect to the log handler.

The LogHandler is actually immutable as in unchangeable, not to be confused with an API that is immutable but properties are still changeable through constructing a new ‘derived’ instance. In other words: irregardless of the API being mutable (var newLogger = logger ; newLogger[metadataKey: “foo”] = “bar”) or immutable (let newLogger = logger.addMetadata(“foo”, “bar”)), metadata can be changed and so can the log level. The LogHandler of a given Logger however cannot be changed. For unchangeable things value semantics and reference semantics are indistinguishable.

So to be more precise: The changeable properties of a Logger have value semantics, ie can be ‘saved’ by assigning a Logger to a fresh variable. And again, I’ll repeat the caveat: this only applies if the implementor of a LogHandler buys into the recommended model of making LogHandler value-types regarding its changeable properties.
And as stated above: this is not a limitation of making Logger’s API mutable but rather by deliberately leaving a door open for people who want only one global log level across all Loggers.

[johannesweiss]

@anandabits

This is only true because the current design defers metadata handling to the LogHandler instead of handling metadata itself, such that it could provide consistent, well-defined semantics for metadata.

Yes. When you say metadata I reckon you mean metadata as used in this proposal + log level, right? In any case, I agree with your statement, it’s one of the deliberate tradeoffs of allowing things like ‘one true global log level’ and other things like being able to dynamically change one subsystems log level (at runtime) through configuration.

I would love to say: Logger owns log level + metadata and we just tell the logging backend what to log but it doesn’t seem quite flexible enough. 100% defined semantics, sounds great.

But if we’re too opinionated we might not be able to support a certain important project (say one of the big frameworks) which would severely hinder adoption I think. Over time we can learn and converge to a good model.

[johannesweiss]

@xwu

But as the expert heading this up, having synthesized the whole discussion, what is your ultimate no-tradeoff design?

I hope to one day implement this as a LogHandler shipping package. But it’d work really well with the proposed API and would be fully within the recommendations .

I also really hope that the ecosystem converges towards best practices regarding logging and then we can tighten the screws and lock down more semantics within the API package.

[neallester]

@johannesweiss

Btw, even with what you call ‘immutable derived loggers’, implementors can still violate the rules. You can return a fresh instance and still mutate the already existing instances.

I don't understand this statement. If the handler, metadata and log level are declared with let in a Logger struct, how is it possible to mutate the already existing instances?

[anandabits (Matthew Johnson)]

@johannesweiss

The LogHandler is actually immutable as in unchangeable

Yes of course, but any log handler that actually does something useful is going to have reference semantics internally. It doesn’t matter that you only store a constant reference to it. If it’s performing IO of any kind reference semantics are involved somewhere. That is the point I am making.

It’s clear that you have accepted the hybrid semantics as a necessary compromise. That’s fine, but let’s not pretend Logger has value semantics across the board even when it is backed by a handler implemented as you recommend.

@johannesweiss

this is not a limitation of making Logger ’s API mutable but rather by deliberately leaving a door open for people who want only one global log level across all Loggers.

If people want a single global log level across all loggers that can be set using any logger then sure.

One thing to keep in mind is that people don’t just have positive requirements. People also have negative requirements, i.e. things they don’t want to be possible in their codebase. The compromises in this design seem to all have been made to accomodate the former at the expense of the latter. We don’t need to rehash the discussion upthread again, but I think it’s worth trying to keep both perspectives in mind.

[anandabits (Matthew Johnson)]

@neallester

I don't understand this statement. If the handler, metadata and log level are declared with let in a Logger struct, how is it possible to mutate the already existing instances?

Metadata and log level are handled by the log handler, not the logger. That’s why their semantics in the current design are undefined.

[johannesweiss]

@neallester

I don't understand this statement. If the handler, metadata and log level are declared with let in a Logger struct, how is it possible to mutate the already existing instances?

For example: A Logger’s log level is whatever its LogHandler returns. So it could just return whatever’s in some global private var rogueLoggingPackageGlobalLogLevel: LogLevel. Even if we changed the API to be let newLogger = oldLogger.setLogLevel(.debug), there’s zero guarantee that oldLogger isn’t also changed because in the current proposal, the log level is controlled by the implementor of the selected LogHandler.

And yes, we could change this and have Logger (which is from the proposed API package) own the log level and therefore the semantics for a change of log level but then we probably won’t be able to express all the ways people want to manage log levels.

[neallester]

@johannesweiss

And yes, we could change this and have Logger (which is from the proposed API package) own the log level and therefore the semantics for a change of log level but then we probably won’t be able to express all the ways people want to manage log levels.

I think this comment hints at the source of my confusion. Above, when you described the requirement

@neallester

there should only be one log level set for the whole program

did you accidentally leave out the additional requirement "and it must be possible to change that single log level for all loggers simultaneously while the system is executing"? If so, I do see how that additional requirement clearly rules out immutable semantics for logger log level (I'm not convinced the necessary compromises are worth it but certainly don't want to re-litigate that question. Clearly, however, application developers deserve as fine grained control over log level as possible. Something which library writers should keep in mind).

It doesn't really explain why the same is true for metadata. Unlike log level, metadata is more "additive".

[hartbit (David Hart)]

I've been reading this thread with a lot of interested and I'd like to add my 2 cents:

Global State

@johannesweiss

xwu:

• Factories are pretty rare in Swift. Unless something really can't be expressed as an initializer, I would agree that every effort ought to be made to use them.

Agreed. But this one I think is really quite important. We could make an initialiser that then takes the lock and reaches out to the global but that's really not very nice. I don't think/I hope that there's just not too many libraries out there that need an awful global like this. But the logging system needs to have one global backend (IMHO, see discussions with @anandabits).

I don't think the fact there is global state is reason for making it visible in the API. The API could be nicer by getting rid of Logging and moving bootstrap and make to Logger (bootstrap can't be moved to LogHandler because its a protocol), like @xwu suggested. See my proposed modified below.

Value Semantics

In the current iteration of the API, the value semantics of Logger are entirely dependent on the value semantics of LogHandler. And I think that's bad for two reasons:

• The value semantics of LogHandler are not guaranteed, and that's very unusual for Swift: you can give value semantics to classes, but it's very unusual for a struct not to have value semantics.
• It's not at all obvious to consumers of the API (it took me some time to grasp it) that the value semantics of Logger dynamically depends on the value semantics of LogHandler, especially as Logger's handler property and initialiser are not public, and therefore not visible in any code completion or documentation: all users see is a simple struct with methods on it.
• Conversely, it's not al all obvious to LogHandler library providers that the value semantics of the type they provide will affect the value semantics of Loggers.

I don't have a good solution to this for now.

Proposal

I've sent a Pull Request https://github.com/weissi/swift-server-logging-api-proposal/pull/41 so people can see what I am proposing. What I did:

• Remove Logging and move its functions to Logger. make becomes an initializer.
• Introduce a argument-less initializer to Logger to initialize it with a global level when people want to use a global logger.
• Remove all factory initializers in example and test code.

Here are examples of what using this new API looks like:

let globalLogger = Logger()
func globalMain() {
    // boostrap with our sample implementation
    Logger.bootstrap(SimpleLogHandler.init)

    // log
    globalLogger.info("Hello")
}

func contextualMain() {
    // boostrap with our sample implementation
    Logger.bootstrap(SimpleLogHandler.init)

    // log

    let logger1 = Logger(label: "context1")
    logger1.info("Hello")

    let logger2 = Logger(label: "context2")
    logger2.info("Hello")
}

public struct SimpleLogHandler: LogHandler {
    private var handler: CommonLogHandler
    private let defaultLogLevel: Logger.Level

    public init(label: String) {
        self.init(label: label, defaultLogLevel: .info)
    }

    public init(label: String, defaultLogLevel: Logger.Level) {
        self.handler = CommonLogHandler(label: label)
        self.defaultLogLevel = defaultLogLevel
    }

    public func log(level: Logger.Level, message: String, metadata: Logger.Metadata?, error: Error?, file: StaticString, function: StaticString, line: UInt) {
        self.handler.log(level: level, message: message, metadata: metadata, error: error) { text in
            print(text)
        }
    }

    public var logLevel: Logger.Level {
        get { return self.handler.logLevel ?? self.defaultLogLevel }
        set { self.handler.logLevel = newValue }
    }

    public var metadata: Logger.Metadata {
        get { return self.handler.metadata }
        set { self.handler.metadata = newValue }
    }

    public subscript(metadataKey metadataKey: String) -> Logger.Metadata.Value? {
        get { return self.handler[metadataKey: metadataKey] }
        set { self.handler[metadataKey: metadataKey] = newValue }
    }
}

[tomerd]

thanks @hartbit et all we are planning to address feedback and finalize the proposal next week

[hartbit (David Hart)]

Let me know once the actual repo is created. I'd love to look at it and provide some feedback then.

[kirilltitov]

Any updates here?

[tomerd]

@johannesweiss @hartbit have been working to address the key feedback and the official repo / package will be published soon

[tkrajacic (Thomas Krajacic)]

One remaining question I have is the intention/purpose of the label on Logger (or the entire Logging framework for that matter).

In my opinion it is valuable to have a (maybe optional?) label for debugging purposes (a bit like giving a DispatchQueue a name), or - if not optional - a "guaranteed metadata" that can be used to identify where a log message came from in a LogHandler.

This however is not implemented in the proposal.

The Logger takes a label (in an initializer in an extension) but doesn't store it (it just uses it for the factory to produce LogHandlers). It is completely unclear to the user, why the Logging system requires a String in the bootstrap method, and for what purpose.
And those LogHandlers don't even have the concept of labels which in turn makes the label somewhat useless.

What is the intention here? What should the label do? What's its intended purpose?