New Version 2.0 API #92

fabianfett · 2023-09-14T13:25:31Z

Motivation

Swift Prometheus has been in development since 2018. The previous maintainer (@MrLotU) stepped down about a year ago. Since then @ktoso and @fabianfett have maintained the existing code base.

In order to improve performance in the Prometheus library, we want to replace the existing implementation with a new one for version 2.0. While we do this we also want to get the general API closer to what the prometheus project recommends in their Writing client libraries guide. This means that we will also start to enforce Prometheus rules. For example having two counter with the same name, but one with labels and one without, is illegal in the Prometheus format. Lastly we want to remove the SwiftNIO dependency that is not needed anymore.

I'm well aware that this is a very large PR that changes lots of moving things.

Changes

Remove SwiftNIO dependency. Vendor in NIOLock from NIOConcurrencyHelpers
Replace PrometheusClient with PrometheusCollectorRegistry
Remove Summary implementation
Replace Counter, Gauge and Histogram implementations
Add Sendability annotations
Require Swift 5.7
Enforces that label names remain constant for a metric name.

Performance difference

In simple testing scenarios:

Exporting values from 1k counters into the prometheus format: ~10x speedup
Exporting values from 1k gauges into the prometheus format: ~10x speedup
Exporting values from 1k histograms into the prometheus format: ~25x speedup

Differences to Writing client libraries guide

There is no explicit Collector protocol, that users could implement to create their own instruments.
- Counter, Gauge and Histogram should implement Collector
Currently the PrometheusCollectorRegistry does not offer a register and unregister method for Collectors
There is no API to create a Counter/Gauge/Histogram that is not registered with a PrometheusCollectorRegistry
Collectors can't be registered with multiple different PrometheusCollectorRegistrys
We don't advertise a default collector

Do we want to offer this API? With the current patch we could add this API later.

FranzBusch

Really nice! Left some comments inline

FranzBusch · 2023-09-15T08:24:17Z

.swiftformat

@@ -0,0 +1,24 @@
+# file options


In all of our recent projects, we actually adopted swift-format since it has a more stable formatting between versions. Would recommend picking it here as well.

Can we change this in a follow up? #94

FranzBusch · 2023-09-15T08:26:27Z

Sources/Prometheus/Counter.swift

