remove global observability scope (#3777)

motivation: better fit with long running process likes IDEs

changes:
* remove the global bootstrap and scope
* add observabilityScope argument to functions that need to emit diagnostics
* adjust call sites and tests

Author: tomerd

Examples/package-info/Sources/package-info/main.swift

@@ -1,3 +1,4 @@
+import Basics
 import TSCBasic
 import Workspace
 
@@ -13,22 +14,16 @@ let packagePath = AbsolutePath(#file).parentDirectory.parentDirectory.parentDire
 
 // There are several levels of information available.
 // Each takes longer to load than the level above it, but provides more detail.
-let diagnostics = DiagnosticsEngine(handlers: [{ print($0)}])
+
+let observability = ObservabilitySystem({ print("\($0): \($1)") })
+
 let workspace = try Workspace(forRootPackage: packagePath)
-let manifest = try tsc_await { workspace.loadRootManifest(at: packagePath, diagnostics: diagnostics, completion: $0) }
-guard !diagnostics.hasErrors else {
-    fatalError("error loading manifest: \(diagnostics)")
-}
-
-let package = try tsc_await { workspace.loadRootPackage(at: packagePath, diagnostics: diagnostics, completion: $0) }
-guard !diagnostics.hasErrors else {
-    fatalError("error loading package: \(diagnostics)")
-}
-
-let graph = try workspace.loadPackageGraph(rootPath: packagePath, diagnostics: diagnostics)
-guard !diagnostics.hasErrors else {
-    fatalError("error loading graph: \(diagnostics)")
-}
+
+let manifest = try tsc_await { workspace.loadRootManifest(at: packagePath, observabilityScope: observability.topScope, completion: $0) }
+
+let package = try tsc_await { workspace.loadRootPackage(at: packagePath, observabilityScope: observability.topScope, completion: $0) }
+
+let graph = try workspace.loadPackageGraph(rootPath: packagePath, observabilityScope: observability.topScope)
 
 // EXAMPLES
 // ========

Sources/Basics/HTPClient+URLSession.swift

@@ -27,6 +27,10 @@ public struct URLSessionHTTPClient: HTTPClientProtocol {
     }
 
     public func execute(_ request: HTTPClient.Request, progress: ProgressHandler?, completion: @escaping CompletionHandler) {
+        self.execute(request, observabilityScope: nil, progress: progress, completion: completion)
+    }
+
+    public func execute(_ request: HTTPClient.Request, observabilityScope: ObservabilityScope? = nil, progress: ProgressHandler?, completion: @escaping CompletionHandler) {
         switch request.kind {
         case .generic:
             let task = self.dataTasksManager.makeTask(request: request, progress: progress, completion: completion)

Sources/Basics/HTTPClient.swift

@@ -33,8 +33,11 @@ public protocol HTTPClientProtocol {
     ///
     /// - Parameters:
     ///   - request: The `HTTPClientRequest` to perform.
+    ///   - observabilityScope: the observability scope to emit diagnostics on
+    ///   - progress: A closure to handle progress for example for downloads
     ///   - callback: A closure to be notified of the completion of the request.
     func execute(_ request: HTTPClientRequest,
+                 observabilityScope: ObservabilityScope?,
                  progress: ProgressHandler?,
                  completion: @escaping CompletionHandler)
 }
@@ -56,21 +59,19 @@ public struct HTTPClient: HTTPClientProtocol {
     public typealias Handler = (Request, ProgressHandler?, @escaping (Result<Response, Error>) -> Void) -> Void
 
     public var configuration: HTTPClientConfiguration
-    private let observabilityScope: ObservabilityScope
     private let underlying: Handler
 
     // static to share across instances of the http client
     private static var hostsErrorsLock = Lock()
     private static var hostsErrors = [String: [Date]]()
 
-    public init(configuration: HTTPClientConfiguration = .init(), handler: Handler? = nil, observabilityScope: ObservabilityScope = ObservabilitySystem.topScope) {
+    public init(configuration: HTTPClientConfiguration = .init(), handler: Handler? = nil) {
         self.configuration = configuration
-        self.observabilityScope = observabilityScope
         // FIXME: inject platform specific implementation here
         self.underlying = handler ?? URLSessionHTTPClient().execute
     }
 
-    public func execute(_ request: Request, progress: ProgressHandler? = nil, completion: @escaping CompletionHandler) {
+    public func execute(_ request: Request, observabilityScope: ObservabilityScope? = nil, progress: ProgressHandler? = nil, completion: @escaping CompletionHandler) {
         // merge configuration
         var request = request
         if request.options.callbackQueue == nil {
@@ -103,7 +104,9 @@ public struct HTTPClient: HTTPClientProtocol {
         // execute
         let callbackQueue = request.options.callbackQueue ?? self.configuration.callbackQueue
         self._execute(
-            request: request, requestNumber: 0,
+            request: request,
+            requestNumber: 0,
+            observabilityScope: observabilityScope,
             progress: progress.map { handler in
                 { received, expected in
                     callbackQueue.async {
@@ -119,9 +122,9 @@ public struct HTTPClient: HTTPClientProtocol {
         )
     }
 
-    private func _execute(request: Request, requestNumber: Int, progress: ProgressHandler?, completion: @escaping CompletionHandler) {
+    private func _execute(request: Request, requestNumber: Int, observabilityScope: ObservabilityScope?, progress: ProgressHandler?, completion: @escaping CompletionHandler) {
         if self.shouldCircuitBreak(request: request) {
-            self.observabilityScope.emit(warning: "Circuit breaker triggered for \(request.url)")
+            observabilityScope?.emit(warning: "Circuit breaker triggered for \(request.url)")
             return completion(.failure(HTTPClientError.circuitBreakerTriggered))
         }
 
@@ -145,10 +148,10 @@ public struct HTTPClient: HTTPClientProtocol {
                     self.recordErrorIfNecessary(response: response, request: request)
                     // handle retry strategy
                     if let retryDelay = self.shouldRetry(response: response, request: request, requestNumber: requestNumber) {
-                        self.observabilityScope.emit(warning: "\(request.url) failed, retrying in \(retryDelay)")
+                        observabilityScope?.emit(warning: "\(request.url) failed, retrying in \(retryDelay)")
                         // TODO: dedicated retry queue?
                         return self.configuration.callbackQueue.asyncAfter(deadline: .now() + retryDelay) {
-                            self._execute(request: request, requestNumber: requestNumber + 1, progress: progress, completion: completion)
+                            self._execute(request: request, requestNumber: requestNumber + 1, observabilityScope: observabilityScope, progress: progress, completion: completion)
                         }
                     }
                     // check for valid response codes
@@ -220,24 +223,24 @@ public struct HTTPClient: HTTPClientProtocol {
 }
 
 public extension HTTPClient {
-    func head(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), completion: @escaping (Result<Response, Error>) -> Void) {
-        self.execute(Request(method: .head, url: url, headers: headers, body: nil, options: options), completion: completion)
+    func head(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), observabilityScope: ObservabilityScope? = .none, completion: @escaping (Result<Response, Error>) -> Void) {
+        self.execute(Request(method: .head, url: url, headers: headers, body: nil, options: options), observabilityScope: observabilityScope, completion: completion)
     }
 
-    func get(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), completion: @escaping (Result<Response, Error>) -> Void) {
-        self.execute(Request(method: .get, url: url, headers: headers, body: nil, options: options), completion: completion)
+    func get(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), observabilityScope: ObservabilityScope? = .none, completion: @escaping (Result<Response, Error>) -> Void) {
+        self.execute(Request(method: .get, url: url, headers: headers, body: nil, options: options), observabilityScope: observabilityScope, completion: completion)
     }
 
-    func put(_ url: URL, body: Data?, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), completion: @escaping (Result<Response, Error>) -> Void) {
-        self.execute(Request(method: .put, url: url, headers: headers, body: body, options: options), completion: completion)
+    func put(_ url: URL, body: Data?, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), observabilityScope: ObservabilityScope? = .none, completion: @escaping (Result<Response, Error>) -> Void) {
+        self.execute(Request(method: .put, url: url, headers: headers, body: body, options: options), observabilityScope: observabilityScope, completion: completion)
     }
 
-    func post(_ url: URL, body: Data?, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), completion: @escaping (Result<Response, Error>) -> Void) {
-        self.execute(Request(method: .post, url: url, headers: headers, body: body, options: options), completion: completion)
+    func post(_ url: URL, body: Data?, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), observabilityScope: ObservabilityScope? = .none, completion: @escaping (Result<Response, Error>) -> Void) {
+        self.execute(Request(method: .post, url: url, headers: headers, body: body, options: options), observabilityScope: observabilityScope, completion: completion)
     }
 
-    func delete(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), completion: @escaping (Result<Response, Error>) -> Void) {
-        self.execute(Request(method: .delete, url: url, headers: headers, body: nil, options: options), completion: completion)
+    func delete(_ url: URL, headers: HTTPClientHeaders = .init(), options: Request.Options = .init(), observabilityScope: ObservabilityScope? = .none, completion: @escaping (Result<Response, Error>) -> Void) {
+        self.execute(Request(method: .delete, url: url, headers: headers, body: nil, options: options), observabilityScope: observabilityScope, completion: completion)
     }
 }
 

Sources/Basics/Observability.swift

@@ -20,59 +20,46 @@ typealias TSCDiagnostic = TSCBasic.Diagnostic
 // designed after https://github.com/apple/swift-metrics
 // designed after https://github.com/apple/swift-distributed-tracing-baggage
 public class ObservabilitySystem {
-    // global
-
-    private static var _global = ObservabilitySystem(factory: NOOPFactory())
-    private static var bootstrapped = false
-    private static let lock = Lock()
-
-    // as we transition to async/await we can take advantage of Task Local values instead of a global
-    public static func bootstrapGlobal(factory: ObservabilityFactory) {
-        Self.lock.withLock {
-            // FIXME: disabled for testing
-            precondition(!Self.bootstrapped, "ObservabilitySystem may only bootstrapped once")
-            Self.bootstrapped = true
-        }
-        Self._global = .init(factory: factory)
-    }
-
-    /// DO  NOT USE, only for testing. friend visibility would be soooo nice here
-    public static func _bootstrapGlobalForTestingOnlySeriously(factory: ObservabilityFactory) {
-        Self._global = .init(factory: factory)
-    }
-
-    public static var topScope: ObservabilityScope {
-        self._global.topScope
-    }
-
-    // instance
-
     public let topScope: ObservabilityScope
 
-    public init(factory: ObservabilityFactory) {
+    /// Create an ObservabilitySystem with a handler provider providing handler such as a collector.
+    public init(_ handlerProvider: ObservabilityHandlerProvider) {
         self.topScope = .init(
             description: "top scope",
             parent: .none,
             metadata: .none,
-            diagnosticsHandler: factory.diagnosticsHandler
+            diagnosticsHandler: handlerProvider.diagnosticsHandler
         )
     }
 
-    private struct NOOPFactory: ObservabilityFactory, DiagnosticsHandler {
+    /// Create an ObservabilitySystem with a single diagnostics handler.
+    public convenience init(_ handler: @escaping (ObservabilityScope, Diagnostic) -> Void) {
+        self.init(SingleDiagnosticsHandler(handler))
+    }
+
+    private struct SingleDiagnosticsHandler: ObservabilityHandlerProvider, DiagnosticsHandler {
         var diagnosticsHandler: DiagnosticsHandler  { self }
 
-        func handleDiagnostic(scope: ObservabilityScope, diagnostic: Diagnostic) {}
+        let underlying: (ObservabilityScope, Diagnostic) -> Void
+
+        init(_ underlying: @escaping (ObservabilityScope, Diagnostic) -> Void) {
+            self.underlying = underlying
+        }
+
+        func handleDiagnostic(scope: ObservabilityScope, diagnostic: Diagnostic) {
+            self.underlying(scope, diagnostic)
+        }
     }
 }
 
-public protocol ObservabilityFactory {
+public protocol ObservabilityHandlerProvider {
     var diagnosticsHandler: DiagnosticsHandler { get }
 }
 
 // MARK: - ObservabilityScope
 
-public final class ObservabilityScope: DiagnosticsEmitterProtocol {
-    private let description: String
+public final class ObservabilityScope: DiagnosticsEmitterProtocol, CustomStringConvertible {
+    public let description: String
     private let parent: ObservabilityScope?
     private let metadata: ObservabilityMetadata?
 
@@ -115,18 +102,21 @@ public final class ObservabilityScope: DiagnosticsEmitterProtocol {
         self.makeDiagnosticsEmitter(metadata: metadataProvider())
     }
 
-    // FIXME: compatibility with DiagnosticsEngine, remove when transition is complete
-    //@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
-    public func makeDiagnosticsEngine() -> DiagnosticsEngine {
-        return .init(handlers: [{ Diagnostic($0).map{ self.diagnosticsHandler.handleDiagnostic(scope: self, diagnostic: $0) } }])
-    }
-
     // FIXME: we want to remove this functionality and move to more conventional error handling
     //@available(*, deprecated, message: "this pattern is deprecated, transition to error handling instead")
     public var errorsReported: Bool {
         self.diagnosticsHandler.errorsReported
     }
 
+    // FIXME: we want to remove this functionality and move to more conventional error handling
+    //@available(*, deprecated, message: "this pattern is deprecated, transition to error handling instead")
+    public var errorsReportedInAnyScope: Bool {
+        if self.errorsReported {
+            return true
+        }
+        return parent?.errorsReportedInAnyScope ?? false
+    }
+
     // DiagnosticsEmitterProtocol
     public func emit(_ diagnostic: Diagnostic) {
         var diagnostic = diagnostic
@@ -228,6 +218,9 @@ extension DiagnosticsEmitterProtocol {
     public func trap<T>(_ closure: () throws -> T) -> T? {
         do  {
             return try closure()
+        } catch Diagnostics.fatalError {
+            // FIXME: (diagnostics) deprecate this with Diagnostics.fatalError
+            return nil
         } catch {
             self.emit(error)
             return nil
@@ -457,6 +450,13 @@ extension ObservabilityMetadata.AnyKey: Hashable {
 
 // MARK: - Compatibility with TSC Diagnostics APIs
 
+//@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
+extension ObservabilityScope {
+    public func makeDiagnosticsEngine() -> DiagnosticsEngine {
+        return .init(handlers: [{ Diagnostic($0).map{ self.diagnosticsHandler.handleDiagnostic(scope: self, diagnostic: $0) } }])
+    }
+}
+
 //@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
 extension Diagnostic {
     init?(_ diagnostic: TSCDiagnostic) {
@@ -483,6 +483,46 @@ extension Diagnostic {
     }
 }
 
+@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
+extension ObservabilitySystem {
+    public convenience init(diagnosticEngine: DiagnosticsEngine) {
+        self.init(DiagnosticsEngineAdapter(diagnosticEngine: diagnosticEngine))
+    }
+
+    private struct DiagnosticsEngineAdapter: ObservabilityHandlerProvider, DiagnosticsHandler {
+        let diagnosticEngine: DiagnosticsEngine
+
+        var diagnosticsHandler: DiagnosticsHandler { self }
+
+        func handleDiagnostic(scope: ObservabilityScope, diagnostic: Diagnostic) {
+            diagnosticEngine.emit(.init(diagnostic))
+        }
+    }
+}
+
+@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
+extension TSCDiagnostic {
+    init(_ diagnostic: Diagnostic) {
+        let location: DiagnosticLocation
+        if let metadata = diagnostic.metadata {
+            location = metadata.legacyDiagnosticLocation
+        } else {
+            location = UnknownLocation.location
+        }
+
+        switch diagnostic.severity {
+        case .error:
+            self = .init(message: .error(diagnostic.message), location: location)
+        case .warning:
+            self = .init(message: .warning(diagnostic.message), location: location)
+        case .info:
+            self = .init(message: .note(diagnostic.message), location: location)
+        case .debug:
+            self = .init(message: .note(diagnostic.message), location: location)
+        }
+    }
+}
+
 //@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
 extension ObservabilityMetadata {
     public var legacyLocation: String? {
@@ -498,3 +538,21 @@ extension ObservabilityMetadata {
         typealias Value = String
     }
 }
+
+@available(*, deprecated, message: "temporary for transition DiagnosticsEngine -> DiagnosticsEmitter")
+extension ObservabilityMetadata {
+    fileprivate var legacyDiagnosticLocation: DiagnosticLocation {
+        if let legacyLocation = self.legacyLocation {
+            return DiagnosticLocationWrapper(legacyLocation)
+        }
+        return UnknownLocation.location
+    }
+
+    private struct DiagnosticLocationWrapper: DiagnosticLocation {
+        var description: String
+
+        init (_ location: String) {
+            self.description = location
+        }
+    }
+}