Expose an API for configuring basic stdio log handling #61

allenhumphreys · 2019-04-17T14:45:48Z

Expose StreamLogHandler to the public which replaces the previously internal StdoutLogHandlers. Users can choose now the output stream, while stdout is the default.

Motivation:

As discussed on #51 and #60

Modifications:

Rename StdoutLogHandler to StreamLogHandler and provide 2 factory methods standardOutput(label:) and standardError(label:) for using during bootstrapping.

Result:

Users who adopt other logging backends during LoggingSystem bootstrapping will still be able to use the default log handler to get very basic console output to their choice of stdout or stderr.

Example:

// Log to both stderr and stdout for good measure
LoggingSystem.bootstrap { 
    MultiplexLogHandler([
        FancyLoggingBackend(label: $0),
        StreamLogHandler.standardError(label: $0), 
        StreamLogHandler.standardOutput(label: $0)
    ])
}

swift-server-bot · 2019-04-17T14:45:50Z

Can one of the admins verify this patch?

swift-server-bot · 2019-04-17T14:45:51Z

Can one of the admins verify this patch?

ktoso

LGTM, thanks a lot for spotting it and the PR! 👍

tomerd · 2019-04-17T14:58:06Z

@swift-server-bot test this please

tomerd · 2019-04-17T19:14:06Z

we may want to finalize #60 before making it public?

tomerd · 2019-04-18T18:09:18Z

@allenhumphreys would you mind changing this PR following the suggestion in #60 (comment), ie create a public generic StdioLogHandler that can be configured to log to either stdout or stderr

allenhumphreys · 2019-04-18T18:42:19Z

@tomerd I can give it a shot. I briefly looked into the correct way to do this in Swift the other day, stdout and stderr are UnsafeMutablePointer<FILE>. I think I can effectively just replace print(...) with fputs(..., stderr|stdout). That about the gist of it?

allenhumphreys · 2019-04-18T18:47:22Z

Digging into the stdlib a little bit.. using something like:

extension UnsafeMutablePointer: TextOutputStream where Pointee == FILE {
    public mutating func write(_ string: String) {
        fputs(string, self)
    }
}

with

print(..., to: &stderr)

might be better but I don't know why you can't print to stdout as part of the stdlib in the first place.

tomerd · 2019-04-18T19:06:09Z

@allenhumphreys wdyt about what @weissi suggested in https://github.com/apple/swift-log/pull/60/files#diff-259e1bfbf733f7f456500e470bf358cdR501

allenhumphreys · 2019-04-18T19:09:33Z

Oh good point, I hadn't seen that portion of it! Thank you!

Sources/Logging/Logging.swift

tomerd

this is shaping up nicely. couple of comments. add some tests too?

allenhumphreys · 2019-04-24T02:17:07Z

@tomerd Tests will have to come on a different day!

weissi

Thank you! That looks great.

Sources/Logging/Logging.swift

tomerd · 2019-04-25T05:52:01Z

@swift-server-bot test this please

tomerd · 2019-04-25T05:55:53Z

@allenhumphreys looks like tests are failing on linux. you can run them locally with docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.1804.50.yaml run test

weissi · 2019-04-25T10:08:03Z

Sources/Logging/Logging.swift

