[Feedback] Server Logging API (with revisions)

(Johannes Weiss) #88

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.

(Johannes Weiss) #89

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 :slight_smile:.

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.

(Neal Lester) #90

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?

(Matthew Johnson) #91

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.

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.

2 Likes
(Matthew Johnson) #92

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

1 Like
(Johannes Weiss) #93

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.

(Neal Lester) #94

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

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".

1 Like
(David Hart) #95

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

Global State

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 }
    }
}
#96

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

(David Hart) #97

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

1 Like
(Kirill Titov) #98

Any updates here?

#99

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

(Thomas Krajacic) #100

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?

(Johannes Weiss) #101

The intention was just that a LogHandler can use the label if it chooses too. But I agree we could have a property to be able to read the label back on Logger which would take it from a to-be-created property on LogHandler.

(Thomas Krajacic) #102

I created a Pull Request to add a label property on LogHandler similar to DispatchQueue.

I am not sure though if it makes more sense in the context of logging to make this property non-optional.

1 Like
(Neal Lester) #103

Would you provide an example use case for this label property? In general wouldn't the owner of a logger already know the label which was used to create the logger (since the owner would have provided the label to the factory)?

(Thomas Krajacic) #104

I guess it can be useful in debugging for example (just like with dispatch queues).
I have used it to designate the subsystem where I create the logger (e.g. let logger Logger(label: "com.app.apiclient")) but I understand that this could also be handled with metadata.

My main point was that since the framework clearly has a notion of labels in the Logging API, there should be a way to read it back out.

(Johannes Weiss) #105

Just to give an update here that the official repo is on track and we'll open that very soon which will then open the development of the OSS project. I'll post another link here very soon.

One other thing that I should have posted here is that there's a separate discussion about the log levels for os_log and this logging API going on in the forums. The logging API will adopt the outcome of that, it very much looks like it'll be the syslog log levels.

4 Likes
(Johannes Weiss) #106

First of all: Sorry for the delay!

After the SSWG accepted this proposal to the 'sandbox' stage, we finally followed through and opened the repository :slight_smile:. So we can now officially kick off the open-source swift-log project!

The swift-log repository got seeded from the pitch/proposal repository which was under my personal github alongside a few changes:

  • adoption of the log levels as discussed in the separate thread
  • name: swift-log
  • proper licensing (Apache 2, as proposed)
  • rename the Logging type to LoggingSystem so we don't have a clash between the module name (Logging) and the type to bootstrap your loggers
  • added a readme and some API docs
  • removed the proposal/examples
  • just like proposed for osLog, to leverage the best performance and have more unification we switched the log messages' type from String to Logger.Message which is a struct that can be created using any string literal + string interpolation. Don't worry, it looks & feels just like a String (logger.info("hello world"))
  • require Swift 5 (required for the new string interpolation support)
  • a bunch of smaller changes

Thank you so much for all the contributions! Now more than ever: please keep your awesome contributions coming, this is only the start of swift-log and swift-log is only the start of a logging that works across the whole ecosystem.

9 Likes
closed #107