From 12f255893052c770bfae1c04c0b8a414dfbb5ebf Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 14:43:03 -0800 Subject: [PATCH 01/10] wip --- .../Interceptors/ConnectInterceptor.swift | 3 ++- .../Interceptors/GRPCWebInterceptor.swift | 3 ++- .../Interceptors/InterceptorChain.swift | 6 +++++ .../Implementation/ProtocolClient.swift | 5 +++- .../Implementation/URLSessionHTTPClient.swift | 24 +++++++++++++++---- .../Interfaces/HTTPClientInterface.swift | 11 ++++++--- .../Connect/Interfaces/HTTPMetrics.swift | 24 +++++++++++++++++++ .../Connect/Interfaces/Interceptor.swift | 5 +++- 8 files changed, 70 insertions(+), 11 deletions(-) create mode 100644 Libraries/Connect/Interfaces/HTTPMetrics.swift diff --git a/Libraries/Connect/Implementation/Interceptors/ConnectInterceptor.swift b/Libraries/Connect/Implementation/Interceptors/ConnectInterceptor.swift index 8002f86c..322075c1 100644 --- a/Libraries/Connect/Implementation/Interceptors/ConnectInterceptor.swift +++ b/Libraries/Connect/Implementation/Interceptors/ConnectInterceptor.swift @@ -94,7 +94,8 @@ extension ConnectInterceptor: Interceptor { tracingInfo: response.tracingInfo ) } - } + }, + responseMetricsFunction: { $0 } ) } diff --git a/Libraries/Connect/Implementation/Interceptors/GRPCWebInterceptor.swift b/Libraries/Connect/Implementation/Interceptors/GRPCWebInterceptor.swift index a87e6fa4..497b6174 100644 --- a/Libraries/Connect/Implementation/Interceptors/GRPCWebInterceptor.swift +++ b/Libraries/Connect/Implementation/Interceptors/GRPCWebInterceptor.swift @@ -104,7 +104,8 @@ extension GRPCWebInterceptor: Interceptor { tracingInfo: response.tracingInfo ) } - } + }, + responseMetricsFunction: { $0 } ) } diff --git a/Libraries/Connect/Implementation/Interceptors/InterceptorChain.swift b/Libraries/Connect/Implementation/Interceptors/InterceptorChain.swift index 3613df5c..3735a753 100644 --- a/Libraries/Connect/Implementation/Interceptors/InterceptorChain.swift +++ b/Libraries/Connect/Implementation/Interceptors/InterceptorChain.swift @@ -48,6 +48,12 @@ struct InterceptorChain { interceptors.reversed().map(\.responseFunction), initial: response ) + }, + responseMetricsFunction: { metrics in + return executeInterceptors( + interceptors.reversed().map(\.responseMetricsFunction), + initial: metrics + ) } ) } diff --git a/Libraries/Connect/Implementation/ProtocolClient.swift b/Libraries/Connect/Implementation/ProtocolClient.swift index 35932b01..05823e41 100644 --- a/Libraries/Connect/Implementation/ProtocolClient.swift +++ b/Libraries/Connect/Implementation/ProtocolClient.swift @@ -69,7 +69,10 @@ extension ProtocolClient: ProtocolClientInterface { headers: headers, message: data )) - return self.httpClient.unary(request: request) { response in + return self.httpClient.unary( + request: request, + onMetrics: { _ = chain.responseMetricsFunction($0) } + ) { response in let response = chain.responseFunction(response) let responseMessage: ResponseMessage if response.code != .ok { diff --git a/Libraries/Connect/Implementation/URLSessionHTTPClient.swift b/Libraries/Connect/Implementation/URLSessionHTTPClient.swift index 86a622d0..0dae302f 100644 --- a/Libraries/Connect/Implementation/URLSessionHTTPClient.swift +++ b/Libraries/Connect/Implementation/URLSessionHTTPClient.swift @@ -20,6 +20,8 @@ import os.log open class URLSessionHTTPClient: NSObject, HTTPClientInterface { /// Lock used for safely accessing stream storage. private let lock = Lock() + /// Closures stored for notifying when metrics are available. + private var metricsClosures = [Int: (HTTPMetrics) -> Void]() /// Force unwrapped to allow using `self` as the delegate. private var session: URLSession! /// List of active streams. @@ -39,12 +41,14 @@ open class URLSessionHTTPClient: NSObject, HTTPClientInterface { @discardableResult open func unary( - request: HTTPRequest, completion: @Sendable @escaping (HTTPResponse) -> Void + request: HTTPRequest, + onMetrics: @Sendable @escaping (HTTPMetrics) -> Void, + onResponse: @Sendable @escaping (HTTPResponse) -> Void ) -> Cancelable { let urlRequest = URLRequest(httpRequest: request) let task = self.session.dataTask(with: urlRequest) { data, urlResponse, error in if let httpURLResponse = urlResponse as? HTTPURLResponse { - completion(HTTPResponse( + onResponse(HTTPResponse( code: Code.fromURLSessionCode(httpURLResponse.statusCode), headers: httpURLResponse.formattedLowercasedHeaders(), message: data, @@ -54,7 +58,7 @@ open class URLSessionHTTPClient: NSObject, HTTPClientInterface { )) } else if let error = error { let code = Code.fromURLSessionCode((error as NSError).code) - completion(HTTPResponse( + onResponse(HTTPResponse( code: code, headers: [:], message: data, @@ -67,7 +71,7 @@ open class URLSessionHTTPClient: NSObject, HTTPClientInterface { tracingInfo: nil )) } else { - completion(HTTPResponse( + onResponse(HTTPResponse( code: .unknown, headers: [:], message: data, @@ -81,6 +85,7 @@ open class URLSessionHTTPClient: NSObject, HTTPClientInterface { )) } } + self.lock.perform { self.metricsClosures[task.taskIdentifier] = onMetrics } task.resume() return Cancelable(cancel: task.cancel) } @@ -142,6 +147,17 @@ extension URLSessionHTTPClient: URLSessionTaskDelegate { let stream = self.lock.perform { self.streams.removeValue(forKey: task.taskIdentifier) } stream?.handleCompletion(error: error) } + + open func urlSession( + _ session: URLSession, task: URLSessionTask, + didFinishCollecting metrics: URLSessionTaskMetrics + ) { + if let metricsClosure = self.lock.perform( + action: { self.metricsClosures.removeValue(forKey: task.taskIdentifier) } + ) { + metricsClosure(HTTPMetrics(taskMetrics: metrics)) + } + } } extension HTTPURLResponse { diff --git a/Libraries/Connect/Interfaces/HTTPClientInterface.swift b/Libraries/Connect/Interfaces/HTTPClientInterface.swift index f77442d4..100065a6 100644 --- a/Libraries/Connect/Interfaces/HTTPClientInterface.swift +++ b/Libraries/Connect/Interfaces/HTTPClientInterface.swift @@ -17,12 +17,17 @@ public protocol HTTPClientInterface { /// Perform a unary HTTP request. /// /// - parameter request: The outbound request headers and data. - /// - parameter completion: Closure that should be called upon completion of the request. + /// - parameter onMetrics: Closure that should be called when metrics are finalized. This may be + /// called before or after `onResponse`. + /// - parameter onResponse: Closure that should be called when a response is received. /// /// - returns: A type which can be used to cancel the outbound request. @discardableResult - func unary(request: HTTPRequest, completion: @Sendable @escaping (HTTPResponse) -> Void) - -> Cancelable + func unary( + request: HTTPRequest, + onMetrics: @Sendable @escaping (HTTPMetrics) -> Void, + onResponse: @Sendable @escaping (HTTPResponse) -> Void + ) -> Cancelable /// Initialize a new HTTP stream. /// diff --git a/Libraries/Connect/Interfaces/HTTPMetrics.swift b/Libraries/Connect/Interfaces/HTTPMetrics.swift new file mode 100644 index 00000000..b5e3d36a --- /dev/null +++ b/Libraries/Connect/Interfaces/HTTPMetrics.swift @@ -0,0 +1,24 @@ +// Copyright 2022-2023 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Foundation + +/// Contains metrics from HTTP requests/streams. +public struct HTTPMetrics: Sendable { + public let taskMetrics: URLSessionTaskMetrics + + public init(taskMetrics: URLSessionTaskMetrics) { + self.taskMetrics = taskMetrics + } +} diff --git a/Libraries/Connect/Interfaces/Interceptor.swift b/Libraries/Connect/Interfaces/Interceptor.swift index 42ee02a4..98fb8022 100644 --- a/Libraries/Connect/Interfaces/Interceptor.swift +++ b/Libraries/Connect/Interfaces/Interceptor.swift @@ -40,13 +40,16 @@ public protocol Interceptor { public struct UnaryFunction { public let requestFunction: (HTTPRequest) -> HTTPRequest public let responseFunction: (HTTPResponse) -> HTTPResponse + public let responseMetricsFunction: (HTTPMetrics) -> HTTPMetrics public init( requestFunction: @escaping (HTTPRequest) -> HTTPRequest, - responseFunction: @escaping (HTTPResponse) -> HTTPResponse + responseFunction: @escaping (HTTPResponse) -> HTTPResponse, + responseMetricsFunction: @escaping (HTTPMetrics) -> HTTPMetrics ) { self.requestFunction = requestFunction self.responseFunction = responseFunction + self.responseMetricsFunction = responseMetricsFunction } } From ebbaa9c70c16600e02d43d03613a8f66ebeb0a33 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:01:38 -0800 Subject: [PATCH 02/10] tests --- .../ConnectTests/InterceptorChainTests.swift | 15 +++++++++++++-- .../ConnectTests/ProtocolClientConfigTests.swift | 4 +++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift index 3ddbfdaf..a6f90d63 100644 --- a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift +++ b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift @@ -19,6 +19,7 @@ private struct MockUnaryInterceptor: Interceptor { let headerID: String let requestExpectation: XCTestExpectation let responseExpectation: XCTestExpectation + let responseMetricsExpectation: XCTestExpectation func unaryFunction() -> Connect.UnaryFunction { return UnaryFunction( @@ -45,6 +46,10 @@ private struct MockUnaryInterceptor: Interceptor { error: response.error, tracingInfo: .init(httpStatus: 200) ) + }, + responseMetricsFunction: { metrics in + self.responseMetricsExpectation.fulfill() + return metrics } ) } @@ -112,20 +117,24 @@ final class InterceptorChainTests: XCTestCase { let bRequestExpectation = self.expectation(description: "Filter B called with request") let aResponseExpectation = self.expectation(description: "Filter A called with response") let bResponseExpectation = self.expectation(description: "Filter B called with response") + let aMetricsExpectation = self.expectation(description: "Filter A called with metrics") + let bMetricsExpectation = self.expectation(description: "Filter B called with metrics") let chain = InterceptorChain( interceptors: [ { _ in return MockUnaryInterceptor( headerID: "filter-a", requestExpectation: aRequestExpectation, - responseExpectation: aResponseExpectation + responseExpectation: aResponseExpectation, + responseMetricsExpectation: aMetricsExpectation ) }, { _ in return MockUnaryInterceptor( headerID: "filter-b", requestExpectation: bRequestExpectation, - responseExpectation: bResponseExpectation + responseExpectation: bResponseExpectation, + responseMetricsExpectation: bMetricsExpectation ) }, ], @@ -155,6 +164,8 @@ final class InterceptorChainTests: XCTestCase { bRequestExpectation, bResponseExpectation, aResponseExpectation, + aMetricsExpectation, + bMetricsExpectation, ], timeout: 1.0, enforceOrder: true), .completed) } diff --git a/Tests/ConnectLibraryTests/ConnectTests/ProtocolClientConfigTests.swift b/Tests/ConnectLibraryTests/ConnectTests/ProtocolClientConfigTests.swift index 908e635a..947e43b0 100644 --- a/Tests/ConnectLibraryTests/ConnectTests/ProtocolClientConfigTests.swift +++ b/Tests/ConnectLibraryTests/ConnectTests/ProtocolClientConfigTests.swift @@ -18,7 +18,9 @@ import XCTest private struct NoopInterceptor: Interceptor { func unaryFunction() -> UnaryFunction { - return .init(requestFunction: { $0 }, responseFunction: { $0 }) + return .init( + requestFunction: { $0 }, responseFunction: { $0 }, responseMetricsFunction: { $0 } + ) } func streamFunction() -> StreamFunction { From 326dc0753116c12a15a258a6c4bf21cadc0b9a60 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:19:36 -0800 Subject: [PATCH 03/10] Update HTTPMetrics.swift --- Libraries/Connect/Interfaces/HTTPMetrics.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Libraries/Connect/Interfaces/HTTPMetrics.swift b/Libraries/Connect/Interfaces/HTTPMetrics.swift index b5e3d36a..16fb0780 100644 --- a/Libraries/Connect/Interfaces/HTTPMetrics.swift +++ b/Libraries/Connect/Interfaces/HTTPMetrics.swift @@ -14,7 +14,7 @@ import Foundation -/// Contains metrics from HTTP requests/streams. +/// Contains metrics collected during the span of an HTTP request. public struct HTTPMetrics: Sendable { public let taskMetrics: URLSessionTaskMetrics From 31690a6fc5d0d56011102d7d9fbc67a36442a4c9 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:27:25 -0800 Subject: [PATCH 04/10] Update InterceptorChainTests.swift --- .../ConnectTests/InterceptorChainTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift index a6f90d63..9c121b78 100644 --- a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift +++ b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift @@ -164,8 +164,8 @@ final class InterceptorChainTests: XCTestCase { bRequestExpectation, bResponseExpectation, aResponseExpectation, - aMetricsExpectation, bMetricsExpectation, + aMetricsExpectation, ], timeout: 1.0, enforceOrder: true), .completed) } From e620e54c9c7ccf7ae7dbe133ba4536ee18de8c27 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:29:09 -0800 Subject: [PATCH 05/10] swiftlint --- .../Implementation/ProtocolClient.swift | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/Libraries/Connect/Implementation/ProtocolClient.swift b/Libraries/Connect/Implementation/ProtocolClient.swift index 05823e41..cdc43ca0 100644 --- a/Libraries/Connect/Implementation/ProtocolClient.swift +++ b/Libraries/Connect/Implementation/ProtocolClient.swift @@ -71,50 +71,50 @@ extension ProtocolClient: ProtocolClientInterface { )) return self.httpClient.unary( request: request, - onMetrics: { _ = chain.responseMetricsFunction($0) } - ) { response in - let response = chain.responseFunction(response) - let responseMessage: ResponseMessage - if response.code != .ok { - let error = (response.error as? ConnectError) + onMetrics: { _ = chain.responseMetricsFunction($0) }, + onResponse: { response in + let response = chain.responseFunction(response) + let responseMessage: ResponseMessage + if response.code != .ok { + let error = (response.error as? ConnectError) ?? ConnectError.from( code: response.code, headers: response.headers, source: response.message ) - responseMessage = ResponseMessage( - code: response.code, - headers: response.headers, - result: .failure(error), - trailers: response.trailers - ) - } else if let message = response.message { - do { responseMessage = ResponseMessage( code: response.code, headers: response.headers, - result: .success(try codec.deserialize(source: message)), + result: .failure(error), trailers: response.trailers ) - } catch let error { + } else if let message = response.message { + do { + responseMessage = ResponseMessage( + code: response.code, + headers: response.headers, + result: .success(try codec.deserialize(source: message)), + trailers: response.trailers + ) + } catch let error { + responseMessage = ResponseMessage( + code: response.code, + headers: response.headers, + result: .failure(ConnectError( + code: response.code, message: nil, exception: error, + details: [], metadata: response.headers + )), + trailers: response.trailers + ) + } + } else { responseMessage = ResponseMessage( code: response.code, headers: response.headers, - result: .failure(ConnectError( - code: response.code, message: nil, exception: error, - details: [], metadata: response.headers - )), + result: .success(.init()), trailers: response.trailers ) } - } else { - responseMessage = ResponseMessage( - code: response.code, - headers: response.headers, - result: .success(.init()), - trailers: response.trailers - ) - } - completion(responseMessage) - } + completion(responseMessage) + }) } public func bidirectionalStream< From 2896253e58df08623633894bf2a9c408a4d2b107 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:29:54 -0800 Subject: [PATCH 06/10] add note --- Libraries/Connect/Implementation/ProtocolClient.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Libraries/Connect/Implementation/ProtocolClient.swift b/Libraries/Connect/Implementation/ProtocolClient.swift index cdc43ca0..9f93ea6f 100644 --- a/Libraries/Connect/Implementation/ProtocolClient.swift +++ b/Libraries/Connect/Implementation/ProtocolClient.swift @@ -71,7 +71,10 @@ extension ProtocolClient: ProtocolClientInterface { )) return self.httpClient.unary( request: request, - onMetrics: { _ = chain.responseMetricsFunction($0) }, + onMetrics: { metrics in + // Response is unused, but metrics are passed to interceptors + _ = chain.responseMetricsFunction(metrics) + }, onResponse: { response in let response = chain.responseFunction(response) let responseMessage: ResponseMessage From f904ef5f8de388a49b106ca2f7ec1f9f539966b3 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 3 Mar 2023 15:31:51 -0800 Subject: [PATCH 07/10] swiftlint --- Libraries/Connect/Implementation/ProtocolClient.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Libraries/Connect/Implementation/ProtocolClient.swift b/Libraries/Connect/Implementation/ProtocolClient.swift index 9f93ea6f..3de85c37 100644 --- a/Libraries/Connect/Implementation/ProtocolClient.swift +++ b/Libraries/Connect/Implementation/ProtocolClient.swift @@ -117,7 +117,8 @@ extension ProtocolClient: ProtocolClientInterface { ) } completion(responseMessage) - }) + } + ) } public func bidirectionalStream< From 21a5145815c455c2d152431f96f3ffd3dc34d4dc Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Mon, 6 Mar 2023 10:11:23 -0800 Subject: [PATCH 08/10] fixup --- Libraries/Connect/Interfaces/HTTPMetrics.swift | 4 ++-- .../ConnectTests/InterceptorChainTests.swift | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Libraries/Connect/Interfaces/HTTPMetrics.swift b/Libraries/Connect/Interfaces/HTTPMetrics.swift index 16fb0780..1ba917ca 100644 --- a/Libraries/Connect/Interfaces/HTTPMetrics.swift +++ b/Libraries/Connect/Interfaces/HTTPMetrics.swift @@ -16,9 +16,9 @@ import Foundation /// Contains metrics collected during the span of an HTTP request. public struct HTTPMetrics: Sendable { - public let taskMetrics: URLSessionTaskMetrics + public let taskMetrics: URLSessionTaskMetrics? - public init(taskMetrics: URLSessionTaskMetrics) { + public init(taskMetrics: URLSessionTaskMetrics?) { self.taskMetrics = taskMetrics } } diff --git a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift index dcc79991..b85231c3 100644 --- a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift +++ b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift @@ -159,6 +159,9 @@ final class InterceptorChainTests: XCTestCase { )) XCTAssertEqual(interceptedResponse.headers["filter-chain"], ["filter-b", "filter-a"]) + let interceptedMetrics = chain.responseMetricsFunction(HTTPMetrics(taskMetrics: nil)) + XCTAssertEqual(interceptedMetrics.taskMetrics, nil) + XCTAssertEqual(XCTWaiter().wait(for: [ aRequestExpectation, bRequestExpectation, From 867b71bf35ee7e2c61f5f53b211249519bfd7d66 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Mon, 6 Mar 2023 11:22:03 -0800 Subject: [PATCH 09/10] add default value --- Libraries/Connect/Interfaces/Interceptor.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Libraries/Connect/Interfaces/Interceptor.swift b/Libraries/Connect/Interfaces/Interceptor.swift index 503bbfca..5db787d9 100644 --- a/Libraries/Connect/Interfaces/Interceptor.swift +++ b/Libraries/Connect/Interfaces/Interceptor.swift @@ -45,7 +45,7 @@ public struct UnaryFunction { public init( requestFunction: @escaping (HTTPRequest) -> HTTPRequest, responseFunction: @escaping (HTTPResponse) -> HTTPResponse, - responseMetricsFunction: @escaping (HTTPMetrics) -> HTTPMetrics + responseMetricsFunction: @escaping (HTTPMetrics) -> HTTPMetrics = { $0 } ) { self.requestFunction = requestFunction self.responseFunction = responseFunction From 30ca32fd4bcd7f6acdcc9450b914ef53b26f0391 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Mon, 6 Mar 2023 13:37:30 -0800 Subject: [PATCH 10/10] swiftlint --- .../ConnectTests/InterceptorChainTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift index b85231c3..782f911f 100644 --- a/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift +++ b/Tests/ConnectLibraryTests/ConnectTests/InterceptorChainTests.swift @@ -160,7 +160,7 @@ final class InterceptorChainTests: XCTestCase { XCTAssertEqual(interceptedResponse.headers["filter-chain"], ["filter-b", "filter-a"]) let interceptedMetrics = chain.responseMetricsFunction(HTTPMetrics(taskMetrics: nil)) - XCTAssertEqual(interceptedMetrics.taskMetrics, nil) + XCTAssertNil(interceptedMetrics.taskMetrics) XCTAssertEqual(XCTWaiter().wait(for: [ aRequestExpectation,