Skip to content

Commit

Permalink
Merge pull request #349 from nachoBonafonte/main
Browse files Browse the repository at this point in the history
Fix ActivityContextManager  contextMap is never being cleaned up
  • Loading branch information
Ignacio Bonafonte authored Nov 14, 2022
2 parents 610b0d4 + e2e0efa commit 61a3258
Show file tree
Hide file tree
Showing 8 changed files with 254 additions and 16 deletions.
1 change: 0 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ let package = Package(
exclude: ["README.md"]),
.target(name: "SwiftMetricsShim",
dependencies: ["OpenTelemetrySdk",
.product(name: "NIO", package: "swift-nio"),
.product(name: "CoreMetrics", package: "swift-metrics")],
path: "Sources/Importers/SwiftMetricsShim",
exclude: ["README.md"]),
Expand Down
198 changes: 198 additions & 0 deletions Sources/Importers/SwiftMetricsShim/Locks.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift Metrics API open source project
//
// Copyright (c) 2018-2019 Apple Inc. and the Swift Metrics API project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of Swift Metrics API project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
import Darwin
#else
import Glibc
#endif

/// A threading lock based on `libpthread` instead of `libdispatch`.
///
/// This object provides a lock on top of a single `pthread_mutex_t`. This kind
/// of lock is safe to use with `libpthread`-based threading models, such as the
/// one used by NIO.
internal final class Lock {
fileprivate let mutex: UnsafeMutablePointer<pthread_mutex_t> = UnsafeMutablePointer.allocate(capacity: 1)

/// Create a new lock.
public init() {
let err = pthread_mutex_init(self.mutex, nil)
precondition(err == 0, "pthread_mutex_init failed with error \(err)")
}

deinit {
let err = pthread_mutex_destroy(self.mutex)
precondition(err == 0, "pthread_mutex_destroy failed with error \(err)")
self.mutex.deallocate()
}

/// Acquire the lock.
///
/// Whenever possible, consider using `withLock` instead of this method and
/// `unlock`, to simplify lock handling.
public func lock() {
let err = pthread_mutex_lock(self.mutex)
precondition(err == 0, "pthread_mutex_lock failed with error \(err)")
}

/// Release the lock.
///
/// Whenever possible, consider using `withLock` instead of this method and
/// `lock`, to simplify lock handling.
public func unlock() {
let err = pthread_mutex_unlock(self.mutex)
precondition(err == 0, "pthread_mutex_unlock failed with error \(err)")
}
}

extension Lock {
/// Acquire the lock for the duration of the given block.
///
/// This convenience method should be preferred to `lock` and `unlock` in
/// most situations, as it ensures that the lock will be released regardless
/// of how `body` exits.
///
/// - Parameter body: The block to execute while holding the lock.
/// - Returns: The value returned by the block.
@inlinable
internal func withLock<T>(_ body: () throws -> T) rethrows -> T {
self.lock()
defer {
self.unlock()
}
return try body()
}

// specialise Void return (for performance)
@inlinable
internal func withLockVoid(_ body: () throws -> Void) rethrows {
try self.withLock(body)
}
}

/// A threading lock based on `libpthread` instead of `libdispatch`.
///
/// This object provides a lock on top of a single `pthread_mutex_t`. This kind
/// of lock is safe to use with `libpthread`-based threading models, such as the
/// one used by NIO.
internal final class ReadWriteLock {
fileprivate let rwlock: UnsafeMutablePointer<pthread_rwlock_t> = UnsafeMutablePointer.allocate(capacity: 1)

/// Create a new lock.
public init() {
let err = pthread_rwlock_init(self.rwlock, nil)
precondition(err == 0, "pthread_rwlock_init failed with error \(err)")
}

deinit {
let err = pthread_rwlock_destroy(self.rwlock)
precondition(err == 0, "pthread_rwlock_destroy failed with error \(err)")
self.rwlock.deallocate()
}

/// Acquire a reader lock.
///
/// Whenever possible, consider using `withLock` instead of this method and
/// `unlock`, to simplify lock handling.
public func lockRead() {
let err = pthread_rwlock_rdlock(self.rwlock)
precondition(err == 0, "pthread_rwlock_rdlock failed with error \(err)")
}

/// Acquire a writer lock.
///
/// Whenever possible, consider using `withLock` instead of this method and
/// `unlock`, to simplify lock handling.
public func lockWrite() {
let err = pthread_rwlock_wrlock(self.rwlock)
precondition(err == 0, "pthread_rwlock_wrlock failed with error \(err)")
}

/// Release the lock.
///
/// Whenever possible, consider using `withLock` instead of this method and
/// `lock`, to simplify lock handling.
public func unlock() {
let err = pthread_rwlock_unlock(self.rwlock)
precondition(err == 0, "pthread_rwlock_unlock failed with error \(err)")
}
}

