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

Awkward collection API #118

Open
ptoffy opened this issue Dec 7, 2024 · 2 comments
Open

Awkward collection API #118

ptoffy opened this issue Dec 7, 2024 · 2 comments

Comments

@ptoffy
Copy link

ptoffy commented Dec 7, 2024

I've switched from v1 to v2 and this was the switch I've had to make to collect metrics

- return try await MetricsSystem.prometheus().collect()
+ var buffer = [UInt8]()
+ (MetricsSystem.factory as? PrometheusMetricsFactory)?.registry.emit(into: &buffer)
+ return String(bytes: buffer, encoding: .utf8) ?? ""

which seems quite the downgrade in terms of usability.
It would be cool if the library provided some helpers to collect data.
I created this

extension MetricsSystem {
    static var prometheus: PrometheusMetricsFactory {
        get throws {
            guard let factory = Self.factory as? PrometheusMetricsFactory else {
                throw Abort(.internalServerError, reason: "Metrics factory is not Prometheus")
            }
            return factory
        }
    }
}

extension PrometheusMetricsFactory {
    func emit() -> String {
        var buffer = [UInt8]()
        self.registry.emit(into: &buffer)
        return String(bytes: buffer, encoding: .utf8) ?? ""
    }
}

to account for it and the result is quite a bit simpler

try MetricsSystem.prometheus.emit()

so I was wondering if a PR would be accepted with this or a similar addition, maybe a [UInt8] version too if needed, especially as the use case is often likely to be an endpoint which we just want to return the data too and we probably won't need a buffer. I'm happy to be proven wrong in case there's a simpler way already or a reason for it to just be like this!

@mman
Copy link

mman commented Dec 10, 2024

@ptoffy

I have been bitten by this as well, but if you look into the implementation of registry.emit, it in the end the will call emit on all the the metrics, and they will all just append to a buffer. So when the buffer is small, it will cause frequent reallocations. If you do this every 15 seconds, and emitting a lot of metrics, you are essentially causing lot of unnecessary reallocations which may or may not affect your server app.

So I'd assume given the knowledge of how much metrics you expect to emit, you can pre-allocate the buffer once and use that on every emit call.

I'm not entirely sure if my understanding is correct, but if it is, I'd love to open a PR against the README and API docs to clarify this... as it looks like a FAQ

@fabianfett
Copy link
Member

So I'd assume given the knowledge of how much metrics you expect to emit, you can pre-allocate the buffer once and use that on every emit call.

That is 100% the intention here! We should better document this however!

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