search

swift-server / swift-prometheus

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

Remove generic argument from Histogram #103

Open fabianfett opened 12 months ago

fabianfett commented 12 months ago

In #92, we added a Histogram that is generic over the observed type. This means that each Histogram was also carrying a generic argument. To make it easier to deal with different Histogram types, we added two typealiases DurationHistogram and ValueHistogram.

The motivation behind using a generic argument was to preserve as much detailed information as possible. Duration can be more precise than Double. However in the grand scheme of things we now believe that this is overkill for a Histogram and a simpler to use Histogram type is more valuable than the lost precision. Thus this patch removes the generic argument from Histogram.

fabianfett commented 12 months ago

@ktoso I should have stressed in my pr description more that I don't love this either and mainly want to drive a discussion here.

fabianfett commented 12 months ago

@ktoso @FranzBusch @tomerd

What pain did it cause that we want to drop the generic?

Mostly requiring adopters to write generic code.

This feels like a step backwards as well tbh

I agree here as well. However, if we decide that Histogram should be generic, we should probably make Counter and Gauge generic as well. wdyt?

FranzBusch commented 12 months ago

I agree here as well. However, if we decide that Histogram should be generic, we should probably make Counter and Gauge generic as well. wdyt?

Not sure. When would we use a Duration based Counter or Gauge? Maybe that's useful. From my understanding on this PR I thought we only had the generic abstractions to make the interop with Prometheus easier but not because of swift-metrics.

fabianfett commented 12 months ago

Not sure. When would we use a Duration based Counter or Gauge? Maybe that's useful. From my understanding on this PR I thought we only had the generic abstractions to make the interop with Prometheus easier but not because of swift-metrics.

@FranzBusch For Counter and Gauge generics would be more about Int64 vs UInt64 vs Double.

fabianfett commented 12 months ago

@ktoso

Metric values in OpenMetrics MUST be either floating points or integers. Note that ingestors of the format MAY only support float64. The non-real values NaN, +Inf and -Inf MUST be supported. NaN MUST NOT be considered a missing value, but it MAY be used to signal a division by zero.

https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#values

ktoso commented 12 months ago

Yeah though that's about storage / emitting isn't it? so, maybe meet in the happy middle and keep the Duration histogram API but store in terms of Doubles if that's what you're after to simplify the implementation @fabianfett ?

Duration effectively "is" an integer.

I'm confused why the pushback to the language's default way of expressing duration. It'll be a pain to accept measurements from things which WILL express them as Duration.

What's the gain we're seeking by dropping support for Duration?