From efcda495bc41b310f17173ba029c9bbd7156e22b Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Sat, 27 May 2023 12:14:04 +0200 Subject: [PATCH] Address logging issues (#304) * Make specific message levels a default extension * Define Loggable protocol * Keep track of message originator metadata * Fix tracing of Core Data logs * Log query on entity not found --- .../PassepartoutCore/Reusable/Logger.swift | 64 ++++++++----------- .../Utils/Utils+Logging.swift | 31 +++++---- ...in+Logging.swift => Domain+Loggable.swift} | 5 +- ...alProvidersRepository+Infrastructure.swift | 8 +-- .../CDLocalProvidersRepository+Provider.swift | 6 +- .../CDLocalProvidersRepository+Server.swift | 20 +++--- .../Strategies/CategoryMapper.swift | 2 +- .../Strategies/InfrastructureMapper.swift | 2 +- .../Strategies/LocationMapper.swift | 2 +- .../Strategies/PresetMapper.swift | 2 +- .../Strategies/ProviderMapper.swift | 2 +- .../Strategies/ServerMapper.swift | 14 ++-- ...in+Logging.swift => Domain+Loggable.swift} | 7 +- .../Strategies/ProfileMapper.swift | 4 +- .../Strategies/SwiftyBeaverLogger.swift | 23 ++----- 15 files changed, 84 insertions(+), 108 deletions(-) rename PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/{Domain+Logging.swift => Domain+Loggable.swift} (92%) rename PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/{Domain+Logging.swift => Domain+Loggable.swift} (89%) diff --git a/PassepartoutLibrary/Sources/PassepartoutCore/Reusable/Logger.swift b/PassepartoutLibrary/Sources/PassepartoutCore/Reusable/Logger.swift index 8be39b00..45f03acb 100644 --- a/PassepartoutLibrary/Sources/PassepartoutCore/Reusable/Logger.swift +++ b/PassepartoutLibrary/Sources/PassepartoutCore/Reusable/Logger.swift @@ -42,15 +42,33 @@ public protocol Logger { var logLevel: LoggerLevel { get set } - func verbose(_ message: Any) + func logMessage(_ level: LoggerLevel, _ message: Any, _ file: String, _ function: String, _ line: Int) +} - func debug(_ message: Any) +extension Logger { + public func verbose(_ message: Any, _ file: String = #file, _ function: String = #function, _ line: Int = #line) { + logMessage(.verbose, message, file, function, line) + } - func info(_ message: Any) + public func debug(_ message: Any, _ file: String = #file, _ function: String = #function, _ line: Int = #line) { + logMessage(.debug, message, file, function, line) + } - func warning(_ message: Any) + public func info(_ message: Any, _ file: String = #file, _ function: String = #function, _ line: Int = #line) { + logMessage(.info, message, file, function, line) + } - func error(_ message: Any) + public func warning(_ message: Any, _ file: String = #file, _ function: String = #function, _ line: Int = #line) { + logMessage(.warning, message, file, function, line) + } + + public func error(_ message: Any, _ file: String = #file, _ function: String = #function, _ line: Int = #line) { + logMessage(.error, message, file, function, line) + } +} + +public protocol Loggable { + var logDescription: String { get } } final class DefaultLogger: Logger { @@ -58,42 +76,10 @@ final class DefaultLogger: Logger { var logLevel: LoggerLevel = .debug - func verbose(_ message: Any) { - guard logLevel.rawValue >= LoggerLevel.verbose.rawValue else { + func logMessage(_ level: LoggerLevel, _ message: Any, _ file: String, _ function: String, _ line: Int) { + guard level.rawValue >= logLevel.rawValue else { return } - logMessage(message) - } - - func debug(_ message: Any) { - guard logLevel.rawValue >= LoggerLevel.debug.rawValue else { - return - } - logMessage(message) - } - - func info(_ message: Any) { - guard logLevel.rawValue >= LoggerLevel.info.rawValue else { - return - } - logMessage(message) - } - - func warning(_ message: Any) { - guard logLevel.rawValue >= LoggerLevel.warning.rawValue else { - return - } - logMessage(message) - } - - func error(_ message: Any) { - guard logLevel.rawValue >= LoggerLevel.error.rawValue else { - return - } - logMessage(message) - } - - private func logMessage(_ message: Any) { guard let string = message as? String else { return } diff --git a/PassepartoutLibrary/Sources/PassepartoutCore/Utils/Utils+Logging.swift b/PassepartoutLibrary/Sources/PassepartoutCore/Utils/Utils+Logging.swift index 07255356..02aac175 100644 --- a/PassepartoutLibrary/Sources/PassepartoutCore/Utils/Utils+Logging.swift +++ b/PassepartoutLibrary/Sources/PassepartoutCore/Utils/Utils+Logging.swift @@ -25,23 +25,32 @@ import Foundation -// XXX: these should be aliases like #define to print original caller line extension Utils { public static func assertCoreDataDecodingFailed( - _ file: String, - _ function: String, - _ line: Int, - _ message: String? = nil + _ message: String? = nil, + _ file: String = #file, + _ function: String = #function, + _ line: Int = #line ) { -// assertionFailure(message ?? "Cannot decode entity required fields - \(file):\(function):\(line)") - pp_log.warning(message ?? "Cannot decode entity required fields - \(file):\(function):\(line)") +// assertionFailure(message ?? "Cannot decode entity required fields", file, function, line) + pp_log.warning(message ?? "Cannot decode entity required fields", file, function, line) } - public static func logFetchError(_ file: String, _ function: String, _ line: Int, _ error: Error) { - pp_log.error("Unable to fetch: \(error) - \(file):\(function):\(line)") + public static func logFetchError( + _ error: Error, + _ file: String = #file, + _ function: String = #function, + _ line: Int = #line + ) { + pp_log.error("Unable to fetch: \(error)", file, function, line) } - public static func logFetchNotFound(_ file: String, _ function: String, _ line: Int) { - pp_log.debug("Not found - \(file):\(function):\(line)") + public static func logFetchNotFound( + _ query: String, + _ file: String = #file, + _ function: String = #function, + _ line: Int = #line + ) { + pp_log.debug("Entity not found: (\(query))", file, function, line) } } diff --git a/PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Logging.swift b/PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Loggable.swift similarity index 92% rename from PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Logging.swift rename to PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Loggable.swift index 3dabd0f6..17edc15d 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Logging.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProviders/Extensions/Domain+Loggable.swift @@ -1,5 +1,5 @@ // -// Domain+Logging.swift +// Domain+Loggable.swift // Passepartout // // Created by Davide De Rosa on 4/7/22. @@ -24,8 +24,9 @@ // import Foundation +import PassepartoutCore -extension ProviderServer { +extension ProviderServer: Loggable { public var logDescription: String { "{'\(categoryName)', \(countryCode), '\(apiId)', \(id)}" } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Infrastructure.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Infrastructure.swift index 0c11c3f3..aead61fc 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Infrastructure.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Infrastructure.swift @@ -39,12 +39,12 @@ extension CDLocalProvidersRepository: InfrastructureRepository { ] do { guard let infrastructureDTO = try context.fetch(request).first else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound("\(name), \(vpnProtocol)") return nil } return infrastructureDTO.defaults?.usernamePlaceholder } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } @@ -61,14 +61,14 @@ extension CDLocalProvidersRepository: InfrastructureRepository { do { let infrastructures = try context.fetch(request) guard !infrastructures.isEmpty else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound("\(name), \(vpnProtocol)") return nil } let recent = infrastructures.first! return recent.lastUpdate } catch { context.rollback() - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Provider.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Provider.swift index e97e1350..2e650665 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Provider.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Provider.swift @@ -45,7 +45,7 @@ extension CDLocalProvidersRepository: ProviderRepository { } return providers.compactMap(ProviderMapper.toModel) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return [] } } @@ -62,13 +62,13 @@ extension CDLocalProvidersRepository: ProviderRepository { do { let providers = try context.fetch(request) guard !providers.isEmpty else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound(name) return nil } let recent = providers.first! return ProviderMapper.toModel(recent) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Server.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Server.swift index 80830380..7c7ca5a1 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Server.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CDLocalProvidersRepository+Server.swift @@ -47,7 +47,7 @@ extension CDLocalProvidersRepository: ServerRepository { let categoryDTOs = try context.fetch(request) return categoryDTOs.compactMap(CategoryMapper.toModel) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return [] } } @@ -73,7 +73,7 @@ extension CDLocalProvidersRepository: ServerRepository { // just preset ids, no full ProviderServer.Preset return serverDTOs.compactMap(ServerMapper.toModel) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return [] } } @@ -94,12 +94,12 @@ extension CDLocalProvidersRepository: ServerRepository { ] do { guard let serverDTO = try context.fetch(request).first else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound("\(providerName), \(vpnProtocol), \(apiId)") return nil } return ServerMapper.toModelWithPresets(serverDTO) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } @@ -121,12 +121,12 @@ extension CDLocalProvidersRepository: ServerRepository { do { try Utils.randomizeFetchResults(request, in: context) guard let serverDTO = try context.fetch(request).first else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound("\(providerName), \(vpnProtocol), \(countryCode)") return nil } return ServerMapper.toModelWithPresets(serverDTO) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } @@ -148,12 +148,12 @@ extension CDLocalProvidersRepository: ServerRepository { do { try Utils.randomizeFetchResults(request, in: context) guard let serverDTO = try context.fetch(request).first else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound("\(providerName), \(vpnProtocol)") return nil } return ServerMapper.toModelWithPresets(serverDTO) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } @@ -171,12 +171,12 @@ extension CDLocalProvidersRepository: ServerRepository { ] do { guard let serverDTO = try context.fetch(request).first else { - Utils.logFetchNotFound(#file, #function, #line) + Utils.logFetchNotFound(id) return nil } return ServerMapper.toModelWithPresets(serverDTO) } catch { - Utils.logFetchError(#file, #function, #line, error) + Utils.logFetchError(error) return nil } } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CategoryMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CategoryMapper.swift index a5d4037e..f1687f98 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CategoryMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/CategoryMapper.swift @@ -64,7 +64,7 @@ struct CategoryMapper: DTOMapper, ModelMapper { let name = dto.name, let locations = dto.locations else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/InfrastructureMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/InfrastructureMapper.swift index 5de4df7a..15e20ee1 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/InfrastructureMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/InfrastructureMapper.swift @@ -100,7 +100,7 @@ struct InfrastructureMapper: DTOMapper { vpnProtocol: vpnProtocol, apiId: apiId ) else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return } server.uniqueId = uniqueId diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/LocationMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/LocationMapper.swift index 95dc6106..948659ff 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/LocationMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/LocationMapper.swift @@ -58,7 +58,7 @@ struct LocationMapper: DTOMapper, ModelMapper { let categoryName = dto.category?.name, let countryCode = dto.countryCode else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/PresetMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/PresetMapper.swift index 57152da0..66e7c30b 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/PresetMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/PresetMapper.swift @@ -60,7 +60,7 @@ struct PresetMapper: DTOMapper, ModelMapper { let vpnProtocol = VPNProtocolType(rawValue: vpnProtocolString), let vpnConfiguration = dto.decodedConfiguration else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ProviderMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ProviderMapper.swift index ee9b469b..5479558c 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ProviderMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ProviderMapper.swift @@ -57,7 +57,7 @@ struct ProviderMapper: DTOMapper, ModelMapper { guard let name = dto.name, let fullName = dto.fullName else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } diff --git a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ServerMapper.swift b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ServerMapper.swift index 7792a2b1..8360aa2f 100644 --- a/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ServerMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutProvidersImpl/Strategies/ServerMapper.swift @@ -73,14 +73,11 @@ struct ServerMapper: DTOMapper, ModelMapper { let providerMetadata = ProviderMapper.toModel(providerDTO), let countryCode = dto.countryCode else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } guard let presetDTOs = categoryDTO.presets?.allObjects as? [CDInfrastructurePreset], !presetDTOs.isEmpty else { - Utils.assertCoreDataDecodingFailed( - #file, #function, #line, - "Category '\(categoryName)' of server \(apiId) has no presets" - ) + Utils.assertCoreDataDecodingFailed("Category '\(categoryName)' of server \(apiId) has no presets") return nil } @@ -109,14 +106,11 @@ struct ServerMapper: DTOMapper, ModelMapper { let categoryDTO = dto.category, let categoryName = categoryDTO.name else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } guard let presetDTOs = dto.category?.presets?.allObjects as? [CDInfrastructurePreset], !presetDTOs.isEmpty else { - Utils.assertCoreDataDecodingFailed( - #file, #function, #line, - "Category '\(categoryName)' has no presets" - ) + Utils.assertCoreDataDecodingFailed("Category '\(categoryName)' has no presets") return nil } diff --git a/PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Logging.swift b/PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Loggable.swift similarity index 89% rename from PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Logging.swift rename to PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Loggable.swift index 308d3fb3..3129c753 100644 --- a/PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Logging.swift +++ b/PassepartoutLibrary/Sources/PassepartoutVPN/Extensions/Domain+Loggable.swift @@ -1,5 +1,5 @@ // -// Domain+Logging.swift +// Domain+Loggable.swift // Passepartout // // Created by Davide De Rosa on 4/7/22. @@ -24,14 +24,15 @@ // import Foundation +import PassepartoutCore -extension Profile.Header { +extension Profile.Header: Loggable { public var logDescription: String { "{\(id), '\(name)'}" } } -extension Profile { +extension Profile: Loggable { public var logDescription: String { header.logDescription } diff --git a/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/ProfileMapper.swift b/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/ProfileMapper.swift index 53321603..3e0c39b3 100644 --- a/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/ProfileMapper.swift +++ b/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/ProfileMapper.swift @@ -48,7 +48,7 @@ struct ProfileMapper: DTOMapper, ModelMapper { static func toModel(_ dto: CDProfile) throws -> Profile? { guard let json = dto.json else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } do { @@ -80,7 +80,7 @@ struct ProfileHeaderMapper: DTOMapper, ModelMapper { guard let uuid = dto.uuid, let name = dto.name else { - Utils.assertCoreDataDecodingFailed(#file, #function, #line) + Utils.assertCoreDataDecodingFailed() return nil } return Profile.Header( diff --git a/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/SwiftyBeaverLogger.swift b/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/SwiftyBeaverLogger.swift index e254b6af..741df9b5 100644 --- a/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/SwiftyBeaverLogger.swift +++ b/PassepartoutLibrary/Sources/PassepartoutVPNImpl/Strategies/SwiftyBeaverLogger.swift @@ -32,8 +32,9 @@ public final class SwiftyBeaverLogger: Logger { public var logLevel: LoggerLevel { didSet { + let nativeLevel = logLevel.toSwiftyBeaver SwiftyBeaver.destinations.forEach { - $0.minLevel = logLevel.toSwiftyBeaver + $0.minLevel = nativeLevel } } } @@ -62,24 +63,8 @@ public final class SwiftyBeaverLogger: Logger { } } - public func verbose(_ message: Any) { - SwiftyBeaver.verbose(message) - } - - public func debug(_ message: Any) { - SwiftyBeaver.debug(message) - } - - public func info(_ message: Any) { - SwiftyBeaver.info(message) - } - - public func warning(_ message: Any) { - SwiftyBeaver.warning(message) - } - - public func error(_ message: Any) { - SwiftyBeaver.error(message) + public func logMessage(_ level: LoggerLevel, _ message: Any, _ file: String, _ function: String, _ line: Int) { + SwiftyBeaver.custom(level: level.toSwiftyBeaver, message: message, file: file, function: function, line: line) } }