#115 -- Remove logging(to:) method

Author: Mordil

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.

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

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<CommandResult>(
         _ command: RedisCommand<CommandResult>,

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<Client: RedisClient>: 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<CommandResult>(_ command: RedisCommand<CommandResult>, eventLoop: EventLoop?, logger: Logger?) -> EventLoopFuture<CommandResult> {
-        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<Void> {
-        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<Void> {
-        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<Void> {
-        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<Void> {
-        let logger = logger ?? self.defaultLogger
-        return self.client.psubscribe(to: patterns, eventLoop: eventLoop, logger: logger, receiver)
-    }
-}

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)
     }
 }
 

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? {

docs/api-design/Logging.md

@@ -28,43 +28,29 @@ 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
 
 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