+import Atomics
+import CoreMetrics
+
+public final class Counter: Sendable {


Oh we require that all metric backing handlers are classes? This is a bit sad since this could just be a struct with reference semantics.

Second question, does this need to be public?

Oh we require that all metric backing handlers are classes? This is a bit sad since this could just be a struct with reference semantics.

I eventually want to change swift-metrics to not require AnyObject anymore. This will make NoOps much cheaper.

Second question, does this need to be public?

Yes. I think users should be able to use the Prometheus lib without going through swift-metrics.

Agreed on both, eventually we can do a revamp of metrics lib. It's one of the earliest and we've learned much since.

Yes, types should be public, prom has more detailed semantics than swift metrics so an "app" can definitely use it direcly

FranzBusch · 2023-09-15T08:26:55Z

Sources/Prometheus/Counter.swift

+        self.labels = labels
+
+        var prerendered = [UInt8]()
+        prerendered.reserveCapacity(64)


Could you comment why we need to reserve 64 here?

FranzBusch · 2023-09-15T08:27:59Z

Sources/Prometheus/Docs.docc/index.md

+
+A prometheus client library for Swift.
+
+## Overview


Can we add installation instructions to the index here as well. Allows us to point people directly at the documentation.

Can we do this in a follow up?

Sources/Prometheus/Counter.swift

FranzBusch · 2023-09-15T08:46:12Z

Sources/Prometheus/Histogram.swift

+public typealias ValueHistogram = Histogram<Double>
+
+public final class Histogram<Value: Bucketable>: @unchecked Sendable {
+    private let lock = NIOLock()


Can we please use a NIOLockedValueBox here. We are really really encouraging everyone to not just put @unchcked Sendable on their types but rather stick everything into the locked value box since that only has condition Sendable conformance and we have come across a few places where people used a lock and incorrectly declared @unchecked Sendable.

FranzBusch · 2023-09-15T08:46:51Z

Sources/Prometheus/PrometheusCollectorRegistry.swift

+///
+/// To use a ``PrometheusCollectorRegistry`` with `swift-metrics` use the ``PrometheusMetricsFactory``.
+public final class PrometheusCollectorRegistry: @unchecked Sendable {
+    // Sendability is enforced through the internal `lock`


Same here please

FranzBusch · 2023-09-15T08:48:05Z

Sources/Prometheus/PrometheusMetricsFactory.swift

+
+import CoreMetrics
+
+/// A wrapper around ``PrometheusClient`` to implement the `swift-metrics` `MetricsFactory` protocol


There is no PrometheusClient anymore

FranzBusch · 2023-09-15T08:48:14Z

Sources/Prometheus/PrometheusMetricsFactory.swift

+
+/// A wrapper around ``PrometheusClient`` to implement the `swift-metrics` `MetricsFactory` protocol
+public struct PrometheusMetricsFactory: Sendable {
+    /// The underlying ``PrometheusClient`` that is used to generate


Same here ;)

FranzBusch · 2023-09-15T08:48:56Z

Sources/Prometheus/PrometheusMetricsFactory.swift

+    public var valueHistogramBuckets: [String: [Double]]
+
+    public var labelAndDimensionSanitizer: @Sendable (_ label: String, _ dimensions: [(String, String)]) -> (String, [(String, String)])
+
+    public init(client: PrometheusCollectorRegistry) {


Can we get some docs on those three things?

Also do I have to create two things now to bootstrap prometheus? Both the registry and the factory? Couldn't we just default this parameter here to .init()?

I wonder if we should create a new one, or if we should provide a .default singleton. I think I'd be more in favor of the .default, wdyt?

I don't see a big benefit on a default singleton here and we avoided introducing singletons in AHC because we want to discuss this in the SSWG in a broader scope. So I would say we shouldn't start introducing it here. Just .init() default seems fine to me.

ktoso · 2023-09-15T12:45:21Z

Sources/Prometheus/Counter.swift

+        if let prerenderedLabels = Self.prerenderLabels(labels) {
+            prerendered.append(UInt8(ascii: "{"))
+            prerendered.append(contentsOf: prerenderedLabels)
+            prerendered.append(contentsOf: #"} "#.utf8)


can't use ascii here?

ktoso · 2023-09-15T12:46:08Z

Sources/Prometheus/Counter.swift

+    }
+
+    public func increment(by amount: Int64) {
+        precondition(amount >= 0)


Can we give those preconditions good messages always please? It's much nicer user experience when crashes contain message

FWIW crashed don't contain precondition messages. Only fatal errors messages are included. I have personally switched over to fatal error exclusively because of that reason

ktoso · 2023-09-15T12:47:24Z

Sources/Prometheus/Counter.swift

+
+public final class Counter: Sendable {
+    let intAtomic = ManagedAtomic(Int64(0))
+    let floatAtomic = ManagedAtomic(Double(0).bitPattern)


nit but it's weird to call a Double property "float"; yeah "floating point" but it's a double ->

Suggested change

let floatAtomic = ManagedAtomic(Double(0).bitPattern)

let doubleAtomic = ManagedAtomic(Double(0).bitPattern)

ktoso · 2023-09-15T12:48:25Z

Sources/Prometheus/Counter.swift

+
+    let name: String
+    let labels: [(String, String)]
+    let prerenderedExport: [UInt8]


That's the key to the perf wins I suppose, good 👍

ktoso · 2023-09-15T12:49:08Z

Sources/Prometheus/Counter.swift

+        // We busy loop here until we can update the atomic successfully.
+        // Using relaxed ordering here is sufficient, since the as-if rules guarantess that
+        // the following operations are executed in the order presented here. Every statement
+        // depends on the execution before.


It's clear here but thx for comments around busy loops, always good to document those 💯

ktoso · 2023-09-15T12:49:57Z

Sources/Prometheus/Gauge.swift

+import Atomics
+import CoreMetrics
+
+public final class Gauge: Sendable {


In follow up please add docs to all those types

ktoso · 2023-09-15T12:51:10Z

Sources/Prometheus/Gauge.swift

+        self.atomic.store(value.bitPattern, ordering: .relaxed)
+    }
+
+    public func increment(by amount: Double = 1.0) {


A bit inconsistent, on purpose? The counter had Int64 variants of APIs as well, no need here?

nope not un porpose

ktoso · 2023-09-15T12:51:34Z

Sources/Prometheus/Histogram.swift

+
+import CoreMetrics
+
+public protocol Bucketable: AdditiveArithmetic, Comparable, Sendable {


Docs for all those types

ktoso · 2023-09-15T12:52:11Z

Sources/Prometheus/Histogram.swift

+    var bucketRepresentation: String { get }
+}
+
+public typealias TimeHistogram = Histogram<Duration>


Hmmm.... not really "Time", or is it from prom specs...? If not, then mirror the contained value maybe?
I'd expect DurationHistogram

Renamed to DurationHistogram

ktoso · 2023-09-15T12:56:09Z

Sources/Prometheus/PrometheusCollectorRegistry.swift

+
+    // MARK: Destroying Metrics
+
+    public func destroyCounter(_ counter: Counter) {


They're pretty clear but please document.

E.g. if one can make a new one after a destroy etc

ktoso · 2023-09-15T12:56:49Z

Sources/Prometheus/PrometheusCollectorRegistry.swift

+    // MARK: Emitting
+
+    public func emit(into buffer: inout [UInt8]) {
+        let metrics = self.box.withLockedValue { $0 }


I know this is the NIO type, but tbh for such there should be a get() on it, WDYT?

ktoso

LGTM overall, minor comments inline.

Andrewangeta · 2023-09-15T13:38:16Z

Looks nice!

ktoso · 2023-09-16T07:19:37Z

Feel free to merge at will btw 👍

FranzBusch

Everything looks good but we are missing an implementation of the newly added Meter type.

In swift-server#92, we added a Histogram that is generic over the observed type. This means that each Histogram was also carrying a generic argument. To work around this we created typealiases for `DurationHistogram` and `ValueHistogram`. The motivation behind this was to preserve as much detailed information as possible. Duration can be more precise than Double. However in the grand scheme of things we now believe that this is overkill and a simpler to use Histogram type is more valuable. Thus this patch removes the generic argument from Histogram.

New Version 2.0 API

84a0a97

fabianfett requested review from ktoso and FranzBusch September 14, 2023 13:25

swiftformat

b2cf8f3

FranzBusch reviewed Sep 15, 2023

View reviewed changes

Code review NIOLockedValueBox

69b44ae

ktoso reviewed Sep 15, 2023

View reviewed changes

ktoso approved these changes Sep 15, 2023

View reviewed changes

fabianfett added 2 commits September 15, 2023 16:24

Code review II

a198d5c

Rename TimeHistogram -> DurationHistogram

b3071b5

Review 3

11d4480

fabianfett requested a review from FranzBusch September 18, 2023 07:46

doc comment

77f4c39

FranzBusch reviewed Sep 18, 2023

View reviewed changes

FranzBusch approved these changes Sep 18, 2023

View reviewed changes

fabianfett merged commit 46b8b95 into swift-server:main Sep 18, 2023
5 checks passed

fabianfett deleted the ff-v2 branch September 18, 2023 12:04

fabianfett added this to the 2.0 milestone Sep 25, 2023

fabianfett mentioned this pull request Sep 26, 2023

Remove generic argument from Histogram #103

Open


		import CoreMetrics

		/// A wrapper around ``PrometheusClient`` to implement the `swift-metrics` `MetricsFactory` protocol

	let floatAtomic = ManagedAtomic(Double(0).bitPattern)
	let doubleAtomic = ManagedAtomic(Double(0).bitPattern)


		import CoreMetrics

		public protocol Bucketable: AdditiveArithmetic, Comparable, Sendable {


		// MARK: Destroying Metrics

		public func destroyCounter(_ counter: Counter) {

New Version 2.0 API #92

New Version 2.0 API #92

Conversation

fabianfett commented Sep 14, 2023

Motivation

Changes

Performance difference

Differences to Writing client libraries guide

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

Andrewangeta commented Sep 15, 2023

ktoso commented Sep 16, 2023

FranzBusch left a comment

Choose a reason for hiding this comment

fabianfett Sep 15, 2023 •

edited

ktoso Sep 15, 2023 •

edited