Remove generic argument from Histogram #103
Conversation
In swift-server#92, we added a Histogram that is generic over the observed type. This means that each Histogram was also carrying a generic argument. To work around this we created typealiases for `DurationHistogram` and `ValueHistogram`. The motivation behind this 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 and a simpler to use Histogram type is more valuable. Thus this patch removes the generic argument from Histogram.
There was a problem hiding this comment.
I don't think this is the right direction.
I know doubles are "good enough" for time but but the API exposed as doubles always is really ugly.
We also now have an observe(Duration) on ANY histogram, even if it is actually e.g. sizes of things, which is rather ugly. The split was useful IMHO and I'm not sure why we should pull back on it - the PR doesn't really provide sufficient argument tbh.
If we just want to simplify implementation I'm sure we could just implement an underlying type over Double and expose two APIs for Double and specialized for Duration if that's the primary reason driving this change
| @@ -67,20 +55,29 @@ public final class Histogram<Value: Bucketable>: Sendable { | |||
| state.count += 1 | |||
| } | |||
| } | |||
|
|
|||
| public func observe(_ value: Duration) { | |||
There was a problem hiding this comment.
So we could record durations into a histogram intended for values; this seems silly for our ecosystem in which we value type safety for such stuff.
| .milliseconds(50), | ||
| .milliseconds(100), | ||
| factory.defaultTimerHistogramBuckets = [ | ||
| 0.005, 0.01, 0.025, 0.05, 0.1 | ||
| ] |
There was a problem hiding this comment.
This feels like a step backwards as well tbh
There was a problem hiding this comment.
I agree that this feels like a step backwards
|
@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. |
| .milliseconds(50), | ||
| .milliseconds(100), | ||
| factory.defaultTimerHistogramBuckets = [ | ||
| 0.005, 0.01, 0.025, 0.05, 0.1 | ||
| ] |
There was a problem hiding this comment.
I agree that this feels like a step backwards
Mostly requiring adopters to write generic code.
I agree here as well. However, if we decide that Histogram should be generic, we should probably make |
Not sure. When would we use a |
@FranzBusch For |
https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#values |
|
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? |
In #92, we added a
Histogramthat is generic over the observed type. This means that eachHistogramwas also carrying a generic argument. To make it easier to deal with differentHistogramtypes, we added two typealiasesDurationHistogramandValueHistogram.The motivation behind using a generic argument was to preserve as much detailed information as possible.
Durationcan be more precise thanDouble. However in the grand scheme of things we now believe that this is overkill for aHistogramand a simpler to useHistogramtype is more valuable than the lost precision. Thus this patch removes the generic argument fromHistogram.