-internal struct StdoutLogHandler: LogHandler {
+public struct StdioLogHandler: LogHandler {
+
+    public enum Stream {


@allenhumphreys just as a question if you think this is useful. We could also do the following here: Instead of an enum Stream we could do

// this is just to prevent name clashes further down #if os(macOS) || os(tvOS) || os(iOS) || os(watchOS) let sysStderr = Darwin.stderr let sysStdout = Darwin.stdout #else let sysStderr = Glibc.stderr let sysStdout = Glibc.stdout #endif public struct Stream { private let underlying: UnsafePointer<FILE> internal init(underlying: UnsafePointer<FILE>) { self.underlying = underlying } public static let stderr: Stream = .init(underlying: sysStderr) public static let stdout: Stream = .init(underlying: sysStdout) }

that way, the API is the same for the user: StdioLogHandler(label: "foo", stream: .stdout) will still work but we have two extra benefits:

in tests, we can use StdioLogHandler(label: "foo", stream: .init(underlying: fopen(".....", "w+")))

if we chose to allow other streams in the future, then we can add them without breaking public API. The problem with enums is: Adding a case breaks the API because someone might have done an exhaustive switch over it. The struct + static let solution from above alleviates that issue.

Thanks @weissi , this is good, but I'd like to propose going a little further.

Make Stream wrap TextOutputStream instead

Rename StdioLogHandler to StreamLogHandler

Go ahead and expose the initializer for Stream

It'd look something like this:

public struct StreamLogHandler: LogHandler { public struct Stream: TextOutputStream { private let underlying: TextOutputStream public init(underlying: TextOutputStream) { self.underlying = underlying } public mutating func write(_ string: String) { var underlying = self.underlying underlying.write(string) } public static let stderr: Stream = .init(underlying: FileOutputStream(file: sysStderr)) public static let stdout: Stream = .init(underlying: FileOutputStream(file: sysStdout)) } private let lock = Lock() public let stream: Stream public init(label: String, stream: Stream = .stdout) { self.stream = stream } ... ... }

FileOutputStream would remain internal, but clearly available as a reference for consumers. This would allow people to immediately swap in their own streams. It eliminates the need to work with Pointers, or even the c standard library at all. Stream ends up's up just being a box and a convenient way to get access to stderr and stdout in a way that hides the UnsafeMutablePointer<FILE> stuff.

Thoughts? Too far?

@allenhumphreys Thanks, that sounds like a great idea to me. I think you can remove the Lock because if the user wants locking, they could implement that themselves, no?

Seems like a good idea 👍

@allenhumphreys Thanks, that sounds like a great idea to me. I think you can remove the Lock because if the user wants locking, they could implement that themselves, no?

Is that a safe default? in case a concurrent program would use the default loggers shipped?

@allenhumphreys ah, you can remove that lock. LogHandlers are supposed to have value semantics for the configuration (metadata & logLevel) and indeed your handler has, so there's no need to lock.

@weissi Thanks for pointing that out, it looks like the lock was a holdover from when StdoutLogHandler was a class, changed at 1d98315

@allenhumphreys thank you! Apologies, for some reason I thought you added the lock. I'd propose to just remove it because it's baggage from the past :).

In light of discovering how Swift.print functions, I tweaked the API again. I removed the wrapper type StreamLogHandler.Stream and exposed StdioOutputStream to allow easily switching between stderr and stdio. StdioOutputStream is not publicly instantiable, though, so the usage is still restricted. I believe this API is now much more straightforward as it just uses TextOutputStream!

@allenhumphreys perfect. I think it you make the output streams non-public, then we can merge this.

Tests/LoggingTests/LoggingTest.swift

allenhumphreys · 2019-04-29T18:29:15Z

@tomerd @ktoso @weissi I have updated the implementation, the documentation I've proposed isn't complete yet, but I've laid out the general direction. I've left the locking behavior the same per my last comment on the thread.

Thanks everyone for continuing to help evolve the direction of this change.

Sources/Logging/Logging.swift

tomerd · 2019-04-30T00:48:13Z

@swift-server-bot test this please

Sources/Logging/Logging.swift

weissi · 2019-04-30T14:05:52Z

Sources/Logging/Logging.swift

+/// A wrapper to facilitate `print`-ing to stderr and stdio that
+/// ensures access to the underlying `FILE` is locked to prevent
+/// cross-thread interleaving of output.
+public struct StdioOutputStream: TextOutputStream {


The StdioOutputStream we should leave internal or private as that's a thing the stdlib should really provide.

Yeah, I have a similar feeling but keeping it private requires some kind of additional wrapper type or switch to power StreamLogHandler's public initializer (fundamentally providing the consumer the ability to switch between stderr and stdout).

As I had it before StreamLogHandler.Stream also conformed to TextOutputStream.

Previously it was

public static let stderr: Stream = .init(underlying: FileOutputStream(file: systemStderr))

now this has the same level of access:

public static let stdout = StdioOutputStream(file: systemStdout)

but with less indirection.

Ultimately something has to be public in that initializer because one of the goals of this change is to allow easily redirected to stderr. Alternatively, to keep the type private, I could have multiple specialized initializers or possibly static factory methods to provide this functionality.

@allenhumphreys if you overload the init of StreamLogHandler instead of using default arguments, it'll work. All the streams stuff can stay internal/private. So

public struct StreamLogHandler { ... public init(label: String) { self.init(stream: StdioOutputStream(stream: sysStdout)) } public init(label: String, stream: TextOutputStream) { ... } ... }

That doesn't leave a good name for an init that goes to stderr, any suggestion good or not as good?
Just thinking out loud:

init(toStdoutWithLabel label: String) init(toStderrWithLabel label: String)

@allenhumphreys Ahh, I see! I'd then go with

public struct StreamLogHandler { public static func makeStdoutLogHandler(label: String) -> StreamLogHandler { ... } public static func makeStderrLogHandler(label: String) -> StreamLogHandler { ... } ... }

and then we can even leave the init private

resolves apple#51

weissi

Awesome, thanks! I'm happy with this.

weissi · 2019-04-30T17:01:32Z

@swift-server-bot test this please

weissi · 2019-04-30T17:02:14Z

@ktoso / @tomerd / @ianpartridge I'm happy exactly as is, would you mind having a quick look? Just because there were a lot of changes.

tomerd · 2019-04-30T17:11:12Z

Sources/Logging/Logging.swift


-    public var logLevel: Logger.Level {
+    /// Factory that makes a `StreamLogHandler` to directs its output to `stdout`
+    public static func makeStdoutLogHandler(label: String) -> StreamLogHandler {


naming nit: i like the new factories but dont love the names. maybe makeStdout and makeStderr is enough. in any case not a big deal, so can go with this too

You're right, it is a bit repetitive in the full context StreamLogHandler.makeStdoutLogHandler, happy to change. While you're considering names, I made up the StreamLogHandler name to convey it's new expanded capabilities, but it could just as well be TextOutputStreamLogHandler :-)

naming is hard. @weissi any preferences?

@tomerd not really but I agree with both of you that makeStdoutLogHandler is a bit long and redundant. We could even do something like standardOutput only. Then it would read

Logging.bootstrap(StreamLogHandler.standardOutput)

makeStdout sounds like it's making a 'stdout' to me...

tomerd

lgtm and thank you @allenhumphreys

weissi

AWESOME, thanks, this looks great!

weissi · 2019-05-01T15:42:22Z

@swift-server-bot test this please

tomerd · 2019-05-01T16:15:51Z

<3

ktoso approved these changes Apr 17, 2019

View reviewed changes

weissi added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Apr 17, 2019

allenhumphreys force-pushed the ah/stdoutlogger-public branch from 6f789c6 to 6cb3f5a Compare April 24, 2019 01:21

tomerd reviewed Apr 24, 2019

View reviewed changes

Sources/Logging/Logging.swift Outdated Show resolved Hide resolved

tomerd reviewed Apr 24, 2019

View reviewed changes

Sources/Logging/Logging.swift Outdated Show resolved Hide resolved

tomerd reviewed Apr 24, 2019

View reviewed changes

Sources/Logging/Logging.swift Outdated Show resolved Hide resolved

tomerd requested changes Apr 24, 2019

View reviewed changes

tomerd requested a review from ktoso April 24, 2019 02:06

weissi approved these changes Apr 24, 2019

View reviewed changes

weissi added this to the 1.1.0 milestone Apr 24, 2019

weissi reviewed Apr 24, 2019

View reviewed changes

Sources/Logging/Logging.swift Show resolved Hide resolved

tomerd approved these changes Apr 25, 2019

View reviewed changes

tomerd mentioned this pull request Apr 25, 2019

RFC: log to stderr not stdout [might not take this one] #60

Closed

weissi reviewed Apr 25, 2019

View reviewed changes

Tests/LoggingTests/LoggingTest.swift Outdated Show resolved Hide resolved

allenhumphreys force-pushed the ah/stdoutlogger-public branch from 3dfe984 to 1d4fe09 Compare April 29, 2019 18:29

allenhumphreys commented Apr 29, 2019

View reviewed changes

Sources/Logging/Logging.swift Show resolved Hide resolved

allenhumphreys commented Apr 30, 2019

View reviewed changes

Sources/Logging/Logging.swift Show resolved Hide resolved

weissi reviewed Apr 30, 2019

View reviewed changes

allenhumphreys changed the title ~~Expose StdoutLogHandler to the public~~ Expose an API for configuring basic stdio log handling Apr 30, 2019

allenhumphreys added 10 commits April 30, 2019 10:58

Expose StdoutLogHandler to the public

1e01fa0

resolves apple#51

Refactor StdoutLogHandler to StdioLogHandler

4b9489f

Simplify initializers

7264a01

Move wrapper creation to initializer

13c4d71

Put wrapper initializer back at the callsite

c217976

Lock FILE for writing, test output stream selection

3a33b76

Refactor into StreamLogHandler, provide initial documentation expansion

8274ff0

Remove unnecessary Lock on value type StreamLogHandler

896914d

Write directly to TextOutputStream, update API accordingly

e53edc0

Final API refinement limiting the exposed API surface

bc40921

allenhumphreys force-pushed the ah/stdoutlogger-public branch from 964e833 to bc40921 Compare April 30, 2019 15:58

weissi approved these changes Apr 30, 2019

View reviewed changes

weissi requested a review from tomerd April 30, 2019 17:01

tomerd reviewed Apr 30, 2019

View reviewed changes

tomerd approved these changes Apr 30, 2019

View reviewed changes

allenhumphreys and others added 2 commits May 1, 2019 10:24

Rename factory methods

92b3daa

Update README.md

3427b97

weissi approved these changes May 1, 2019

View reviewed changes

weissi merged commit ea3eea9 into apple:master May 1, 2019

weissi mentioned this pull request May 1, 2019

StdoutLogHandler should be public #51

Closed

Expose an API for configuring basic stdio log handling #61

Expose an API for configuring basic stdio log handling #61

Conversation

allenhumphreys commented Apr 17, 2019 • edited by weissi Loading

Motivation:

Modifications:

Result:

Example:

swift-server-bot commented Apr 17, 2019

swift-server-bot commented Apr 17, 2019

ktoso left a comment

Choose a reason for hiding this comment

tomerd commented Apr 17, 2019

tomerd commented Apr 17, 2019

tomerd commented Apr 18, 2019

allenhumphreys commented Apr 18, 2019

allenhumphreys commented Apr 18, 2019

tomerd commented Apr 18, 2019 • edited Loading

allenhumphreys commented Apr 18, 2019

tomerd left a comment

Choose a reason for hiding this comment

allenhumphreys commented Apr 24, 2019

weissi left a comment • edited Loading

Choose a reason for hiding this comment

tomerd commented Apr 25, 2019

tomerd commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenhumphreys commented Apr 29, 2019

tomerd commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi commented Apr 30, 2019

weissi commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi commented May 1, 2019

tomerd commented May 1, 2019

allenhumphreys commented Apr 17, 2019 •

edited by weissi

Loading

tomerd commented Apr 18, 2019 •

edited

Loading

weissi left a comment •

edited

Loading