diff --git a/Sources/RediStack/RedisClient.swift b/Sources/RediStack/RedisClient.swift index 957099e..0482155 100644 --- a/Sources/RediStack/RedisClient.swift +++ b/Sources/RediStack/RedisClient.swift @@ -30,11 +30,6 @@ public protocol RedisClient { /// The client's configured default logger instance, if set. var defaultLogger: Logger? { get } - /// Overrides the default logger on the client with the provided instance for the duration of the returned object. - /// - Parameter logger: The logger instance to use in commands on the returned client instance. - /// - Returns: A client using the temporary default logger override for command logging. - func logging(to logger: Logger) -> RedisClient - /// Sends the given command to Redis. /// - Parameters: /// - command: The command to send to Redis for execution. diff --git a/Sources/RediStack/RedisConnection.swift b/Sources/RediStack/RedisConnection.swift index 93bdaeb..cb9a12a 100644 --- a/Sources/RediStack/RedisConnection.swift +++ b/Sources/RediStack/RedisConnection.swift @@ -364,10 +364,6 @@ extension RedisConnection { // MARK: Logging extension RedisConnection { - public func logging(to logger: Logger) -> RedisClient { - return CustomLoggerRedisClient(defaultLogger: self.prepareLoggerForUse(logger), client: self) - } - private func prepareLoggerForUse(_ logger: Logger?) -> Logger { guard var logger = logger else { return self.defaultLogger } logger[metadataKey: RedisLogging.MetadataKeys.connectionID] = "\(self.id)" diff --git a/Sources/RediStack/RedisConnectionPool.swift b/Sources/RediStack/RedisConnectionPool.swift index 1de6a71..f92dc09 100644 --- a/Sources/RediStack/RedisConnectionPool.swift +++ b/Sources/RediStack/RedisConnectionPool.swift @@ -174,7 +174,8 @@ extension RedisConnectionPool { /// For example: /// ```swift /// let countFuture = pool.leaseConnection { - /// let client = $0.logging(to: myLogger) + /// client in + /// /// return client.authorize(with: userPassword) /// .flatMap { connection.select(database: userDatabase) } /// .flatMap { connection.increment(counterKey) } @@ -324,10 +325,6 @@ extension RedisConnectionPool { // MARK: RedisClient extension RedisConnectionPool: RedisClient { public var eventLoop: EventLoop { self.loop } - - public func logging(to logger: Logger) -> RedisClient { - return CustomLoggerRedisClient(defaultLogger: logger, client: self) - } public func send( _ command: RedisCommand, diff --git a/Sources/RediStack/RedisLogging.swift b/Sources/RediStack/RedisLogging.swift index d113133..ca1f1e6 100644 --- a/Sources/RediStack/RedisLogging.swift +++ b/Sources/RediStack/RedisLogging.swift @@ -65,57 +65,3 @@ extension Logger { /// The prototypical instance used for Redis connection pools. public static var redisBaseConnectionPoolLogger: Logger { RedisLogging.baseConnectionPoolLogger } } - -// MARK: RedisClient Logger Overrides - -/// This is an implementation detail of baseline RediStack RedisClients that stores a reference to an underlying -/// RedisClient and a given logger instance, which is used as a new default logger on all commands. -internal struct CustomLoggerRedisClient: RedisClient { - internal var eventLoop: EventLoop { self.client.eventLoop } - - internal let defaultLogger: Logger - - private let client: Client - - internal init(defaultLogger: Logger, client: Client) { - self.defaultLogger = defaultLogger - self.client = client - } - - // create a new instance by just reusing the same client and passing the new logger instance - - internal func logging(to logger: Logger) -> RedisClient { - return Self(defaultLogger: logger, client: client) - } - - // forward methods to the underlying client - - // in each case we need to explicitly create a logger variable using the provided logger argument, defaulting to - // the default logger if the argument is nil, because if we do it inline, the compiler will deduce the type - // as optional, allowing the (possibly) nil argument to pass through without providing the default logger in nil cases - - internal func send(_ command: RedisCommand, eventLoop: EventLoop?, logger: Logger?) -> EventLoopFuture { - let logger = logger ?? self.defaultLogger - return self.client.send(command, eventLoop: eventLoop, logger: logger) - } - - internal func unsubscribe(from channels: [RedisChannelName], eventLoop: EventLoop?, logger: Logger?) -> EventLoopFuture { - let logger = logger ?? self.defaultLogger - return self.client.unsubscribe(from: channels, eventLoop: eventLoop, logger: logger) - } - - internal func punsubscribe(from patterns: [String], eventLoop: EventLoop?, logger: Logger?) -> EventLoopFuture { - let logger = logger ?? self.defaultLogger - return self.client.punsubscribe(from: patterns, eventLoop: eventLoop, logger: logger) - } - - internal func subscribe(to channels: [RedisChannelName], eventLoop: EventLoop?, logger: Logger?, _ receiver: @escaping RedisPubSubEventReceiver) -> EventLoopFuture { - let logger = logger ?? self.defaultLogger - return self.client.subscribe(to: channels, eventLoop: eventLoop, logger: logger, receiver) - } - - internal func psubscribe(to patterns: [String], eventLoop: EventLoop?, logger: Logger?, _ receiver: @escaping RedisPubSubEventReceiver) -> EventLoopFuture { - let logger = logger ?? self.defaultLogger - return self.client.psubscribe(to: patterns, eventLoop: eventLoop, logger: logger, receiver) - } -} diff --git a/Sources/RediStackTestUtils/Extensions/RediStack.swift b/Sources/RediStackTestUtils/Extensions/RediStack.swift index e8168ad..3d62800 100644 --- a/Sources/RediStackTestUtils/Extensions/RediStack.swift +++ b/Sources/RediStackTestUtils/Extensions/RediStack.swift @@ -13,6 +13,7 @@ //===----------------------------------------------------------------------===// import Foundation +import Logging import NIOCore import RediStack @@ -23,9 +24,10 @@ extension RedisConnection.Configuration { public init( host: String = RedisConnection.Configuration.defaultHostname, port: Int = RedisConnection.Configuration.defaultPort, - password: String? = nil + password: String? = nil, + defaultLogger: Logger? = nil ) throws { - try self.init(hostname: host, port: port, password: password) + try self.init(hostname: host, port: port, password: password, defaultLogger: defaultLogger) } } diff --git a/Tests/RediStackIntegrationTests/RedisLoggingTests.swift b/Tests/RediStackIntegrationTests/RedisLoggingTests.swift index 03d6543..cf9278d 100644 --- a/Tests/RediStackIntegrationTests/RedisLoggingTests.swift +++ b/Tests/RediStackIntegrationTests/RedisLoggingTests.swift @@ -24,8 +24,7 @@ final class RedisLoggingTests: RediStackIntegrationTestCase { let handler = TestLogHandler() let logger = Logger(label: #function, factory: { _ in return handler }) _ = try self.connection - .logging(to: logger) - .ping() + .ping(logger: logger) .wait() XCTAssertFalse(handler.events.isEmpty) } @@ -34,11 +33,21 @@ final class RedisLoggingTests: RediStackIntegrationTestCase { let defaultHandler = TestLogHandler() let defaultLogger = Logger(label: #function, factory: { _ in return defaultHandler }) + let connection = try RedisConnection.make( + configuration: .init( + host: self.redisHostname, + port: self.redisPort, + password: self.redisPassword, + defaultLogger: defaultLogger + ), + boundEventLoop: self.connection.eventLoop + ).wait() + defaultHandler.events = [] // empty out events from initialization + let expectedHandler = TestLogHandler() let expectedLogger = Logger(label: "something_else", factory: { _ in return expectedHandler }) - _ = try self.connection - .logging(to: defaultLogger) + _ = try connection .ping(logger: expectedLogger) .wait() @@ -51,8 +60,7 @@ final class RedisLoggingTests: RediStackIntegrationTestCase { let logger = Logger(label: #function, factory: { _ in return handler }) _ = try self.connection - .logging(to: logger) - .ping() + .ping(logger: logger) .wait() XCTAssertEqual( handler.metadata[RedisLogging.MetadataKeys.connectionID], @@ -76,8 +84,7 @@ final class RedisLoggingTests: RediStackIntegrationTestCase { pool.activate() _ = try pool - .logging(to: logger) - .ping() + .ping(logger: logger) .wait() XCTAssertTrue(handler.metadata.keys.contains(RedisLogging.MetadataKeys.connectionID)) XCTAssertEqual( @@ -108,8 +115,7 @@ final class RedisLoggingTests: RediStackIntegrationTestCase { hosts.register("default.local", instances: [address]) _ = try client - .logging(to: logger) - .ping() + .ping(logger: logger) .wait() XCTAssertTrue(handler.metadata.keys.contains(RedisLogging.MetadataKeys.connectionID)) XCTAssertEqual( @@ -132,6 +138,7 @@ final class TestLogHandler: LogHandler { func log(level: Logger.Level, message: Logger.Message, metadata: Logger.Metadata?, file: String, function: String, line: UInt) { self.events.append((level, message, metadata, file, function, line)) + // print(message, dump(metadata), file, function, line) } subscript(metadataKey key: String) -> Logger.Metadata.Value? { diff --git a/docs/api-design/Logging.md b/docs/api-design/Logging.md index da13265..485bcd3 100644 --- a/docs/api-design/Logging.md +++ b/docs/api-design/Logging.md @@ -28,24 +28,10 @@ to as [_Protocol-based Context Passing_](https://forums.swift.org/t/the-context- ```swift // example code, may not reflect current implementation -private struct CustomLoggingRedisClient: RedisClient { - // a client that this object will act as a context proxy for - private let client: RedisClient - private let logger: Logger - /* conformance to RedisClient protocol */ -} - -extension RedisClient { - public func logging(to logger: Logger) -> RedisClient { - return CustomLoggingRedisClient(client: self, logger: logger) - } -} - let myCustomLogger = ... let connection = ... -connection - .logging(to: myCustomLogger) // will use this logger for all 'user-space' logs for any requests made - .ping() +// will use this logger for all 'user-space' logs generated while serving this command +connection.ping(logger: myCustomLogger) ``` ## Log Guidelines @@ -53,18 +39,18 @@ connection 1. Prefer logging at `trace` levels 1. Prefer `debug` for any log that contains metadata, especially complex ones like structs or classes - exceptions to this guideline may include metadata such as object IDs that are triggering the logs -1. Dynamic values should be attached as metadata rather than string interpolated -1. All log metadata keys should be added to the `RedisLogging` namespace -1. Log messages should be in all lowercase, with no punctuation preferred - - if a Redis command keyword (such as `QUIT`) is in the log message, it should be in all caps -1. `warning` logs should be reserved for situations that could lead to `error` or `critical` conditions - - this may include leaks or bad state +1. Dynamic values SHOULD be attached as metadata rather than string interpolated +1. All log metadata keys SHOULD be added to the `RedisLogging` namespace +1. Log messages SHOULD be in all lowercase, with no punctuation preferred + - if a Redis command keyword (such as `QUIT`) is in the log message, it MUST be in all caps +1. `warning` logs SHOULD be reserved for situations that could lead to `error` or `critical` conditions + - this MAY include leaks or bad state 1. Only use `error` in situations where the error cannot be expressed by the language, such as by throwing an error or failing `EventLoopFuture`s. - this is to avoid high severity logs that developers cannot control and must create filtering mechanisms if they want to ignore emitted logs from **RediStack** 1. Log a `critical` message before any `preconditionFailure` or `fatalError` ### Metadata -1. All keys should have the `rdstk` prefix to avoid collisions -1. Public metadata keys should be 16 characters or less to avoid as many String allocations as possible -1. Keys should be computed properties to avoid memory costs +1. All keys SHOULD have the `rdstk` prefix to avoid collisions +1. Public metadata keys SHOULD be 16 characters or less to avoid as many String allocations as possible +1. Keys SHOULD be computed properties to avoid memory costs