Add derived gauges#498
Conversation
Slightly more correct, but shouldn't make a difference in practice since WeakMethod should only be used in python 2, where this is equivalent to raise.
…to derived-gauges
|
@reyang I'd be interested to hear your feedback on this PR too, in particular your thoughts on the |
opencensus/common/utils.py
Outdated
| """Get a weak reference to bound or unbound `func`. | ||
|
|
||
| If `func` is unbound (i.e. has no __self__ attr) get a weakref.ref, | ||
| otherwise get a wrapper that simulates simulates weakref.ref. |
There was a problem hiding this comment.
minor: remove duplicated simulates
| return ts_list | ||
|
|
||
|
|
||
| class GaugePoint(object): |
There was a problem hiding this comment.
We could probably put the __repr__ implementation in the base class.
There was a problem hiding this comment.
Probably extract the lock initialization in to GaugePoint.__init__ as well?
class GaugePoint(object):
def __init__(self):
self._value_lock = threading.Lock()
There was a problem hiding this comment.
I didn't do this because I wanted DerivedGaugePoint to be a GaugePoint too, and this would mean that we couldn't call its parent's constructor (or create an unused lock if we did).
I think there's good reason to have abstract classes like these where all the methods are unimplemented, but you can argue that classes should either be abstract or concrete -- and not like Gauge and BaseGauge that implement some methods and not others. What do you think?
There was a problem hiding this comment.
I see, makes sense.
I don't have strong opinion here, both approaches have their valid reasons and I don't see a clear winner. Go with your choice :)
This PR follows #494 and adds derived gauge classes
DerivedLongGauge,DerivedDoubleGauge, andDerivedGaugePointsimilar to the corresponding classes in the java client. It also changesTimeSeriesto allow null-valued points to deal with derived gauges tracking functions that have been garbage collected.