From 65b6cf28b30047500a098f96759c9cbf85771182 Mon Sep 17 00:00:00 2001 From: Josh Elkins Date: Fri, 6 Sep 2024 17:13:16 -0500 Subject: [PATCH 1/2] fix: Add manual @unchecked Sendable conformance to HttpTelemetry --- .../Networking/Http/CRT/CRTClientEngine.swift | 39 ++++++++------- .../Networking/Http/HttpTelemetry.swift | 47 ++++++++++++++++--- .../URLSession/URLSessionHTTPClient.swift | 28 ++++++----- 3 files changed, 77 insertions(+), 37 deletions(-) diff --git a/Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift b/Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift index 295d6c796..f21528918 100644 --- a/Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift +++ b/Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift @@ -220,18 +220,20 @@ public class CRTClientEngine: HTTPClient { context: telemetryContext) // END - smithy.client.http.requests.queued_duration - // TICK - smithy.client.http.connections.limit - telemetry.httpMetricsUsage.connectionsLimit = serialExecutor.maxConnectionsPerEndpoint - + let connectionsLimit = serialExecutor.maxConnectionsPerEndpoint let connectionMgrMetrics = connectionMgr.fetchMetrics() + telemetry.updateHTTPMetricsUsage { httpMetricsUsage in + // TICK - smithy.client.http.connections.limit + httpMetricsUsage.connectionsLimit = connectionsLimit - // TICK - smithy.client.http.connections.usage - telemetry.httpMetricsUsage.idleConnections = connectionMgrMetrics.availableConcurrency - telemetry.httpMetricsUsage.acquiredConnections = connectionMgrMetrics.leasedConcurrency + // TICK - smithy.client.http.connections.usage + httpMetricsUsage.idleConnections = connectionMgrMetrics.availableConcurrency + httpMetricsUsage.acquiredConnections = connectionMgrMetrics.leasedConcurrency - // TICK - smithy.client.http.requests.usage - telemetry.httpMetricsUsage.inflightRequests = connectionMgrMetrics.leasedConcurrency - telemetry.httpMetricsUsage.queuedRequests = connectionMgrMetrics.pendingConcurrencyAcquires + // TICK - smithy.client.http.requests.usage + httpMetricsUsage.inflightRequests = connectionMgrMetrics.leasedConcurrency + httpMetricsUsage.queuedRequests = connectionMgrMetrics.pendingConcurrencyAcquires + } do { // DURATION - smithy.client.http.connections.uptime @@ -433,18 +435,21 @@ public class CRTClientEngine: HTTPClient { wrappedContinuation.safeResume(error: error) return } - // TICK - smithy.client.http.connections.limit - telemetry.httpMetricsUsage.connectionsLimit = serialExecutor.maxConnectionsPerEndpoint + let connectionsLimit = serialExecutor.maxConnectionsPerEndpoint let connectionMgrMetrics = connectionMgr.fetchMetrics() + telemetry.updateHTTPMetricsUsage { httpMetricsUsage in + // TICK - smithy.client.http.connections.limit + httpMetricsUsage.connectionsLimit = connectionsLimit - // TICK - smithy.client.http.connections.usage - telemetry.httpMetricsUsage.idleConnections = connectionMgrMetrics.availableConcurrency - telemetry.httpMetricsUsage.acquiredConnections = connectionMgrMetrics.leasedConcurrency + // TICK - smithy.client.http.connections.usage + httpMetricsUsage.idleConnections = connectionMgrMetrics.availableConcurrency + httpMetricsUsage.acquiredConnections = connectionMgrMetrics.leasedConcurrency - // TICK - smithy.client.http.requests.usage - telemetry.httpMetricsUsage.inflightRequests = connectionMgrMetrics.leasedConcurrency - telemetry.httpMetricsUsage.queuedRequests = connectionMgrMetrics.pendingConcurrencyAcquires + // TICK - smithy.client.http.requests.usage + httpMetricsUsage.inflightRequests = connectionMgrMetrics.leasedConcurrency + httpMetricsUsage.queuedRequests = connectionMgrMetrics.pendingConcurrencyAcquires + } // At this point, continuation is resumed when the initial headers are received // it is now safe to write the body diff --git a/Sources/ClientRuntime/Networking/Http/HttpTelemetry.swift b/Sources/ClientRuntime/Networking/Http/HttpTelemetry.swift index 049b4734e..d944a2c7a 100644 --- a/Sources/ClientRuntime/Networking/Http/HttpTelemetry.swift +++ b/Sources/ClientRuntime/Networking/Http/HttpTelemetry.swift @@ -6,11 +6,12 @@ import protocol SmithyHTTPAPI.HTTPClient import struct Smithy.Attributes import struct Smithy.AttributeKey +import class Foundation.NSRecursiveLock /// Container for HTTPClient telemetry, including configurable attributes and names. /// /// Note: This is intended to be used within generated code, not directly. -public class HttpTelemetry { +public class HttpTelemetry: @unchecked Sendable { private static var idleConnectionAttributes: Attributes { var attributes = Attributes() attributes.set(key: HttpMetricsAttributesKeys.state, value: ConnectionState.idle) @@ -37,20 +38,52 @@ public class HttpTelemetry { internal let loggerProvider: any LoggerProvider internal let tracerScope: String - internal let tracerAttributes: Attributes? internal let spanName: String - internal let spanAttributes: Attributes? internal let connectionsAcquireDuration: any Histogram private let connectionsLimit: any AsyncMeasurementHandle private let connectionsUsage: any AsyncMeasurementHandle - internal var httpMetricsUsage: HttpMetricsUsage internal let connectionsUptime: any Histogram private let requestsUsage: any AsyncMeasurementHandle internal let requestsQueuedDuration: any Histogram internal let bytesSent: any MonotonicCounter internal let bytesReceived: any MonotonicCounter + // Lock to enforce exclusive access to non-Sendable properties + private let lock = NSRecursiveLock() + + // The properties _tracerAttributes, _spanAttributes, and _httpMetricsUsage + // are not Sendable & must be protected by lock. + private let _tracerAttributes: Attributes? + + var tracerAttributes: Attributes? { + lock.lock() + defer { lock.unlock() } + return _tracerAttributes + } + + private let _spanAttributes: Attributes? + + var spanAttributes: Attributes? { + lock.lock() + defer { lock.unlock() } + return _spanAttributes + } + + private var _httpMetricsUsage: HttpMetricsUsage + + var httpMetricsUsage: HttpMetricsUsage { + lock.lock() + defer { lock.unlock() } + return _httpMetricsUsage + } + + func updateHTTPMetricsUsage(_ block: (inout HttpMetricsUsage) -> Void) { + lock.lock() + defer { lock.unlock() } + block(&_httpMetricsUsage) + } + public init( httpScope: String, telemetryProvider: any TelemetryProvider = DefaultTelemetry.provider, @@ -66,11 +99,11 @@ public class HttpTelemetry { self.loggerProvider = telemetryProvider.loggerProvider let meterScope: String = meterScope != nil ? meterScope! : httpScope self.tracerScope = tracerScope != nil ? tracerScope! : httpScope - self.tracerAttributes = tracerAttributes + self._tracerAttributes = tracerAttributes self.spanName = spanName != nil ? spanName! : "HTTP" - self.spanAttributes = spanAttributes + self._spanAttributes = spanAttributes let httpMetricsUsage = HttpMetricsUsage() - self.httpMetricsUsage = httpMetricsUsage + self._httpMetricsUsage = httpMetricsUsage let meter = telemetryProvider.meterProvider.getMeter(scope: meterScope, attributes: meterAttributes) self.connectionsAcquireDuration = meter.createHistogram( diff --git a/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift b/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift index e313dbfe1..9d8107f83 100644 --- a/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift +++ b/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift @@ -515,21 +515,23 @@ public final class URLSessionHTTPClient: HTTPClient { context: telemetryContext) // END - smithy.client.http.requests.queued_duration - // TICK - smithy.client.http.connections.limit - telemetry.httpMetricsUsage.connectionsLimit = session.configuration.httpMaximumConnectionsPerHost - - // TICK - smithy.client.http.connections.usage - // TODO(observability): instead of the transient stores, should rely on the Key/Value observer patttern + let connectionsLimit = session.configuration.httpMaximumConnectionsPerHost let totalCount = session.delegateQueue.operationCount let maxConcurrentOperationCount = session.delegateQueue.maxConcurrentOperationCount - telemetry.httpMetricsUsage.acquiredConnections = totalCount < maxConcurrentOperationCount - ? totalCount - : maxConcurrentOperationCount - telemetry.httpMetricsUsage.idleConnections = totalCount - telemetry.httpMetricsUsage.acquiredConnections - - // TICK - smithy.client.http.requests.usage - telemetry.httpMetricsUsage.inflightRequests = telemetry.httpMetricsUsage.acquiredConnections - telemetry.httpMetricsUsage.queuedRequests = telemetry.httpMetricsUsage.idleConnections + telemetry.updateHTTPMetricsUsage { httpMetricsUsage in + // TICK - smithy.client.http.connections.limit + httpMetricsUsage.connectionsLimit = connectionsLimit + + // TICK - smithy.client.http.connections.usage + // TODO(observability): instead of the transient stores, should rely on the Key/Value observer patttern + httpMetricsUsage.acquiredConnections = + totalCount < maxConcurrentOperationCount ? totalCount : maxConcurrentOperationCount + httpMetricsUsage.idleConnections = totalCount - httpMetricsUsage.acquiredConnections + + // TICK - smithy.client.http.requests.usage + httpMetricsUsage.inflightRequests = httpMetricsUsage.acquiredConnections + httpMetricsUsage.queuedRequests = httpMetricsUsage.idleConnections + } // Create the request (with a streaming body when needed.) do { From 086d6e2a11254da00373a1952c2cdf406a53288f Mon Sep 17 00:00:00 2001 From: Josh Elkins Date: Fri, 6 Sep 2024 17:52:49 -0500 Subject: [PATCH 2/2] fix swiftlint --- .../Networking/Http/URLSession/URLSessionHTTPClient.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift b/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift index 9d8107f83..133604866 100644 --- a/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift +++ b/Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift @@ -524,7 +524,7 @@ public final class URLSessionHTTPClient: HTTPClient { // TICK - smithy.client.http.connections.usage // TODO(observability): instead of the transient stores, should rely on the Key/Value observer patttern - httpMetricsUsage.acquiredConnections = + httpMetricsUsage.acquiredConnections = totalCount < maxConcurrentOperationCount ? totalCount : maxConcurrentOperationCount httpMetricsUsage.idleConnections = totalCount - httpMetricsUsage.acquiredConnections