-
Notifications
You must be signed in to change notification settings - Fork 45
add lifecycle metrics #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,9 @@ import Darwin | |
import Glibc | ||
#endif | ||
import Backtrace | ||
import CoreMetrics | ||
import Dispatch | ||
import Logging | ||
import Metrics | ||
|
||
// MARK: - LifecycleTask | ||
|
||
|
@@ -473,9 +473,12 @@ public class ComponentLifecycle: LifecycleTask { | |
guard case .idle = self.state else { | ||
preconditionFailure("invalid state, \(self.state)") | ||
} | ||
self.log("starting") | ||
self.state = .starting(queue) | ||
} | ||
|
||
self.log("starting") | ||
Counter(label: "\(self.label).lifecycle.start").increment() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is label risky i.e. containing spaces etc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm good question, do you think we should normalize it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was just thinking about it but I also just remembered that last time we said that the libs should sanitize, so we do e.g. in the prometheus one https://github.com/MrLotU/SwiftPrometheus/blob/f4d4adfeb1178e250a0ea427017fb4d1cc877de3/Sources/Prometheus/PrometheusMetrics.swift#L112 🤔 I guess it's fine 👍 |
||
|
||
if tasks.count == 0 { | ||
self.log(level: .notice, "no tasks provided") | ||
} | ||
|
@@ -517,7 +520,9 @@ public class ComponentLifecycle: LifecycleTask { | |
return callback(index, nil) | ||
} | ||
self.log("starting tasks [\(tasks[index].label)]") | ||
let startTime = DispatchTime.now() | ||
start { error in | ||
Timer(label: "\(self.label).\(tasks[index].label).lifecycle.start").recordNanoseconds(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it be worth making some struct/enum with the labels such that it is easier to look in one place to see "aha, these are all the metric labels this lib exports"? I do this in some other project, it is easier to know what metrics are exposed then. So either some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have it here https://github.com/apple/swift-metrics/blob/44e8bfc7f5a0686e21d5d21a7ea5454e74ed3449/Sources/Metrics/Metrics.swift#L42-L44 maybe it's not released yet? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see but it's not in CoreMetrics hm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yes... I prefer not to introduce dependency on Foundation for low level libs if we can get away with it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah though this could live in core I guess, I'll ticketify -- it's just Dispatch dep, not foundation hm |
||
if let error = error { | ||
self.log(level: .error, "failed to start [\(tasks[index].label)]: \(error)") | ||
return callback(index, error) | ||
|
@@ -532,9 +537,12 @@ public class ComponentLifecycle: LifecycleTask { | |
|
||
private func _shutdown(on queue: DispatchQueue, tasks: [LifecycleTask], callback: @escaping () -> Void) { | ||
self.stateLock.withLock { | ||
log("shutting down") | ||
self.state = .shuttingDown(queue) | ||
} | ||
|
||
self.log("shutting down") | ||
Counter(label: "\(self.label).lifecycle.shutdown").increment() | ||
|
||
self.shutdownTask(on: queue, tasks: tasks.reversed(), index: 0, errors: nil) { errors in | ||
self.stateLock.withLock { | ||
guard case .shuttingDown = self.state else { | ||
|
@@ -555,8 +563,11 @@ public class ComponentLifecycle: LifecycleTask { | |
if index >= tasks.count { | ||
return callback(errors) | ||
} | ||
|
||
self.log("stopping tasks [\(tasks[index].label)]") | ||
let startTime = DispatchTime.now() | ||
shutdown { error in | ||
Timer(label: "\(self.label).\(tasks[index].label).lifecycle.shutdown").recordNanoseconds(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
var errors = errors | ||
if let error = error { | ||
if errors == nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it intentional? AFAIR libraries shouldn't normally depend on CoreMetrics unless implementing a MetricFactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreMetrics is useful when you do not want to pul in Foundation