apple/swift-metrics

Should Gauge gain `add` in addition to `record`?

ktoso opened this issue · 4 comments

ktoso commented

Expected behavior

I'd like to use gauges to easily implement a metric of how many things I have alive in my system -- examples could be threads or connections.

// ==== ----------------------------------------------------------------------------------------------------------------
// goal: easily implement gauges which measure "amount of alive things"

class Heavy {

    init() {
        // this is hard to express when we only have "record":
        let gauge = Gauge(label: "heavy-1")
        gauge.add(1) 
    }

    deinit {
        // this is hard to express when we only have "record":
        gauge.add(-1) 
    }

    // or start() / stop(), same story

}

Actual behavior

Can't implement this like that since we do not offer add, we only offer record, so I have to implement my own thing which keep the total number and then use that to record into the actual metrics gauge:

Implementation idea 1, which fails since Gauge is not open:

// ==== ----------------------------------------------------------------------------------------------------------------
// wishful thinking workaround impl, though heavy...

///
/// error: cannot inherit from non-open class 'Gauge' outside of its defining module
/// internal class AddGauge: Gauge {
///                ^
@usableFromInline
internal class AddGauge: Gauge {

    @usableFromInline
    internal let _storage: Atomic<Int>
    internal let underlying: Gauge

    public convenience init(label: String, dimensions: [(String, String)] = []) {
        self.underlying = Gauge(label: label, dimensions: dimensions)
    }

    override func record<DataType: BinaryFloatingPoint>(_ value: DataType) {
        self.underlying.record(value)
    }

    @inlinable
    func add<DataType: BinaryFloatingPoint>(_ value: DataType) {
        let value = self._storage.add(value)
        self.underlying.record(value)
    }
}

So what I end up is something like:

// ==== ----------------------------------------------------------------------------------------------------------------
// current workaround

@usableFromInline
internal class AddGauge {

    @usableFromInline
    internal let _storage: Atomic<Int>
    @usableFromInline
    internal let underlying: Gauge

    public init(label: String, dimensions: [(String, String)] = []) {
        self._storage = .init(value: 0)
        self.underlying = Gauge(label: label, dimensions: dimensions)
    }

    @inlinable
    func record(_ value: Int) {
        self.underlying.record(value)
    }

    @inlinable
    func add<DataType: BinaryFloatingPoint>(_ value: DataType) {
        let v = Int(value)
        self.underlying.record(self._storage.add_load(v))
    }
}

Main question:

  • Should we add add() to Gauge as it enables this popular use case?
    • if yes, this needs a major bump, better now than later I guess though
    • if no, what's the pattern people should adopt for this use case?
  • alternatively making the types open would allow for implementing such things nicer...
    • that's likely a BAD idea though, so let's not
  • we could tell people to implement a special weird handler, and add an extension to Counter.add which has to pattern match on the handler if it supports an add operation or not... it seems a bit weird though and would come at a cost.

Other points:

tomerd commented

@ktoso if you feel strongly about this, it would be good time since we are working on 1.2.0

ktoso commented

Thanks for reminder! I’ll give it a look on Monday if my workaround is a good general approach or it built in api would be much better :)

ktoso commented

Looking into it 💭

ktoso commented

Spent some time with this expressing some metrics I wanted to capture with "add" and thanks to some great discussion with @yim-lee was able to figure out we can express everything we need by using a positive + negative counter pair;

This is actually quite good, as one can track all those events separately and also get their independent progress as well as post process them nicely (sum(positive, -negative) == current count) in aggregation systems like graphite, prometheus etc.

For curious people who'd bump into the same issue:

@usableFromInline
internal struct MetricsPNCounter {
    @usableFromInline
    let positive: Counter
    @usableFromInline
    let negative: Counter

    init(label: String, positive positiveDimensions: [(String, String)] = [], negative negativeDimensions: [(String, String)] = []) {
        self.positive = Counter(label: label, dimensions: positiveDimensions)
        self.negative = Counter(label: label, dimensions: negativeDimensions)
    }

    @inlinable
    func increment(by value: Int64 = 1) {
        assert(value > 0, "value MUST BE > 0")
        self.positive.increment(by: value)
    }

    @inlinable
    func decrement(by value: Int64 = 1) {
        assert(value > 0, "value MUST BE > 0")
        self.negative.increment(by: value)
    }
}

So in prometheus one could see those as

resource{label="bananas", event="start"} 16
resource{label="bananas", event="stop"} 10

meaning that right now we have 6 active bananas; this is to be calculated in post processing though.