From de5d050e7bdbf11cd9b677acf78ab711d69d4928 Mon Sep 17 00:00:00 2001 From: Vinod Vydier Date: Tue, 19 Dec 2023 21:43:00 -0600 Subject: [PATCH 1/2] removeContextValue fix and added a Test for it --- .../Context/ActivityContextManager.swift | 12 +++--- .../Context/ActivityContextManagerTests.swift | 39 +++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/Sources/OpenTelemetryApi/Context/ActivityContextManager.swift b/Sources/OpenTelemetryApi/Context/ActivityContextManager.swift index 2f5596d5..40478e52 100644 --- a/Sources/OpenTelemetryApi/Context/ActivityContextManager.swift +++ b/Sources/OpenTelemetryApi/Context/ActivityContextManager.swift @@ -68,14 +68,12 @@ class ActivityContextManager: ContextManager { } func removeContextValue(forKey key: OpenTelemetryContextKeys, value: AnyObject) { - let activityIdent = os_activity_get_identifier(OS_ACTIVITY_CURRENT, nil) rlock.lock() - if let currentValue = contextMap[activityIdent]?[key.rawValue], - currentValue === value - { - contextMap[activityIdent]?[key.rawValue] = nil - if contextMap[activityIdent]?.isEmpty ?? false { - contextMap[activityIdent] = nil + + for (activityKey, activity) in contextMap where value === activity[key.rawValue] { + contextMap[activityKey]?[key.rawValue] = nil + if contextMap[activityKey]?.isEmpty ?? false { + contextMap[activityKey] = nil } } if let scope = objectScope.object(forKey: value) { diff --git a/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift b/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift index 28f40ad5..f9ba900f 100644 --- a/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift +++ b/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift @@ -308,6 +308,45 @@ class ActivityContextManagerTests: XCTestCase { XCTAssert(OpenTelemetry.instance.contextProvider.activeSpan === nil) } + + @available(macOS 10.15, iOS 13.0, tvOS 13.0, *) + func testRemoveContextValuesFromSpan() { + + //Create a span + let span1 = defaultTracer.spanBuilder(spanName: "span1").startSpan() + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: span1) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === span1) + + //Add it to one parent in one thread + let parent1 = defaultTracer.spanBuilder(spanName: "parent1").startSpan() + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent1) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent1) + + DispatchQueue.global().async { + let activeSpan = ActivityContextManager.instance.getCurrentContextValue(forKey: .span) + XCTAssert(activeSpan === parent1) + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: span1) + parent1.end() + } + + //Add it to another parent in another thread + let parent2 = defaultTracer.spanBuilder(spanName: "parent2").startSpan() + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent2) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent2) + + DispatchQueue.global().async { + let activeSpan = ActivityContextManager.instance.getCurrentContextValue(forKey: .span) + XCTAssert(activeSpan === parent2) + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: span1) + parent2.end() + } + + //remove all the contexts from the span and check if the Context is nil + sleep(1) + ActivityContextManager.instance.removeContextValue(forKey: .span, value: span1) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === nil) + } + @available(macOS 10.15, iOS 13.0, tvOS 13.0, *) func testActiveSpanIsKeptPerTaskAsync() async { let expectation1 = self.expectation(description: "firstSpan created") From 57249c7b798f3710e76b06b0d53b8b4d37541867 Mon Sep 17 00:00:00 2001 From: Vinod Vydier Date: Thu, 21 Dec 2023 11:58:49 -0600 Subject: [PATCH 2/2] modified testRemoveContextValuesFromSpan to call span.end() instead of explicitly calling removeContextValue --- .../Context/ActivityContextManagerTests.swift | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift b/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift index f9ba900f..f2f27255 100644 --- a/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift +++ b/Tests/OpenTelemetryApiTests/Context/ActivityContextManagerTests.swift @@ -318,11 +318,11 @@ class ActivityContextManagerTests: XCTestCase { XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === span1) //Add it to one parent in one thread - let parent1 = defaultTracer.spanBuilder(spanName: "parent1").startSpan() - ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent1) - XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent1) - DispatchQueue.global().async { + let parent1 = self.defaultTracer.spanBuilder(spanName: "parent1").startSpan() + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent1) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent1) + let activeSpan = ActivityContextManager.instance.getCurrentContextValue(forKey: .span) XCTAssert(activeSpan === parent1) ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: span1) @@ -330,20 +330,22 @@ class ActivityContextManagerTests: XCTestCase { } //Add it to another parent in another thread - let parent2 = defaultTracer.spanBuilder(spanName: "parent2").startSpan() - ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent2) - XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent2) - DispatchQueue.global().async { + let parent2 = self.defaultTracer.spanBuilder(spanName: "parent2").startSpan() + ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: parent2) + XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === parent2) + let activeSpan = ActivityContextManager.instance.getCurrentContextValue(forKey: .span) XCTAssert(activeSpan === parent2) ActivityContextManager.instance.setCurrentContextValue(forKey: .span, value: span1) parent2.end() } - //remove all the contexts from the span and check if the Context is nil sleep(1) - ActivityContextManager.instance.removeContextValue(forKey: .span, value: span1) + // Remove all the contexts from the span and check if the Context is nil + // Ending the span will remove all the context associated with it, dont need to call removeContextValue explicitly + // ActivityContextManager.instance.removeContextValue(forKey: .span, value: span1) + span1.end() XCTAssert(ActivityContextManager.instance.getCurrentContextValue(forKey: .span) === nil) }