move labels equality check out of critical section in summary/histogram (#61)

avolokhov · web-flow · commit ba329daafa03 · 2021-08-31T10:25:09.000+02:00
* move labels equality check out of critical section in summary/histogram

* preconditions, addressing review comments
diff --git a/Sources/Prometheus/MetricTypes/Histogram.swift b/Sources/Prometheus/MetricTypes/Histogram.swift
@@ -6,12 +6,12 @@ import Dispatch
 /// See https://prometheus.io/docs/concepts/metric_types/#Histogram
 public struct Buckets: ExpressibleByArrayLiteral {
     public typealias ArrayLiteralElement = Double
-    
+
     public init(arrayLiteral elements: Double...) {
         self.init(elements)
     }
-    
-    fileprivate init (_ r: [Double]) {
+
+    fileprivate init(_ r: [Double]) {
         if r.isEmpty {
             self = Buckets.defaultBuckets
             return
@@ -24,13 +24,13 @@ public struct Buckets: ExpressibleByArrayLiteral {
         assert(Array(Set(r)).sorted(by: <) == r.sorted(by: <), "Buckets contain duplicate values.")
         self.buckets = r
     }
-    
+
     /// The upper bounds
     public let buckets: [Double]
-        
+
     /// Default buckets used by Histograms
     public static let defaultBuckets: Buckets = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10]
-    
+
     /// Create linear buckets used by Histograms
     ///
     /// - Parameters:
@@ -42,7 +42,7 @@ public struct Buckets: ExpressibleByArrayLiteral {
         let arr = (0..<count).map { Double(start) + Double($0) * Double(width) }
         return Buckets(arr)
     }
-    
+
     /// Create exponential buckets used by Histograms
     ///
     ///  - Parameters:
@@ -88,28 +88,28 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
     public let name: String
     /// Help text of this Histogram, optional
     public let help: String?
-    
+
     /// Type of the metric, used for formatting
     public let _type: PromMetricType = .histogram
-    
+
     /// Bucketed values for this Histogram
     private var buckets: [PromCounter<NumType, EmptyLabels>] = []
-    
+
     /// Buckets used by this Histogram
     internal let upperBounds: [Double]
-    
+
     /// Labels for this Histogram
     internal let labels: Labels
-    
+
     /// Sub Histograms for this Histogram
     fileprivate var subHistograms: [Labels: PromHistogram<NumType, Labels>] = [:]
-    
+
     /// Total value of the Histogram
     private let sum: PromCounter<NumType, EmptyLabels>
-    
+
     /// Lock used for thread safety
     private let lock: Lock
-    
+
     /// Creates a new Histogram
     ///
     /// - Parameters:
@@ -125,18 +125,18 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
         self.prometheus = p
 
         self.sum = .init("\(self.name)_sum", nil, 0, p)
-        
+
         self.labels = labels
-        
+
         self.upperBounds = buckets.buckets
-        
+
         self.lock = Lock()
-        
+
         buckets.buckets.forEach { _ in
             self.buckets.append(.init("\(name)_bucket", nil, 0, p))
         }
     }
-    
+
     /// Gets the metric string for this Histogram
     ///
     /// - Returns:
@@ -147,6 +147,8 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
         }
 
         var output = [String]()
+        // HELP/TYPE + (histogram + subHistograms) * (buckets + sum + count)
+        output.reserveCapacity(2 + (subHistograms.count + 1) * (buckets.count + 2))
 
         if let help = self.help {
             output.append("# HELP \(self.name) \(help)")
@@ -200,11 +202,9 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
     ///     - value: Value to observe
     ///     - labels: Labels to attach to the observed value
     public func observe(_ value: NumType, _ labels: Labels? = nil) {
-        if let labels = labels, type(of: labels) != type(of: EmptySummaryLabels()) {
-            let his: PromHistogram<NumType, Labels> = self.lock.withLock {
-                self.getOrCreateHistogram(with: labels)
-            }
-            his.observe(value)
+        if let labels = labels, type(of: labels) != type(of: EmptyHistogramLabels()) {
+            self.getOrCreateHistogram(with: labels)
+                .observe(value)
         }
         self.sum.inc(value)
 
@@ -215,7 +215,7 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
             }
         }
     }
-    
+
     /// Time the duration of a closure and observe the resulting time in seconds.
     ///
     /// - parameters:
@@ -233,14 +233,39 @@ public class PromHistogram<NumType: DoubleRepresentable, Labels: HistogramLabels
 
     /// Helper for histograms & labels
     fileprivate func getOrCreateHistogram(with labels: Labels) -> PromHistogram<NumType, Labels> {
-        let histogram = self.subHistograms[labels]
-        if let histogram = histogram, histogram.name == self.name, histogram.help == self.help {
+        let subHistograms = lock.withLock { self.subHistograms }
+        if let histogram = subHistograms[labels] {
+            precondition(histogram.name == self.name,
+                         """
+                         Somehow got 2 subHistograms with the same data type  / labels 
+                         but different names: expected \(self.name), got \(histogram.name)
+                         """)
+            precondition(histogram.help == self.help,
+                         """
+                         Somehow got 2 subHistograms with the same data type  / labels 
+                         but different help messages: expected \(self.help ?? "nil"), got \(histogram.help ?? "nil")
+                         """)
             return histogram
         } else {
-            guard let prometheus = prometheus else { fatalError("Lingering Histogram") }
-            let newHistogram = PromHistogram(self.name, self.help, labels, Buckets(self.upperBounds), prometheus)
-            self.subHistograms[labels] = newHistogram
-            return newHistogram
+            return lock.withLock {
+                if let histogram = subHistograms[labels] {
+                    precondition(histogram.name == self.name,
+                                 """
+                                 Somehow got 2 subHistograms with the same data type  / labels 
+                                 but different names: expected \(self.name), got \(histogram.name)
+                                 """)
+                    precondition(histogram.help == self.help,
+                                 """
+                                 Somehow got 2 subHistograms with the same data type  / labels 
+                                 but different help messages: expected \(self.help ?? "nil"), got \(histogram.help ?? "nil")
+                                 """)
+                    return histogram
+                }
+                guard let prometheus = prometheus else { fatalError("Lingering Histogram") }
+                let newHistogram = PromHistogram(self.name, self.help, labels, Buckets(self.upperBounds), prometheus)
+                self.subHistograms[labels] = newHistogram
+                return newHistogram
+            }
         }
     }
 }
diff --git a/Sources/Prometheus/MetricTypes/Summary.swift b/Sources/Prometheus/MetricTypes/Summary.swift
@@ -53,7 +53,7 @@ public class PromSummary<NumType: DoubleRepresentable, Labels: SummaryLabels>: P
     internal let quantiles: [Double]
     
     /// Sub Summaries for this Summary
-    fileprivate var subSummaries: [PromSummary<NumType, Labels>] = []
+    fileprivate var subSummaries: [Labels: PromSummary<NumType, Labels>] = [:]
     
     /// Lock used for thread safety
     private let lock: Lock
@@ -100,6 +100,8 @@ public class PromSummary<NumType: DoubleRepresentable, Labels: SummaryLabels>: P
         }
 
         var output = [String]()
+        // HELP/TYPE + (summary + subSummaries) * (quantiles + sum + count)
+        output.reserveCapacity(2 + (subSummaries.count + 1) * (quantiles.count + 2))
 
         if let help = self.help {
             output.append("# HELP \(self.name) \(help)")
@@ -117,7 +119,7 @@ public class PromSummary<NumType: DoubleRepresentable, Labels: SummaryLabels>: P
         output.append("\(self.name)_count\(labelsString) \(self.count.get())")
         output.append("\(self.name)_sum\(labelsString) \(format(self.sum.get().doubleValue))")
 
-        subSummaries.forEach { subSum in
+        subSummaries.values.forEach { subSum in
             var subSumLabels = subSum.labels
             let subSumValues = lock.withLock { subSum.values }
             calculateQuantiles(quantiles: self.quantiles, values: subSumValues.map { $0.doubleValue }).sorted { $0.key < $1.key }.forEach { (arg) in
@@ -162,13 +164,13 @@ public class PromSummary<NumType: DoubleRepresentable, Labels: SummaryLabels>: P
     ///     - value: Value to observe
     ///     - labels: Labels to attach to the observed value
     public func observe(_ value: NumType, _ labels: Labels? = nil) {
+        if let labels = labels, type(of: labels) != type(of: EmptySummaryLabels()) {
+            let sum = self.getOrCreateSummary(withLabels: labels)
+            sum.observe(value)
+        }
+        self.count.inc(1)
+        self.sum.inc(value)
         self.lock.withLock {
-            if let labels = labels, type(of: labels) != type(of: EmptySummaryLabels()) {
-                guard let sum = self.prometheus?.getOrCreateSummary(withLabels: labels, forSummary: self) else { fatalError("Lingering Summary") }
-                sum.observe(value)
-            }
-            self.count.inc(1)
-            self.sum.inc(value)
             if self.values.count == self.capacity {
                 _ = self.values.popFirst()
             }
@@ -190,22 +192,42 @@ public class PromSummary<NumType: DoubleRepresentable, Labels: SummaryLabels>: P
         }
         return try body()
     }
-}
-
-extension PrometheusClient {
-    /// Helper for summaries & labels
-    fileprivate func getOrCreateSummary<T: Numeric, U: SummaryLabels>(withLabels labels: U, forSummary summary: PromSummary<T, U>) -> PromSummary<T, U> {
-        let summaries = summary.subSummaries.filter { (metric) -> Bool in
-            guard metric.name == summary.name, metric.help == summary.help, metric.labels == labels else { return false }
-            return true
-        }
-        if summaries.count > 2 { fatalError("Somehow got 2 summaries with the same data type") }
-        if let summary = summaries.first {
+    fileprivate func getOrCreateSummary(withLabels labels: Labels) -> PromSummary<NumType, Labels> {
+        let subSummaries = self.lock.withLock { self.subSummaries }
+        if let summary = subSummaries[labels] {
+            precondition(summary.name == self.name,
+                         """
+                         Somehow got 2 subSummaries with the same data type  / labels 
+                         but different names: expected \(self.name), got \(summary.name)
+                         """)
+            precondition(summary.help == self.help,
+                         """
+                         Somehow got 2 subSummaries with the same data type  / labels 
+                         but different help messages: expected \(self.help ?? "nil"), got \(summary.help ?? "nil")
+                         """)
             return summary
         } else {
-            let newSummary = PromSummary<T, U>(summary.name, summary.help, labels, summary.capacity, summary.quantiles, self)
-            summary.subSummaries.append(newSummary)
-            return newSummary
+            return lock.withLock {
+                if let summary = self.subSummaries[labels] {
+                    precondition(summary.name == self.name,
+                                 """
+                                 Somehow got 2 subSummaries with the same data type  / labels 
+                                 but different names: expected \(self.name), got \(summary.name)
+                                 """)
+                    precondition(summary.help == self.help,
+                                 """
+                                 Somehow got 2 subSummaries with the same data type  / labels 
+                                 but different help messages: expected \(self.help ?? "nil"), got \(summary.help ?? "nil")
+                                 """)
+                    return summary
+                }
+                guard let prometheus = prometheus else {
+                    fatalError("Lingering Summary")
+                }
+                let newSummary = PromSummary(self.name, self.help, labels, self.capacity, self.quantiles, prometheus)
+                self.subSummaries[labels] = newSummary
+                return newSummary
+            }
         }
     }
 }
diff --git a/Tests/SwiftPrometheusTests/HistogramTests.swift b/Tests/SwiftPrometheusTests/HistogramTests.swift
@@ -33,6 +33,40 @@ final class HistogramTests: XCTestCase {
         self.prom = nil
         try! self.group.syncShutdownGracefully()
     }
+
+    func testConcurrent() throws {
+        let prom = PrometheusClient()
+        let histogram = prom.createHistogram(forType: Double.self, named: "my_histogram",
+                                             helpText: "Histogram for testing",
+                                             buckets: Buckets.exponential(start: 1, factor: 2, count: 63),
+                                             labels: DimensionHistogramLabels.self)
+        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 8)
+        let semaphore = DispatchSemaphore(value: 2)
+        _ = elg.next().submit {
+            for _ in 1...1_000 {
+                let labels = DimensionHistogramLabels([("myValue", "1")])
+                let labels2 = DimensionHistogramLabels([("myValue", "2")])
+
+                histogram.observe(1.0, labels)
+                histogram.observe(1.0, labels2)
+            }
+            semaphore.signal()
+        }
+        _ = elg.next().submit {
+            for _ in 1...1_000 {
+                let labels = DimensionHistogramLabels([("myValue", "1")])
+                let labels2 = DimensionHistogramLabels([("myValue", "2")])
+
+                histogram.observe(1.0, labels2)
+                histogram.observe(1.0, labels)
+            }
+            semaphore.signal()
+        }
+        semaphore.wait()
+        try elg.syncShutdownGracefully()
+        XCTAssertTrue(histogram.collect().contains("my_histogram_count 4000.0"))
+        XCTAssertTrue(histogram.collect().contains("my_histogram_sum 4000.0"))
+    }
     
     func testHistogramSwiftMetrics() {
         let recorder = Recorder(label: "my_histogram")
diff --git a/Tests/SwiftPrometheusTests/SummaryTests.swift b/Tests/SwiftPrometheusTests/SummaryTests.swift
@@ -70,6 +70,39 @@ final class SummaryTests: XCTestCase {
         my_summary_sum{myValue="labels"} 123.0\n
         """)
     }
+
+    func testConcurrent() throws {
+        let prom = PrometheusClient()
+        let summary = prom.createSummary(forType: Double.self, named: "my_summary",
+                                             helpText: "Summary for testing",
+                                             labels: DimensionSummaryLabels.self)
+        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 8)
+        let semaphore = DispatchSemaphore(value: 2)
+        _ = elg.next().submit {
+            for _ in 1...1_000 {
+                let labels = DimensionSummaryLabels([("myValue", "1")])
+                let labels2 = DimensionSummaryLabels([("myValue", "2")])
+
+                summary.observe(1.0, labels)
+                summary.observe(1.0, labels2)
+            }
+            semaphore.signal()
+        }
+        _ = elg.next().submit {
+            for _ in 1...1_000 {
+                let labels = DimensionSummaryLabels([("myValue", "1")])
+                let labels2 = DimensionSummaryLabels([("myValue", "2")])
+
+                summary.observe(1.0, labels2)
+                summary.observe(1.0, labels)
+            }
+            semaphore.signal()
+        }
+        semaphore.wait()
+        try elg.syncShutdownGracefully()
+        XCTAssertTrue(summary.collect().contains("my_summary_count 4000.0"))
+        XCTAssertTrue(summary.collect().contains("my_summary_sum 4000.0"))
+    }
     
     func testSummaryWithPreferredDisplayUnit() {
         let summary = Timer(label: "my_summary", preferredDisplayUnit: .seconds)
