Skip to content
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

Should Gauge gain add in addition to record? #36

Closed
ktoso opened this issue Jul 8, 2019 · 4 comments
Closed

Should Gauge gain add in addition to record? #36

ktoso opened this issue Jul 8, 2019 · 4 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@ktoso
Copy link
Member

ktoso commented Jul 8, 2019

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:

@ktoso ktoso added enhancement New feature or request semver/major Breaks existing public API. labels Jul 8, 2019
@ktoso ktoso changed the title Should Gauge gain add in addition to record Should Gauge gain add in addition to record? Jul 8, 2019
@ktoso ktoso added question Further information is requested and removed semver/major Breaks existing public API. labels Jul 9, 2019
@tomerd
Copy link
Member

tomerd commented Aug 23, 2019

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

@ktoso
Copy link
Member Author

ktoso commented Aug 24, 2019

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
Copy link
Member Author

ktoso commented Aug 26, 2019

Looking into it 💭

@ktoso
Copy link
Member Author

ktoso commented Aug 26, 2019

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.

@ktoso ktoso closed this as completed Aug 26, 2019
@ktoso ktoso added wontfix This will not be worked on and removed enhancement New feature or request labels Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants