add lifecycle metrics #93
Conversation
tomerd
force-pushed
the
feature/mettrics
branch
from
ea7e5ef to
0dc16e9
Compare
tomerd
force-pushed
the
feature/mettrics
branch
2 times, most recently
from
79855f8 to
52a0c8f
Compare
| self.state = .starting(queue) | ||
| } | ||
|
|
||
| self.log("starting") | ||
| Counter(label: "\(self.label).lifecycle.start").increment() |
There was a problem hiding this comment.
is label risky i.e. containing spaces etc?
There was a problem hiding this comment.
hmm good question, do you think we should normalize it?
There was a problem hiding this comment.
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 👍
tomerd
force-pushed
the
feature/mettrics
branch
from
52a0c8f to
28f7ef6
Compare
motivation: startup/shutdown metrics are important for real-life services, for example spike in start metrics indicates a crash-loop changes: * add start and shutdown counters to ComponentLifecycle * add start and shutdown timers to report duration of startup and shutdown operations
tomerd
force-pushed
the
feature/mettrics
branch
from
28f7ef6 to
39b9cd1
Compare
| 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.
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.
Sure not all that much here but it's a nice pattern;
So either some Labels.start(base: self.label) or Metrics.timer(base: self.label) I guess?
There was a problem hiding this comment.
We have func recordInterval(since: DispatchTime, now: DispatchTime = .now()) { for this type of nanosecond recording, consider using it
There was a problem hiding this comment.
recordInterval is not define in the core library afaict, where did you see it?
There was a problem hiding this comment.
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.
I see but it's not in CoreMetrics hm
There was a problem hiding this comment.
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.
Yeah though this could live in core I guess, I'll ticketify -- it's just Dispatch dep, not foundation hm
| @@ -18,9 +18,9 @@ import Darwin | |||
| import Glibc | |||
| #endif | |||
| import Backtrace | |||
| import CoreMetrics | |||
There was a problem hiding this comment.
is it intentional? AFAIR libraries shouldn't normally depend on CoreMetrics unless implementing a MetricFactory.
There was a problem hiding this comment.
CoreMetrics is useful when you do not want to pul in Foundation
| 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.
Consider using recordinternal(since: startTime)
I do not believe there is a standard for this, and for this reason our Metrics API is also is not opinionated about this: as you pointed out different system (Graphite, Prometheus) have their own conventions and constraints so it must be the "job" of the "backend" library to sanitize the labels to whatever its sensitive to, for example Prometheus is sensitive to dot notation, so its the "job" of the Prometheus "backend" library to convert dots into underscores or whatever. |
|
Yeah agreed on the sanitization, sounds good. LGTM |
motivation: startup/shutdown metrics are important for real-life services, for example spike in start metrics indicates a crash-loop
changes: