Skip to content

Commit 04e4d48

Browse files
YasumotoYasumoto
committed
Code review feedback
1 parent 6b53a9d commit 04e4d48

File tree

7 files changed

+53
-69
lines changed

7 files changed

+53
-69
lines changed

‎README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ To keep SwiftPrometheus as clean and lightweight as possible, there is no way of
101101

102102
This could look something like this:
103103
```swift
104-
router.get("/metrics") { request -> String in
104+
router.get("/metrics") { request -> Future<String> in
105105
let promise = req.eventLoop.newPromise(String.self)
106106
prom.getMetrics {
107107
promise.succeed(result: $0)

‎Sources/Prometheus/MetricTypes/Counter.swift

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,16 @@ public class Counter<NumType: Numeric, Labels: MetricLabels>: Metric, Prometheus
6767
/// - amount: Amount to increment the counter with
6868
/// - labels: Labels to attach to the value
6969
///
70-
public func inc(_ amount: NumType = 1, _ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
70+
public func inc(_ amount: NumType = 1, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
7171
prometheusQueue.async(flags: .barrier) {
7272
if let labels = labels {
7373
var val = self.metrics[labels] ?? self.initialValue
7474
val += amount
7575
self.metrics[labels] = val
76-
done(val)
7776
} else {
7877
self.value += amount
79-
done(self.value)
8078
}
79+
done()
8180
}
8281
}
8382

‎Sources/Prometheus/MetricTypes/Gauge.swift

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
4040

4141
/// Gets the metric string for this Gauge
4242
///
43-
/// - Returns:
44-
/// Newline seperated Prometheus formatted metric string
43+
/// - Parameters:
44+
/// - done: Callback passing a newline separated Prometheus-formatted metric string
4545
///
4646
public func getMetric(_ done: @escaping (String) -> Void) {
4747
prometheusQueue.async(flags: .barrier) {
@@ -69,14 +69,14 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
6969
/// - amount: Amount to set the gauge to
7070
/// - labels: Labels to attach to the value
7171
///
72-
public func set(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
72+
public func set(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
7373
prometheusQueue.async(flags: .barrier) {
7474
if let labels = labels {
7575
self.metrics[labels] = amount
76-
done(amount)
76+
done()
7777
} else {
7878
self.value = amount
79-
done(self.value)
79+
done()
8080
}
8181
}
8282
}
@@ -87,18 +87,17 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
8787
/// - amount: Amount to increment the Gauge with
8888
/// - labels: Labels to attach to the value
8989
/// - done: Completion handler
90-
/// - observedValue: Value written to the Gauge
9190
///
92-
public func inc(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
91+
public func inc(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
9392
prometheusQueue.async(flags: .barrier) {
9493
if let labels = labels {
9594
var val = self.metrics[labels] ?? self.initialValue
9695
val += amount
9796
self.metrics[labels] = val
98-
done(val)
97+
done()
9998
} else {
10099
self.value += amount
101-
done(self.value)
100+
done()
102101
}
103102
}
104103
}
@@ -109,9 +108,9 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
109108
/// - labels: Labels to attach to the value
110109
/// - done: Completion handler
111110
///
112-
public func inc(_ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
111+
public func inc(_ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
113112
self.inc(1, labels) {
114-
done($0)
113+
done()
115114
}
116115
}
117116

@@ -122,16 +121,16 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
122121
/// - labels: Labels to attach to the value
123122
/// - done: Completion handler
124123
///
125-
public func dec(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
124+
public func dec(_ amount: NumType, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
126125
prometheusQueue.async(flags: .barrier) {
127126
if let labels = labels {
128127
var val = self.metrics[labels] ?? self.initialValue
129128
val -= amount
130129
self.metrics[labels] = val
131-
done(val)
130+
done()
132131
} else {
133132
self.value -= amount
134-
done(self.value)
133+
done()
135134
}
136135
}
137136
}
@@ -141,9 +140,9 @@ public class Gauge<NumType: Numeric, Labels: MetricLabels>: Metric, PrometheusHa
141140
/// - Parameters:
142141
/// - labels: Labels to attach to the value
143142
///
144-
public func dec(_ labels: Labels? = nil, _ done: @escaping (NumType) -> Void = { _ in }) {
143+
public func dec(_ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
145144
self.dec(1, labels) {
146-
done($0)
145+
done()
147146
}
148147
}
149148

‎Sources/Prometheus/MetricTypes/Histogram.swift

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ extension HistogramLabels {
1414
}
1515
}
1616

17-
/// Prometheus Counter metric
17+
/// Prometheus Histogram metric
1818
///
1919
/// See https://prometheus.io/docs/concepts/metric_types/#Histogram
2020
public class Histogram<NumType: DoubleRepresentable, Labels: HistogramLabels>: Metric, PrometheusHandled {
@@ -71,8 +71,8 @@ public class Histogram<NumType: DoubleRepresentable, Labels: HistogramLabels>: M
7171

7272
/// Gets the metric string for this Histogram
7373
///
74-
/// - Returns:
75-
/// Newline seperated Prometheus formatted metric string
74+
/// - Parameters:
75+
/// - done: Newline separated Prometheus-formatted metric string
7676
public func getMetric(_ done: @escaping (String) -> Void) {
7777
prometheusQueue.async(flags: .barrier) {
7878
var output = [String]()
@@ -124,12 +124,11 @@ public class Histogram<NumType: DoubleRepresentable, Labels: HistogramLabels>: M
124124
/// - value: Value to observe
125125
/// - labels: Labels to attach to the observed value
126126
/// - done: Completion handler
127-
/// - observedValue: Value written to the histogram
128127
public func observe(_ value: NumType, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
129128
prometheusQueue.async(flags: .barrier) {
130129
if let labels = labels, type(of: labels) != type(of: EmptySummaryLabels()) {
131130
let his = self.prometheus.getOrCreateHistogram(with: labels, for: self)
132-
his.observe(value, nil, done)
131+
his.observe(value)
133132
}
134133
self.total.inc(value)
135134

@@ -139,11 +138,7 @@ public class Histogram<NumType: DoubleRepresentable, Labels: HistogramLabels>: M
139138
break
140139
}
141140
}
142-
143-
// Only run the callback once; not on both the label and without labels
144-
if labels == nil {
145-
done(value)
146-
}
141+
done()
147142
}
148143
}
149144
}

‎Sources/Prometheus/MetricTypes/Info.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,9 @@ public class Info<Labels: MetricLabels>: Metric, PrometheusHandled {
3939

4040
/// Gets the metric string for this Info
4141
///
42-
/// - Returns:
43-
/// Newline seperated Prometheus formatted metric string
42+
/// - Parameters:
43+
/// - done: Callback passing a newline separated Prometheus-formatted metric string
44+
///
4445
public func getMetric(_ done: @escaping (String) -> Void) {
4546
prometheusQueue.async(flags: .barrier) {
4647
var output = [String]()

‎Sources/Prometheus/MetricTypes/Summary.swift

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,6 @@ public class Summary<NumType: DoubleRepresentable, Labels: SummaryLabels>: Metri
7676
/// - done: Completion handler
7777
/// - metric: String value in prom-format
7878
///
79-
/// - Returns:
80-
/// Newline seperated Prometheus formatted metric string
8179
public func getMetric(_ done: @escaping (_ metric: String) -> Void) {
8280
prometheusQueue.async(flags: .barrier) {
8381
var output = [String]()
@@ -124,21 +122,17 @@ public class Summary<NumType: DoubleRepresentable, Labels: SummaryLabels>: Metri
124122
/// - value: Value to observe
125123
/// - labels: Labels to attach to the observed value
126124
/// - done: Completion handler
127-
/// - observedValue: Value written to the summary
125+
///
128126
public func observe(_ value: NumType, _ labels: Labels? = nil, _ done: @escaping () -> Void = { }) {
129127
prometheusQueue.async(flags: .barrier) {
130128
if let labels = labels, type(of: labels) != type(of: EmptySummaryLabels()) {
131129
let sum = self.prometheus.getOrCreateSummary(withLabels: labels, forSummary: self)
132-
sum.observe(value, nil, done)
130+
sum.observe(value)
133131
}
134132
self.count.inc(1)
135133
self.sum.inc(value)
136134
self.values.append(value)
137-
138-
// Only run the callback once; not on both the label and without labels
139-
if labels == nil {
140-
done(value)
141-
}
135+
done()
142136
}
143137
}
144138
}

‎Tests/SwiftPrometheusTests/SwiftPrometheusTests.swift

Lines changed: 24 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,17 @@ final class SwiftPrometheusTests: XCTestCase {
5757

5858
let counter = prom.createCounter(forType: Int.self, named: "my_counter", helpText: "Counter for testing", initialValue: 10, withLabelType: BaseLabels.self)
5959
XCTAssertEqual(counter.get(), 10)
60-
counter.inc(10) { int in
61-
XCTAssertEqual(counter.get(), 20)
62-
XCTAssertEqual(int, 20)
60+
counter.inc(10) {
61+
semaphore.signal()
6362
}
64-
counter.inc(10, BaseLabels(myValue: "labels")) { int in
65-
XCTAssertEqual(counter.get(), 20)
63+
semaphore.wait()
64+
XCTAssertEqual(counter.get(), 20)
65+
66+
counter.inc(10, BaseLabels(myValue: "labels")) {
6667
semaphore.signal()
6768
}
6869
semaphore.wait()
70+
XCTAssertEqual(counter.get(), 20)
6971
XCTAssertEqual(counter.get(BaseLabels(myValue: "labels")), 20)
7072

7173
counter.getMetric { metric in
@@ -80,24 +82,24 @@ final class SwiftPrometheusTests: XCTestCase {
8082

8183
let gauge = prom.createGauge(forType: Int.self, named: "my_gauge", helpText: "Gauge for testing", initialValue: 10, withLabelType: BaseLabels.self)
8284
XCTAssertEqual(gauge.get(), 10)
83-
gauge.inc(10) { _ in
85+
gauge.inc(10) {
8486
semaphore.signal()
8587
}
8688
semaphore.wait()
8789

8890
XCTAssertEqual(gauge.get(), 20)
89-
gauge.dec(12) { _ in
91+
gauge.dec(12) {
9092
semaphore.signal()
9193
}
9294
semaphore.wait()
9395

9496
XCTAssertEqual(gauge.get(), 8)
95-
gauge.set(20) { _ in
97+
gauge.set(20) {
9698
semaphore.signal()
9799
}
98100
semaphore.wait()
99101

100-
gauge.inc(10, BaseLabels(myValue: "labels")) { _ in
102+
gauge.inc(10, BaseLabels(myValue: "labels")) {
101103
semaphore.signal()
102104
}
103105
semaphore.wait()
@@ -116,25 +118,23 @@ final class SwiftPrometheusTests: XCTestCase {
116118
let semaphore = DispatchSemaphore(value: 0)
117119

118120
let histogram = prom.createHistogram(forType: Double.self, named: "my_histogram", helpText: "Histogram for testing", buckets: [0.5, 1, 2, 3, 5, Double.greatestFiniteMagnitude], labels: BaseHistogramLabels.self)
119-
histogram.observe(1) { _ in
121+
histogram.observe(1) {
120122
semaphore.signal()
121123
}
122124
semaphore.wait()
123-
124-
histogram.observe(2) { _ in
125+
histogram.observe(2) {
125126
semaphore.signal()
126127
}
127128
semaphore.wait()
128-
129-
histogram.observe(3) { _ in
129+
histogram.observe(3) {
130130
semaphore.signal()
131131
}
132132
semaphore.wait()
133-
134-
histogram.observe(3, .init(myValue: "labels")) { _ in
133+
histogram.observe(3, .init(myValue: "labels")) {
135134
semaphore.signal()
136135
}
137136
semaphore.wait()
137+
138138
var metricOutput = ""
139139
histogram.getMetric { metric in
140140
metricOutput = metric
@@ -158,34 +158,30 @@ final class SwiftPrometheusTests: XCTestCase {
158158
}
159159

160160
func testSummary() {
161-
let summary = prom.createSummary(forType: Double.self, named: "my_summary", helpText: "Summary for testing", quantiles: [0.5, 0.9, 0.99], labels: BaseSummaryLabels.self)
162161
let semaphore = DispatchSemaphore(value: 0)
163-
164-
summary.observe(1) { _ in
162+
163+
let summary = prom.createSummary(forType: Double.self, named: "my_summary", helpText: "Summary for testing", quantiles: [0.5, 0.9, 0.99], labels: BaseSummaryLabels.self)
164+
summary.observe(1) {
165165
semaphore.signal()
166166
}
167167
semaphore.wait()
168-
169-
summary.observe(2) { _ in
168+
summary.observe(2) {
170169
semaphore.signal()
171170
}
172171
semaphore.wait()
173-
174-
summary.observe(4) { _ in
172+
summary.observe(4) {
175173
semaphore.signal()
176174
}
177175
semaphore.wait()
178-
179-
summary.observe(10000) { _ in
176+
summary.observe(10000) {
180177
semaphore.signal()
181178
}
182179
semaphore.wait()
183-
184-
summary.observe(123, .init(myValue: "labels")) { _ in
180+
summary.observe(123, .init(myValue: "labels")) {
185181
semaphore.signal()
186182
}
187183
semaphore.wait()
188-
184+
189185
var outputMetric = ""
190186
summary.getMetric { metric in
191187
outputMetric = metric

0 commit comments

Comments
 (0)