Skip to content

Commit

Permalink
fix: Add @unchecked Sendable conformance to HttpTelemetry (#810)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbelkins
jbelkins authored Sep 7, 2024
1 parent e87dd41 commit f259354
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 37 deletions.
39 changes: 22 additions & 17 deletions Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
47 changes: 40 additions & 7 deletions Sources/ClientRuntime/Networking/Http/HttpTelemetry.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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(
Expand Down
28 changes: 15 additions & 13 deletions Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit f259354

Please sign in to comment.