Refactoring and metric wrapper unification (mark 2)#344
Conversation
brian-brazil
left a comment
brian-brazil
left a comment
There was a problem hiding this comment.
I'm still not seeing why you need the complex class hierarchy, we only need to support labels() once.
|
Was about to start a discussion on the topic, then I fount this PR. Absolutely love the initiative and hope this gets merged quickly. Ease of use is already amazing, but lack of type hints due to decorator functions magic is a nightmare. |
The more complex class hierarchy makes it so that the child metrics don't actually even have a I can simplify that and raise a runtime error within |
That sounds good. |
Signed-off-by: Aarni Koskela <akx@iki.fi>
|
Okay, review comments addressed and the re-refactor pushed as new commits. I can squash/rebase as needed, but figured this might make reviewing easier. |
akx
force-pushed
the
refac3
branch
2 times, most recently
from
e87bfec to
f8b78e8
Compare
|
That looks okay. Can you squash sanely? |
The LabelWrapper class is also no more. For metric classes where you have set `labelnames`, you can call `.labels()` as before, to acquire a child metric object. The internal API for child-level metrics did change slightly with this change; if you have a custom metric of some sort, be aware that instead of `_samples`, you should override `_child_samples`. Fixes prometheus#210 Signed-off-by: Aarni Koskela <akx@iki.fi>
|
@brian-brazil Done deal. Squashed the last 3 commits. |
|
Thanks! |
3 participants
This is a rebase of #313 (not my favorite rebase btw; that's why the N refactoring commits in #313 become one big one here) – the comments below are mostly the same.
🔧 This PR refactors the 1200+-line
core.pyinto separate modules for each functionality. This does not affect imports for client code; all the sameprometheus_client-provided non-underscored names are re-exported from that file. Some previously underscored names were brought into the light.✨ Secondly (and this is what prompted me to really do this work), it fixes #210 by removing the factory function magic decorator
MetricWrapper; as it was before, say, an invocationGauge(...)could either return aGaugeobject, as you'd expect, or a_LabelWrapperif you'd passed in a list of label names. This tripped up code completion (and tbqh, me :) ) and static analysis.Instead, the six metric wrappers (Counter, Enum, Gauge, Histogram, Info, Summary) are now plain old classes (though with a slightly arcane diamond-shaped inheritance hierarchy that lets them be used both with and without
.labels()).👓 No significant test code was changed for this (aside from the removal of the
__wrapped__attribute as there is nothing to wrap anymore), so I would say this is backwards API compatible (with the exception of client code that might have done checks likeisinstance(x, _LabelWrapper)).I'm glad to answer any questions and/or modify things as needed! :)