Add gauge registry#503
Conversation
opencensus/metrics/registry.py
Outdated
| from opencensus.metrics.export import metric_producer | ||
|
|
||
|
|
||
| class Registry(metric_producer.MetricProducer): |
There was a problem hiding this comment.
Do we want to name this class as opencensus.metrics.GaugeRegistry or opencensus.metrics.gauge.Registry? opencensus.metrics.Registry is a very generic name.
There was a problem hiding this comment.
That makes sense, gauge.Registry works for me.
opencensus/metrics/registry.py
Outdated
| """ | ||
| now = datetime.now() | ||
| metrics = set() | ||
| with self._gauges_lock: |
There was a problem hiding this comment.
Depending on whether it is CPython or something else, we might not need the threading.Lock while getting values().
There was a problem hiding this comment.
What about if another thread changes a value while we're iterating here? E.g.
import time
from concurrent.futures import ThreadPoolExecutor
tpe1 = ThreadPoolExecutor(max_workers=1)
tpe2 = ThreadPoolExecutor(max_workers=1)
dd = dict(zip(range(10), range(10)))
def read_values():
global dd
for vv in dd.values():
print(vv, end=", ")
time.sleep(.01)
print()
def mess_with_values():
global dd
for kk in dd:
dd[kk] = -dd[kk]
time.sleep(.01)
tpe1.submit(read_values)
tpe2.submit(mess_with_values)0, 1, 2, 3, 4, -5, -6, -7, -8, -9,
There was a problem hiding this comment.
I think there are better ways to do this, all the locking in gauges so far has been pretty naive. One obvious improvement is to use a read/write lock to let more than one thread read at once. I didn't spend time optimizing this yet because I don't think we're likely to have a bottleneck here, but I did want to make sure the class was threadsafe.
There was a problem hiding this comment.
I thought we are only adding new gauges (add_gauge) instead of altering the existing gauges?
And after we take a snapshot of the registered gauges, we get the metrics based on a pre-calculated value now.
There was a problem hiding this comment.
I thought we'd want to remove gauges too, but after talking to @bogdandrutu it turns out that this is by design.
Looking more closely at dictview I think you're right that we don't need a lock here.
opencensus/metrics/registry.py
Outdated
| :return: A set of `Metric`s, one for each registered gauge. | ||
| """ | ||
| now = datetime.now() | ||
| metrics = set() |
There was a problem hiding this comment.
Is there a preference over set than tuple?
There was a problem hiding this comment.
No preference, just to emphasize that it's unordered. The java client returns an ArrayList but the order isn't fixed.
This PR adds a registry class for gauges. The class exists so we can wrap up a set of
Gauges inside a metric producer. Callingregistry.get_metrics()returns the set of metrics we get by callinggauge.get_metrics(ts)on each registered gauge for the current timestampts.Registryroughly follows theMetricRegistryimplementation in the java client, but exposes a singleadd_gaugemethod instead of separate methods for each gauge type.The advantages of this change are:
Gaugeconstructors inRegistrymethods that create gaugesAnd the disadvantages are:
This PR follows #498.