extension ReadWriteLock {
/// Acquire the reader lock for the duration of the given block.
///
/// This convenience method should be preferred to `lock` and `unlock` in
/// most situations, as it ensures that the lock will be released regardless
/// of how `body` exits.
///
/// - Parameter body: The block to execute while holding the lock.
/// - Returns: The value returned by the block.
@inlinable
internal func withReaderLock<T>(_ body: () throws -> T) rethrows -> T {
self.lockRead()
defer {
self.unlock()
}
return try body()
}

/// Acquire the writer lock for the duration of the given block.
///
/// This convenience method should be preferred to `lock` and `unlock` in
/// most situations, as it ensures that the lock will be released regardless
/// of how `body` exits.
///
/// - Parameter body: The block to execute while holding the lock.
/// - Returns: The value returned by the block.
@inlinable
internal func withWriterLock<T>(_ body: () throws -> T) rethrows -> T {
self.lockWrite()
defer {
self.unlock()
}
return try body()
}

// specialise Void return (for performance)
@inlinable
internal func withReaderLockVoid(_ body: () throws -> Void) rethrows {
try self.withReaderLock(body)
}

// specialise Void return (for performance)
@inlinable
internal func withWriterLockVoid(_ body: () throws -> Void) rethrows {
try self.withWriterLock(body)
}
}
1 change: 0 additions & 1 deletion Sources/Importers/SwiftMetricsShim/SwiftMetricsShim.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

import CoreMetrics
import NIOConcurrencyHelpers
import OpenTelemetryApi

public class OpenTelemetrySwiftMetrics: MetricsFactory {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class ActivityContextManager: ContextManager {
}

var objectScope = NSMapTable<AnyObject, ScopeElement>(keyOptions: .weakMemory, valueOptions: .strongMemory)

var contextMap = [os_activity_id_t: [String: AnyObject]]()

func getCurrentContextValue(forKey key: OpenTelemetryContextKeys) -> AnyObject? {
Expand Down Expand Up @@ -69,6 +68,14 @@ class ActivityContextManager: ContextManager {
}

func removeContextValue(forKey key: OpenTelemetryContextKeys, value: AnyObject) {
let activityIdent = os_activity_get_identifier(OS_ACTIVITY_CURRENT, nil)
rlock.lock()
contextMap[activityIdent]?[key.rawValue] = nil
if contextMap[activityIdent]?.isEmpty ?? false {
contextMap[activityIdent] = nil
}
print("ContextMap Count: \(contextMap.count)")
rlock.unlock()
if let scope = objectScope.object(forKey: value) {
var scope = scope.scope
os_activity_scope_leave(&scope)
Expand Down
31 changes: 24 additions & 7 deletions Sources/OpenTelemetryApi/Trace/PropagatedSpan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Foundation
/// The PropagatedSpan is the default Span that is used when no Span
/// implementation is available. All operations are no-op except context propagation.
class PropagatedSpan: Span {
var name: String = ""
var name: String

var kind: SpanKind

Expand All @@ -25,23 +25,40 @@ class PropagatedSpan: Span {
/// Returns a DefaultSpan with an invalid SpanContext.
convenience init() {
let invalidContext = SpanContext.create(traceId: TraceId(),
spanId: SpanId(),
traceFlags: TraceFlags(),
traceState: TraceState())
self.init(context: invalidContext, kind: .client)
spanId: SpanId(),
traceFlags: TraceFlags(),
traceState: TraceState())
self.init(name: "", context: invalidContext, kind: .client)
}

/// Creates an instance of this class with the SpanContext.
/// - Parameter context: the SpanContext
convenience init(context: SpanContext) {
self.init(context: context, kind: .client)
self.init(name: "", context: context, kind: .client)
}

/// Creates an instance of this class with the SpanContext and Span kind
/// - Parameters:
/// - context: the SpanContext
/// - kind: the SpanKind
init(context: SpanContext, kind: SpanKind) {
convenience init(context: SpanContext, kind: SpanKind) {
self.init(name: "", context: context, kind: kind)
}

/// Creates an instance of this class with the SpanContext and Span name
/// - Parameters:
/// - context: the SpanContext
/// - kind: the SpanKind
convenience init(name: String, context: SpanContext) {
self.init(name: name, context: context, kind: .client)
}

/// Creates an instance of this class with the SpanContext, Span kind and name
/// - Parameters:
/// - context: the SpanContext
/// - kind: the SpanKind
init(name: String, context: SpanContext, kind: SpanKind) {
self.name = name
self.context = context
self.kind = kind
}
Expand Down
6 changes: 4 additions & 2 deletions Sources/OpenTelemetryApi/Trace/PropagatedSpanBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,19 @@ class PropagatedSpanBuilder: SpanBuilder {
private var tracer: Tracer
private var isRootSpan: Bool = false
private var spanContext: SpanContext?
private var spanName: String

init(tracer: Tracer, spanName: String) {
self.tracer = tracer
self.spanName = spanName
}

@discardableResult public func startSpan() -> Span {
if spanContext == nil, !isRootSpan {
spanContext = OpenTelemetry.instance.contextProvider.activeSpan?.context
}
return PropagatedSpan(context: spanContext ?? SpanContext.create(traceId: TraceId.random(),
return PropagatedSpan(name: spanName,
context: spanContext ?? SpanContext.create(traceId: TraceId.random(),
spanId: SpanId.random(),
traceFlags: TraceFlags(),
traceState: TraceState()))
Expand Down Expand Up @@ -63,5 +66,4 @@ class PropagatedSpanBuilder: SpanBuilder {
func setActive(_ active: Bool) -> Self {
return self
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class DefaultBaggageManagerTests: XCTestCase {
XCTAssert(self.defaultBaggageManager.getCurrentBaggage() === self.baggage)
semaphore2.signal()
semaphore.wait()
XCTAssert(self.defaultBaggageManager.getCurrentBaggage() === self.baggage)
XCTAssertNil(self.defaultBaggageManager.getCurrentBaggage())
expec.fulfill()
}
semaphore2.wait()
Expand Down
Loading

0 comments on commit 61a3258

Please sign in to comment.