From a8494ce38c544050ae402bae997707b3d6da9ed6 Mon Sep 17 00:00:00 2001 From: Jamie <2119834+jamieQ@users.noreply.github.com> Date: Fri, 21 Jun 2024 15:25:21 -0500 Subject: [PATCH] [fix]: short circuit worker logging & require explicit opt-in (#285) ### Issue the existing os logging implementations log various type descriptions as part of the log message. these strings are apparently calculated even if the logging has been disabled, which introduces unneeded/undesirable work in release builds. these logs shouldn't be made in release builds, and should be opt-in in other contexts. ### Description this change adds short circuiting logic for all the os logging methods in addition to a new environment variable check that can be used to enable the logging when debugging. --- Workflow/Sources/WorkflowLogger.swift | 48 +++++++++++++++++++--- WorkflowCombine/Sources/Logger.swift | 7 ++++ WorkflowConcurrency/Sources/Logger.swift | 7 ++++ WorkflowReactiveSwift/Sources/Logger.swift | 16 +++++++- WorkflowRxSwift/Sources/Logger.swift | 16 +++++++- 5 files changed, 84 insertions(+), 10 deletions(-) diff --git a/Workflow/Sources/WorkflowLogger.swift b/Workflow/Sources/WorkflowLogger.swift index d68a5eb17..8a4ae2641 100644 --- a/Workflow/Sources/WorkflowLogger.swift +++ b/Workflow/Sources/WorkflowLogger.swift @@ -14,14 +14,19 @@ * limitations under the License. */ +import Foundation import os.signpost private extension OSLog { /// Logging will use this log handle when enabled static let workflow = OSLog(subsystem: "com.squareup.Workflow", category: "Workflow") - /// The active log handle to use when logging. Defaults to the shared `.disabled` handle. - static var active: OSLog = .disabled + /// The active log handle to use when logging. If `WorkflowLogging.osLoggingSupported` is + /// `true`, defaults to the `workflow` handle, otherwise defaults to the shared `.disabled` + /// handle. + static var active: OSLog = { + WorkflowLogging.isOSLoggingAllowed ? .workflow : .disabled + }() } // MARK: - @@ -29,6 +34,20 @@ private extension OSLog { /// Namespace for specifying logging configuration data. public enum WorkflowLogging {} +extension WorkflowLogging { + /// Flag indicating whether `OSLog` logs may be recorded. Note, actual emission of + /// log statements in specific cases may depend on additional configuration options, so + /// this being `true` does not necessarily imply logging will occur. + @_spi(Logging) + public static let isOSLoggingAllowed: Bool = { + let env = ProcessInfo.processInfo.environment + guard let value = env["com.squareup.workflow.allowOSLogging"] else { + return false + } + return (value as NSString).boolValue + }() +} + extension WorkflowLogging { public struct Config { /// Configuration options to control logging during a render pass. @@ -60,11 +79,16 @@ extension WorkflowLogging { /// To enable logging, at a minimum you must set: /// `WorkflowLogging.enabled = true` /// + /// Additionally, ``isOSLoggingAllowed`` must also be configured to be `true`. + /// /// If you wish for more control over what the runtime will log, you may additionally specify /// a custom value for `WorkflowLogging.config`. public static var enabled: Bool { get { OSLog.active === OSLog.workflow } - set { OSLog.active = newValue ? .workflow : .disabled } + set { + guard isOSLoggingAllowed else { return } + OSLog.active = newValue ? .workflow : .disabled + } } /// Configuration options used to determine which activities are logged. @@ -93,7 +117,10 @@ final class WorkflowLogger { // MARK: Workflows static func logWorkflowStarted(ref: WorkflowNode) { - guard WorkflowLogging.config.logLifetimes else { return } + guard + WorkflowLogging.isOSLoggingAllowed, + WorkflowLogging.config.logLifetimes + else { return } let signpostID = OSSignpostID(log: .active, object: ref) os_signpost( @@ -107,14 +134,20 @@ final class WorkflowLogger { } static func logWorkflowFinished(ref: WorkflowNode) { - guard WorkflowLogging.config.logLifetimes else { return } + guard + WorkflowLogging.isOSLoggingAllowed, + WorkflowLogging.config.logLifetimes + else { return } let signpostID = OSSignpostID(log: .active, object: ref) os_signpost(.end, log: .active, name: "Alive", signpostID: signpostID) } static func logSinkEvent(ref: AnyObject, action: Action) { - guard WorkflowLogging.config.logActions else { return } + guard + WorkflowLogging.isOSLoggingAllowed, + WorkflowLogging.config.logActions + else { return } let signpostID = OSSignpostID(log: .active, object: ref) os_signpost( @@ -165,6 +198,9 @@ final class WorkflowLogger { private static func shouldLogRenderTimings( isRootNode: Bool ) -> Bool { + guard WorkflowLogging.isOSLoggingAllowed else { + return false + } switch WorkflowLogging.config.renderLoggingMode { case .none: return false diff --git a/WorkflowCombine/Sources/Logger.swift b/WorkflowCombine/Sources/Logger.swift index 6fa3279c1..377d0342e 100644 --- a/WorkflowCombine/Sources/Logger.swift +++ b/WorkflowCombine/Sources/Logger.swift @@ -15,6 +15,7 @@ */ import os.signpost +@_spi(Logging) import Workflow private extension OSLog { static let worker = OSLog(subsystem: "com.squareup.WorkflowCombine", category: "Worker") @@ -29,6 +30,8 @@ final class WorkerLogger { // MARK: - Workers func logStarted() { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .begin, log: .worker, @@ -40,6 +43,8 @@ final class WorkerLogger { } func logFinished(status: StaticString) { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .end, log: .worker, @@ -50,6 +55,8 @@ final class WorkerLogger { } func logOutput() { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .event, log: .worker, diff --git a/WorkflowConcurrency/Sources/Logger.swift b/WorkflowConcurrency/Sources/Logger.swift index 5a9fa4e21..2935d231c 100644 --- a/WorkflowConcurrency/Sources/Logger.swift +++ b/WorkflowConcurrency/Sources/Logger.swift @@ -15,6 +15,7 @@ */ import os.signpost +@_spi(Logging) import Workflow private extension OSLog { static let worker = OSLog(subsystem: "com.squareup.WorkflowConcurrency", category: "Worker") @@ -29,6 +30,8 @@ final class WorkerLogger { // MARK: - Workers func logStarted() { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .begin, log: .worker, @@ -40,6 +43,8 @@ final class WorkerLogger { } func logFinished(status: StaticString) { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .end, log: .worker, @@ -50,6 +55,8 @@ final class WorkerLogger { } func logOutput() { + guard WorkflowLogging.isOSLoggingAllowed else { return } + os_signpost( .event, log: .worker, diff --git a/WorkflowReactiveSwift/Sources/Logger.swift b/WorkflowReactiveSwift/Sources/Logger.swift index 8921814a7..a30da572a 100644 --- a/WorkflowReactiveSwift/Sources/Logger.swift +++ b/WorkflowReactiveSwift/Sources/Logger.swift @@ -15,6 +15,7 @@ */ import os.signpost +@_spi(Logging) import Workflow // Namespace for Worker logging public enum WorkerLogging {} @@ -22,14 +23,19 @@ public enum WorkerLogging {} extension WorkerLogging { public static var enabled: Bool { get { OSLog.active === OSLog.worker } - set { OSLog.active = newValue ? .worker : .disabled } + set { + guard WorkflowLogging.isOSLoggingAllowed else { return } + OSLog.active = newValue ? .worker : .disabled + } } } private extension OSLog { static let worker = OSLog(subsystem: "com.squareup.WorkflowReactiveSwift", category: "Worker") - static var active: OSLog = .disabled + static var active: OSLog = { + WorkflowLogging.isOSLoggingAllowed ? .worker : .disabled + }() } // MARK: - @@ -43,6 +49,8 @@ final class WorkerLogger { // MARK: - Workers func logStarted() { + guard WorkerLogging.enabled else { return } + os_signpost( .begin, log: .active, @@ -54,10 +62,14 @@ final class WorkerLogger { } func logFinished(status: StaticString) { + guard WorkerLogging.enabled else { return } + os_signpost(.end, log: .active, name: "Running", signpostID: signpostID, status) } func logOutput() { + guard WorkerLogging.enabled else { return } + os_signpost( .event, log: .active, diff --git a/WorkflowRxSwift/Sources/Logger.swift b/WorkflowRxSwift/Sources/Logger.swift index a3d2807e2..03d544ccc 100644 --- a/WorkflowRxSwift/Sources/Logger.swift +++ b/WorkflowRxSwift/Sources/Logger.swift @@ -15,6 +15,7 @@ */ import os.signpost +@_spi(Logging) import Workflow // Namespace for Worker logging public enum WorkerLogging {} @@ -22,14 +23,19 @@ public enum WorkerLogging {} extension WorkerLogging { public static var enabled: Bool { get { OSLog.active === OSLog.worker } - set { OSLog.active = newValue ? .worker : .disabled } + set { + guard WorkflowLogging.isOSLoggingAllowed else { return } + OSLog.active = newValue ? .worker : .disabled + } } } private extension OSLog { static let worker = OSLog(subsystem: "com.squareup.WorkflowRxSwift", category: "Worker") - static var active: OSLog = .disabled + static var active: OSLog = { + WorkflowLogging.isOSLoggingAllowed ? .worker : .disabled + }() } // MARK: - @@ -43,6 +49,8 @@ final class WorkerLogger { // MARK: - Workers func logStarted() { + guard WorkerLogging.enabled else { return } + os_signpost( .begin, log: .active, @@ -54,10 +62,14 @@ final class WorkerLogger { } func logFinished(status: StaticString) { + guard WorkerLogging.enabled else { return } + os_signpost(.end, log: .active, name: "Running", signpostID: signpostID, status) } func logOutput() { + guard WorkerLogging.enabled else { return } + os_signpost( .event, log: .active,