Pull out ...WithExemplar methods into separate interfaces #708
Conversation
beorn7
force-pushed
the
beorn7/exemplars
branch
from
3418c7b
to
5465f31
Compare
There was a problem hiding this comment.
I like this much more and I don't feel it adds much of complexity for the users. Given the proper support for exemplars will take for backends months, we will probably have already a v2 where we can change things.
I am fully on board with this replacing the incompatibility we added previously 👍 Thanks!
| // Demonstrate exemplar support with a dummy ID. This would be | ||
| // something like a trace ID in a real application. | ||
| // Note the necessary type assertion. | ||
| rpcDurationsHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar( |
There was a problem hiding this comment.
I would maybe in this example check if it's actually implementing this? So:
if e, ok := rpcDurationsHistogram.(prometheus.ExemplarObserver); ok {
e..ObserveWithExemplar( ...
}
There was a problem hiding this comment.
This is because encouraging avoiding panics in user's code is nice... But one can argue that we directly call our library constructor, so up to you here, not a blocker (:
There was a problem hiding this comment.
Yeah, it's all in the same file. I have added a comment now that explains why we don't check the type assertion.
There was a problem hiding this comment.
And I'll also fix the "tipo". :o(
This is, sadly, the only way to avoid a breaking change. The cost is that anyone using exemplars has to perform a type assertion. This is, however, a common pattern where interfaces turn out to need additional methods in a stable library or only some implementations can provide the additional methods (AKA "interface upgrade"). Needless to say that in v2 of this library, we'll do things in a more straight forward way. Signed-off-by: beorn7 <beorn@grafana.com>
beorn7
force-pushed
the
beorn7/exemplars
branch
from
5465f31
to
f34b098
Compare
I wouldn't be that optimistic. I expect v2 to take a while to reach a usable state. And even after that has happened, we'll need some time where the v2 branch can be used experimentally but will still need some time to stabilize for a real v2 release. And ever after that, I'd expect v1 to be used by a significant fraction of the current ~12k known importers for a long time… Anyway, I assume that the above won't swing your otherwise quite clear verdict here. I'm fairly confident this is the way to go, too. I'll merge this and then adjust the release PR for final approval. |
|
Awesome, yup same opinion on my side. |
This is, sadly, the only way to avoid a breaking change. The cost is
that anyone using exemplars has to perform a type assertion. This is,
however, a common pattern where interfaces turn out to need additional
methods in a stable library or only some implementations can provide
the additional methods (AKA "interface upgrade").
Needless to say that in v2 of this library, we'll do things in a more
straight forward way.
By now, I'm fairly confident we need to go down this way. It would be extremely confusing and damaging if enough users got bitten by the original approach so that we had to revert to the approach in this PR anyway.
The trade-off in this PR is the following: I'd expect relatively few users to use exemplars. And those who do are most likely advanced users who are not shocked by a type assertion. Thus, the collective cost is relatively low, but it buys as the benefit of not risking breakage.
On top of all that, a user of a
HistogramVechas to do the type assertion anyway.In different news, I'm still torn about the naming. The interface name should follow the pattern with the
...ersuffix, butObserveWithExemplareris not really an option.ExemplarObserveris not too bad, but perhaps we should rename the method toExemplarObservethen? Not quite as descriptive, but the upside is that it is shorter.WDYT?