Allow client error delegate to have a logger injected (#654)

Motivation:

When errors are logged it's useful to have some contextual information.
We can enable this by injecting a `logger` into the error delegate.

Modifications:

Change the API for the `ClientErrorDelegate` to also pass in a logger.
Remove additional logging from next to the call to the delegate.

Result:

Users can log errors with more contextual information in the delegate.

Sources/GRPC/ClientConnection.swift

@@ -488,7 +488,7 @@ extension ClientConnection {
     public init(
       target: ConnectionTarget,
       eventLoopGroup: EventLoopGroup,
-      errorDelegate: ClientErrorDelegate? = DebugOnlyLoggingClientErrorDelegate.shared,
+      errorDelegate: ClientErrorDelegate? = LoggingClientErrorDelegate(),
       connectivityStateDelegate: ConnectivityStateDelegate? = nil,
       tls: Configuration.TLS? = nil,
       connectionBackoff: ConnectionBackoff? = ConnectionBackoff()

Sources/GRPC/ClientErrorDelegate.swift

@@ -27,36 +27,23 @@ public protocol ClientErrorDelegate: class {
   ///
   /// - Parameters:
   ///   - error: The error which was caught.
+  ///   - logger: A logger with relevant metadata for the RPC or connection the error relates to.
   ///   - file: The file where the error was raised.
   ///   - line: The line within the file where the error was raised.
-  func didCatchError(_ error: Error, file: StaticString, line: Int)
+  func didCatchError(_ error: Error, logger: Logger, file: StaticString, line: Int)
 }
 
-/// A `ClientErrorDelegate` which logs errors only in debug builds.
-public class DebugOnlyLoggingClientErrorDelegate: ClientErrorDelegate {
-  public static let shared = DebugOnlyLoggingClientErrorDelegate()
-  private let logger = Logger(labelSuffix: "ClientErrorDelegate")
+/// A `ClientErrorDelegate` which logs errors.
+public class LoggingClientErrorDelegate: ClientErrorDelegate {
+  public init() { }
 
-  private init() { }
-
-  public func didCatchError(_ error: Error, file: StaticString, line: Int) {
-    debugOnly {
-      self.logger.error(
-        "client error",
-        metadata: [MetadataKey.error: "\(error)"],
-        file: "\(file)",
-        function: "<unknown>",
-        line: UInt(line)
-      )
-    }
+  public func didCatchError(_ error: Error, logger: Logger, file: StaticString, line: Int) {
+    logger.error(
+      "grpc client error",
+      metadata: [MetadataKey.error: "\(error)"],
+      file: "\(file)",
+      function: "<unknown>",
+      line: UInt(line)
+    )
   }
 }
-
-/// A utility function that runs the body code only in debug builds, without emitting compiler
-/// warnings.
-///
-/// This is currently the only way to do this in Swift: see
-/// https://forums.swift.org/t/support-debug-only-code/11037 for a discussion.
-internal func debugOnly(_ body: () -> Void) {
-  assert({ body(); return true }())
-}

Sources/GRPC/DelegatingErrorHandler.swift

@@ -43,9 +43,8 @@ public class DelegatingErrorHandler: ChannelInboundHandler {
 
     if let delegate = self.delegate {
       let grpcError = (error as? GRPCError) ?? .unknown(error, origin: .client)
-      delegate.didCatchError(grpcError.wrappedError, file: grpcError.file, line: grpcError.line)
+      delegate.didCatchError(grpcError.wrappedError, logger: self.logger, file: grpcError.file, line: grpcError.line)
     }
-    self.logger.error("caught error in client channel", metadata: [MetadataKey.error: "\(error)"])
     context.close(promise: nil)
   }
 }

Sources/GRPC/GRPCClientResponseChannelHandler.swift

@@ -98,7 +98,7 @@ internal class GRPCClientResponseChannelHandler<ResponseMessage: Message>: Chann
   /// - Parameter error: the error to observe.
   internal func onError(_ error: Error) {
     let grpcError = (error as? GRPCError) ?? GRPCError.unknown(error, origin: .client)
-    self.errorDelegate?.didCatchError(grpcError.wrappedError, file: grpcError.file, line: grpcError.line)
+    self.errorDelegate?.didCatchError(grpcError.wrappedError, logger: self.logger, file: grpcError.file, line: grpcError.line)
     self.onStatus(grpcError.asGRPCStatus())
   }
 

Sources/GRPC/Shims.swift

@@ -35,3 +35,8 @@ extension Server.Configuration {
 
 @available(*, deprecated, renamed: "PlatformSupport")
 public enum GRPCNIO {}
+
+extension ClientErrorDelegate {
+  @available(*, deprecated, message: "Please use 'didCatchError(_:logger:file:line:)' instead")
+  public func didCatchError(_ error: Error, file: StaticString, line: Int) { }
+}

Tests/GRPCTests/ClientTLSFailureTests.swift

@@ -17,6 +17,7 @@ import Foundation
 import GRPC
 import GRPCSampleData
 import EchoImplementation
+import Logging
 import NIO
 import NIOSSL
 import XCTest
@@ -29,7 +30,7 @@ class ErrorRecordingDelegate: ClientErrorDelegate {
     self.expectation = expectation
   }
 
-  func didCatchError(_ error: Error, file: StaticString, line: Int) {
+  func didCatchError(_ error: Error, logger: Logger, file: StaticString, line: Int) {
     self.errors.append(error)
     self.expectation.fulfill()
   }

Tests/GRPCTests/DelegatingErrorHandlerTests.swift

@@ -17,6 +17,7 @@ import Foundation
 import GRPC
 import NIO
 import NIOSSL
+import Logging
 import XCTest
 
 class DelegatingErrorHandlerTests: GRPCTestCase {
@@ -25,7 +26,7 @@ class DelegatingErrorHandlerTests: GRPCTestCase {
 
     init() { }
 
-    func didCatchError(_ error: Error, file: StaticString, line: Int) {
+    func didCatchError(_ error: Error, logger: Logger, file: StaticString, line: Int) {
       self.errors.append(error)
     }
   }