swift-server / swift-prometheus

Prometheus client library for Swift
https://swiftpackageindex.com/swift-server/swift-prometheus
Apache License 2.0
145 stars 31 forks source link

Histogram backed timer #46

Closed avolokhov closed 3 years ago

avolokhov commented 3 years ago

Hey @MrLotU,

Thanks for considering this PR. In my project I'm running multiple replicas of my backend service. For a proper monitoring I needed aggregatable timer metrics to get cumulative percentiles across the whole fleet. Here's the solution I came up with. Since the project is in alpha, I decided to start with breaking a few APIs in order to keep the code simple. Please let me know if you think we should approach this differently.

Fixes https://github.com/MrLotU/SwiftPrometheus/issues/44

Checklist

Motivation and Context

Server side applications are never run in as single instance. Summary backed timer doesn't allow aggregation and therefore doesn't allow to render accumulated latency percentiles across multiple instances. Histogram backed timer provides a solution with it. Timer implementation is yet another fact that only makes sense for bridging Prometheus to swift-metrics (in addition to Labels that are init parameters in swift-metrics world and call parameters in Prometheus world, and therefore LabelSanitizer). It feels natural to separate non-trivial swift-metrics bridge from Prometheus core, keeping either of them simpler and more testable.

Description

grennis commented 3 years ago

+1 for this.

I tried the branch and it seemed to work great. The only problem I saw was that in Vapor using:

        try MetricsSystem.prometheus().collect(into: promise)

gave me Fatal error: leaking promise. But I worked around by using the client I had created:

        client.collect(into: promise)

And this worked.

In any case would love to see this get merged, as providing a histogram backed timer is really a necessity.

MrLotU commented 3 years ago

+1 for this.

I tried the branch and it seemed to work great. The only problem I saw was that in Vapor using:

        try MetricsSystem.prometheus().collect(into: promise)

gave me Fatal error: leaking promise. But I worked around by using the client I had created:

        client.collect(into: promise)

And this worked.

In any case would love to see this get merged, as providing a histogram backed timer is really a necessity.

@grennis The issue you mention about leaking promises seems unrelated to this PR, but should not be happening for sure. Would you mind filing a bug-report with some extra information like system setup, full error message and reproduction steps? You can do so here

Thanks!

avolokhov commented 3 years ago

@MrLotU thanks for the review! I believe, I've addressed all the comments.

MrLotU commented 3 years ago

Released in 1.0.0-alpha.10