Merge pull request #159 from Sherlouk/sherlouk/MetricsShimFix

Add Locks to SwiftMetricsShim
open-telemetry · Apr 8, 2021 · b8603e2 · b8603e2
2 parents 580cb19 + 8ac0f27
commit b8603e2
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 37 deletions.
diff --git a/Sources/SwiftMetricsShim/MetricHandlers.swift b/Sources/SwiftMetricsShim/MetricHandlers.swift
@@ -18,8 +18,8 @@ import OpenTelemetryApi
 
 class SwiftCounterMetric: CounterHandler, SwiftMetric {
 
-    var metricName: String
-    var metricType: MetricType = .counter
+    let metricName: String
+    let metricType: MetricType = .counter
     let counter: AnyCounterMetric<Int>
     let labels: [String: String]
 
@@ -41,8 +41,8 @@ class SwiftCounterMetric: CounterHandler, SwiftMetric {
 
 class SwiftGaugeMetric: RecorderHandler, SwiftMetric {
 
-    var metricName: String
-    var metricType: MetricType = .gauge
+    let metricName: String
+    let metricType: MetricType = .gauge
     let counter: AnyCounterMetric<Double>
     let labels: [String: String]
 
@@ -64,8 +64,8 @@ class SwiftGaugeMetric: RecorderHandler, SwiftMetric {
 
 class SwiftHistogramMetric: RecorderHandler, SwiftMetric {
 
-    var metricName: String
-    var metricType: MetricType = .histogram
+    let metricName: String
+    let metricType: MetricType = .histogram
     let measure: AnyMeasureMetric<Double>
     let labels: [String: String]
 
@@ -87,8 +87,8 @@ class SwiftHistogramMetric: RecorderHandler, SwiftMetric {
 
 class SwiftSummaryMetric: TimerHandler, SwiftMetric {
 
-    var metricName: String
-    var metricType: MetricType = .summary
+    let metricName: String
+    let metricType: MetricType = .summary
     let measure: AnyMeasureMetric<Double>
     let labels: [String: String]
 

diff --git a/Sources/SwiftMetricsShim/SwiftMetricsShim.swift b/Sources/SwiftMetricsShim/SwiftMetricsShim.swift
@@ -21,6 +21,7 @@ public class OpenTelemetrySwiftMetrics: MetricsFactory {
 
     internal let meter: Meter
     internal var metrics = [MetricKey: SwiftMetric]()
+    internal let lock = Lock()
 
     public init(meter: Meter) {
         self.meter = meter
@@ -31,14 +32,16 @@ public class OpenTelemetrySwiftMetrics: MetricsFactory {
     /// Counter: A counter is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on
     /// restart. For example, you can use a counter to represent the number of requests served, tasks completed, or errors.
     public func makeCounter(label: String, dimensions: [(String, String)]) -> CounterHandler {
-        if let existing = metrics[.init(name: label, type: .counter)] {
-            return existing as! CounterHandler
+        lock.withLock {
+            if let existing = metrics[.init(name: label, type: .counter)] {
+                return existing as! CounterHandler
+            }
+
+            let metric = SwiftCounterMetric(name: label, labels: dimensions.dictionary, meter: meter)
+
+            storeMetric(metric)
+            return metric
         }
-
-        let metric = SwiftCounterMetric(name: label, labels: dimensions.dictionary, meter: meter)
-
-        storeMetric(metric)
-        return metric
     }
 
     /// Recorder: A recorder collects observations within a time window (usually things like response sizes) and can provide aggregated information about the
@@ -48,33 +51,37 @@ public class OpenTelemetrySwiftMetrics: MetricsFactory {
     /// like temperatures or current memory usage, but also "counts" that can go up and down, like the number of active threads. Gauges are modeled as a
     /// Recorder with a sample size of 1 that does not perform any aggregation.
     public func makeRecorder(label: String, dimensions: [(String, String)], aggregate: Bool) -> RecorderHandler {
-        if let existing = metrics[.init(name: label, type: .histogram)] {
-            return existing as! RecorderHandler
+        lock.withLock {
+            if let existing = metrics[.init(name: label, type: .histogram)] {
+                return existing as! RecorderHandler
+            }
+
+            if let existing = metrics[.init(name: label, type: .gauge)] {
+                return existing as! RecorderHandler
+            }
+
+            let metric: SwiftMetric & RecorderHandler = aggregate ?
+                SwiftHistogramMetric(name: label, labels: dimensions.dictionary, meter: meter) :
+                SwiftGaugeMetric(name: label, labels: dimensions.dictionary, meter: meter)
+
+            storeMetric(metric)
+            return metric
         }
-
-        if let existing = metrics[.init(name: label, type: .gauge)] {
-            return existing as! RecorderHandler
-        }
-
-        let metric: SwiftMetric & RecorderHandler = aggregate ?
-            SwiftHistogramMetric(name: label, labels: dimensions.dictionary, meter: meter) :
-            SwiftGaugeMetric(name: label, labels: dimensions.dictionary, meter: meter)
-
-        storeMetric(metric)
-        return metric
     }
 
     /// Timer: A timer collects observations within a time window (usually things like request duration) and provides aggregated information about the data sample,
     /// for example min, max and various quantiles. It is similar to a Recorder but specialized for values that represent durations.
     public func makeTimer(label: String, dimensions: [(String, String)]) -> TimerHandler {
-        if let existing = metrics[.init(name: label, type: .summary)] {
-            return existing as! TimerHandler
+        lock.withLock {
+            if let existing = metrics[.init(name: label, type: .summary)] {
+                return existing as! TimerHandler
+            }
+
+            let metric = SwiftSummaryMetric(name: label, labels: dimensions.dictionary, meter: meter)
+
+            storeMetric(metric)
+            return metric
         }
-
-        let metric = SwiftSummaryMetric(name: label, labels: dimensions.dictionary, meter: meter)
-
-        storeMetric(metric)
-        return metric
     }
 
     private func storeMetric(_ metric: SwiftMetric) {
@@ -96,8 +103,10 @@ public class OpenTelemetrySwiftMetrics: MetricsFactory {
     }
 
     private func destroyMetric(_ metric: SwiftMetric?) {
-        if let name = metric?.metricName, let type = metric?.metricType {
-            metrics.removeValue(forKey: .init(name: name, type: type))
+        lock.withLock {
+            if let name = metric?.metricName, let type = metric?.metricType {
+                metrics.removeValue(forKey: .init(name: name, type: type))
+            }
         }
     }
 }
diff --git a/Tests/SwiftMetricsShim/SwiftMetricsShimTests.swift b/Tests/SwiftMetricsShim/SwiftMetricsShimTests.swift
@@ -125,5 +125,23 @@ class SwiftMetricsShimTests: XCTestCase {
         XCTAssertEqual(data.sum, 1000000000)
         XCTAssertNil(data.labels["label_one"])
     }
+
+    // MARK: - Test Concurrency
+
+    func testConcurrency() throws {
+        DispatchQueue.concurrentPerform(iterations: 5) { iteration in
+            let counter = Counter(label: "my_counter")
+            counter.increment()
+        }
+
+        meter.collect()
+
+        let metric = testProcessor.metrics[0]
+        let data = try XCTUnwrap(metric.data.last as? SumData<Int>)
+        XCTAssertEqual(metric.name, "my_counter")
+        XCTAssertEqual(metric.aggregationType, .intSum)
+        XCTAssertEqual(data.sum, 5)
+        XCTAssertNil(data.labels["label_one"])
+    }
 
 }