swift-metrics: Timer does not maintain units

Hey, yeah when we say “backend” in this thread it usually refers to “metrics library that is specific to some backend but is not the metrics-api but an implementation there of” if that makes sense. So this project does not actually do anything, it is like slf4j, in the sense that it is only the API. And then there’s metrics library implementations “backends” which actually emit the metrics; be it to Prometheus or statsd or similar.

One thing I’m not sure about is if this additional constructor will also have to exist on the Timer type… sorry I could not investigate this one more yet.

Sorry it took me a while to get around here;

I agree that there may be two “types” of users and/or backends, and while I’d argue the “let backend report however it wants” makes the “more sense” I guess those views are a bit subjective, and various backends have their own styles. While always reporting in most fine grained unit available => no information loss, while reporting in seconds => information loss; it also means more noise in the numbers and perhaps post aggregation etc.

It does seem indeed to be a recommended pattern in prometheus for the metric names to be aware about the unit they report, i.e.:

http_request_duration_seconds

So one would want to report those in seconds as @Yasumoto mentions.

Having that said, @ddossot’s idea seems quite solid, so we can indeed:

So what we could do is that we add a new method to the protocol but provide a default implementation which just removes the units and calls into the old one. Apart from that shim, we’d never call into the old (unit-less) implementation anymore and document it appropriately.

So this perhaps can work, as Tom suggests?

public protocol TimerHandler: AnyObject {
    init (storageUnit: TimeUnit)
    func recordInStorageUnit(_ duration: Int64, _ recordedInTimeUnit: TimeUnit)

public class Timer {
    @inlinable
    public func recordNanoseconds(_ duration: Int64) {
        self.handler.recordInStorageUnit(duration, .nanoseconds)
    }
    public func recordSeconds<DataType: BinaryInteger>(_ duration: DataType) {
        // ... 
        self.handler.recordInStorageUnit(result, .seconds)
...
}

Though it complicates the code paths a little bit… Would you want to give it a shot at PRing this change @Yasumoto ?

Because I’d like to publish SwiftPrometheus 1.0 sometime soon, I went ahead and created the PR for this so I can solve this issue before my 1.0 release 😄

Awesome, thanks for digging into this, folks! If I can take a few days I’d be happy to give this a whirl. I’ll follow the thread on Timer(storageUnit: TimeUnit) and report back!

the tradeoff is that some backend will need to silently ignore this since they can’t support display options, leading to unexpected user experience

That’s fair and sounds okey. Like the statsd one would ignore and always emit in ms. Documentation would say it does not support that as well then I suppose.

=> also not sure i follow why we would need to introduce recordInStorageUnit instead of keeping the recording API recordNanoseconds in place and translating to the display unit on “collect” (to use prometheus terminology). what am i missing?

I see, I slightly overdid the proposal there then; You’re right, silently storing and always delivering to the backend in nanos, but at least the backend was told what “unit it is”, so it can render as seconds if it so wanted to (which prometheus libs seem to like to do).

So: just the added constructor, and the “front” always delivers to backend as ns and the backend can do whatever it wants. Sounds good to me 👍

another way, and maybe what @ddossot is alluding to in we had to have an optional construction argument on the concrete implementation to allow specifying the conversion unit if any. is that the metrics backend library offers a way to configure this at the concrete timer constructor, eg let timer = FooTimer(unit: .seconds). another, similar way could be for the backend library to offer configuration options for metrics objects by their label, eg "timer.foo.unit": .seconds (or similar) which you specify when initializing the concrete backend. the config based approach is similar to how many configuration based logging systems work, see an example in https://github.com/apple/swift-log/blob/master/Tests/LoggingTests/TestLogger.swift#L18

@ktoso wdyt?

We’ve observed these combinations in generic metrics API:

1. back-end requires times to be reported in a fixed unit
2. back-end is unit-less 2.1. users want all times to be unified to a single unit 2.2. users know the units of the different KPIs and interpret them when looking at collected data

Case 1 is easy as the concrete implementation for the back-end knows its unit of choice. For case 2, we had to have an optional construction argument on the concrete implementation to allow specifying the conversion unit if any.