Skip to content

Conversation

@ktoso
Copy link
Member

@ktoso ktoso commented Jun 10, 2020

Motivation:

  • Developers may have very opinionated and "not global" opinions about loggers
    • I recently talked with a team which might consider wanting to put all logs from the actor system (and things it invokes) into a separate log file, separate from access logs of a vapor app
    • this is doable today, but a bit "un-usual" by overriding the system.settings.logging.overrideLoggerFactory so we've always thought about this, but other teams really like to "pass in a logger" which here becomes a bit weird.

This allows the following patterns, while keeping all our functionality about LogCapture and similar.
Tons of discussions with @weissi led to this current rehash of how we create loggers.

We do not inherently depend on the source addition, but there are some spots where it would be the right thing:

  • for serialization
  • maybe for dead letters
  • for the top level "system logger"

We would not use "source" for actor paths through as those we do want to "inherit" when we'd call some other library, so it has to be a Logger and not a LoggerWithSource.

This rehash allows for the following pattern:

let rootLogger = Logger( ... my special logger, maybe even custom handler)
ActorSystem(...) { settings in 
settings.logging.logger = rootLogger

This logger is used as "template" to create new loggers in the entire system; an actor's logger is a copy of this logger + its actor specific (e.g. path) metadata set when the actor is created.

Modifications:

Removes

//    /// Optionally override Logger that shall be offered to actors and the system.
//    /// This is used instead of globally configured `Logging.Logger()` factories by the actor system.
//    public var overrideLoggerFactory: ((String) -> Logger)?

In favor of

    public var logger: Logger = LoggingSettings.makeDefaultLogger()

    static func makeDefaultLogger() -> Logger {
        Logger(label: "<<ActorSystem>>")
    }

which users may change.

Result:

  • More flexible logging configuration.
  • "label" is a bit useless, we can't control it
  • actor paths are ONLY present in actor/path metadata, nowhere else (they used to be in the label as well, but that was slightly wrong semantically)


/// At what level should the library actually log and print logs // TODO: We'd want to replace this by proper log handlers which allow config by labels
public var defaultLevel: Logger.Level = .info // TODO: maybe remove this? should be up to logging library to configure for us as well
public var logLevel: Logger.Level { // TODO: deprecate
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe... not sure tbh.

When using the "globally set up one" this is useful since one can just set the level easily.


/// Optionally override Logger that shall be offered to actors and the system.
/// This is used instead of globally configured `Logging.Logger()` factories by the actor system.
public var overrideLoggerFactory: ((String) -> Logger)?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is replaced by allowing to pass in the "base logger"

var metadata: Logger.Metadata {
[
"swim/membersToPing": "\(self.membersToPing)",
"swim/membersToPing": Logger.Metadata.Value.array(self.membersToPing.map { "\($0)" }),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provides a nicer printout in our metadata aware LogCapture

}
}

return valueDescription
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small improvement while I was in here, easier to look at nested metadatas now in the LogCapture


if self.captureLogs {
settings.logging.overrideLoggerFactory = capture.loggerFactory(captureLabel: name)
settings.logging.logger = capture.logger(label: name)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the "main" change I guess; we no longer do the override factory but pass in a logger than can be a specific one.

FYI @weissi

@ktoso ktoso added this to the 0.5.0 - Cluster Hardening milestone Jun 10, 2020
@ktoso
Copy link
Member Author

ktoso commented Jun 10, 2020

FYI @budde, with this change it'll be possible to directly "set" the "base" logger on an actor system.
It was previously already possible via setting overrideLoggerFactory if you wanted to have a logger specific to the actor system i.e. "some other file" like we discussed. With this change this is a bit simpler.

@ktoso ktoso merged commit 094ee2e into apple:master Jun 11, 2020
@ktoso ktoso deleted the wip-pass-in-logger branch June 11, 2020 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants