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

Allow additional dimensions when recording a value #38

Closed
Yasumoto opened this issue Jul 16, 2019 · 8 comments
Closed

Allow additional dimensions when recording a value #38

Yasumoto opened this issue Jul 16, 2019 · 8 comments

Comments

@Yasumoto
Copy link
Contributor

Current behavior

I create a Counter to track the total number of requests in my application:

private let requestsCounter = Metrics.Counter(label: "http_requests_total", dimensions: [(String, String)]())

Desired Behavior

I'd like to use that in my router to increase the metric on "per-path, per-method, and per-response-code" dimensions:

requestsCounter.dimensions = [
        ("method", request.http.method.string),
        ("path", request.http.url.path),
        ("http_code", "200")
]

Note that this is heavily biased toward using 🔥 Prometheus with its concept of labels which map to our dimensions. In other systems (such as graphite) I'd use different label names.

Considered Approaches

I could create a separate Counter for each path, but then I'd need to create a boldly exploding multi-dimensional matrix of Counters for each dimension.

However, what if we updated the actual "write a value" methods like Counter.increment() and Timer.record() to also take in more dimensions? We'd need to update the way we handle DimensionLabels in SwiftPrometheus but I think that'd allow us to leverage labels from swift-metrics.

@ktoso
Copy link
Member

ktoso commented Jul 16, 2019

Counters are (relatively) cheap. Why would you avoid creating many of them? Especially for paths, which usually are relatively static?

If you want to get this API to get "look and feel parity with prometheus without using prometheus" you can get there without changing all of the metrics APIs -- by providing an extension func boringCounter.withLabels(...).increment() which internally would create another counter based on the previous one; this does not have to live in this package, as it can be easily added on top as extensions.

But having that said... Some APIs work like that, some don't. With this SPI we aim to do the common ground of all APIs such that we backends do not need to go do great lengths to adopt these types -- the more surface we add, the more complex it becomes to adopt.

Note also, that the prime use of this SPI package is to offer metrics without knowing what backend one has -- e.g. in other libraries. If you are an end user application (webapp or something else) and really really need to use specific backend features, you are free to do so. Not all those patterns of features have to be exposed on the "common currency" types which this package represents.


Given an easy way to implement what you ask for exists, maybe you could start with that and we keep the ticket to see if it becomes a strongly demanded pattern across many backends and consider it then? We think this package needs to be very careful when adopting API changes, as it is the "common ground" API for many backends.

@Yasumoto
Copy link
Contributor Author

Yep, thanks for talking through the additional context 🙇‍♂️

I think we're running into the "generic solutions get us some of the way there" but in this case it'd lead to a non-idiomatic solution for a particular backend (Prometheus) which isn't ideal.

I wonder if there's something along the lines of only exporting high-level metrics for core libraries. But if an application bootstraps a specific backend, we can rely on the additional features (like labels) to provide tighter integration with various backends instead in some way. I'll keep pondering, and of course more ideas appreciated!

@t089
Copy link

t089 commented Jul 16, 2019

Not sure if this is clear, but you can easily use dimensions as labels today. What you are asking for it just a slightly shuffled around syntax, but in the end it really boils down to the same thing. A web framework might emit request count metrics like this eg:

public let metrics: Middleware = { req, res, ctx in
    res.whenComplete { r in
        switch r {
        case .success:
            Counter(label: "http_requests_total", dimensions: [
                ( "route", req.header.path ),
                ("method", req.header.method.rawValue),
                ("status", "\(res.status.code)") ]).increment()
        case .failure:
            Counter(label: "http_requests_total", dimensions: [
                ( "route", req.header.path ),
                ("method", req.header.method.rawValue),
                ("status", "error") ]).increment()
        }
    }
    
    ctx.next()
}

Isn’t this exactly what you want? Note, that it is not your job as an adopter of swift-metrics to worry about the lifecycle of the counter. The implementation has to worry about this. You just create one out of thin air when you need it.

@Yasumoto
Copy link
Contributor Author

You just create one out of thin air when you need it.

@t089 that might be where my mental block was. Thank you!! I'll follow that path then report back 🎉

@ktoso
Copy link
Member

ktoso commented Jul 16, 2019

Great example there @t089, thanks for chiming in 👍

There's more ways one could get to the "code shape" you're after, but they all will end up as something like the above.

Please report back if there's more discussion to be had here or if we can close the ticket once you've confirmed on your end if the solution works for you @Yasumoto :)

@Yasumoto
Copy link
Contributor Author

Yep, this was exactly what I was looking for, thank y'all so much for the help!

I'm wondering if there's value in expanding out the documentation (maybe a "usage guide" ?) to help point folks in this direction, or maybe this existed and I just missed it. If not, happy to have this closed!

@Yasumoto
Copy link
Contributor Author

Here's where I wound up in case it helps anyone in the future: vapor-community/VaporMonitoring#5

@Yasumoto
Copy link
Contributor Author

Closing since this is rad. Thanks again, folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants