Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Add gauge registry#503

Merged
c24t merged 4 commits intocensus-instrumentation:masterfrom
c24t:gauge-registry
Feb 19, 2019
Merged

Add gauge registry#503
c24t merged 4 commits intocensus-instrumentation:masterfrom
c24t:gauge-registry

Conversation

@c24t
Copy link
Copy Markdown
Member

@c24t c24t commented Feb 13, 2019

This PR adds a registry class for gauges. The class exists so we can wrap up a set of Gauges inside a metric producer. Calling registry.get_metrics() returns the set of metrics we get by calling gauge.get_metrics(ts) on each registered gauge for the current timestamp ts.

Registry roughly follows the MetricRegistry implementation in the java client, but exposes a single add_gauge method instead of separate methods for each gauge type.

The advantages of this change are:

  • it gives us a simpler API, less code
  • avoids duplicating the arguments to Gauge constructors in Registry methods that create gauges
  • makes gauge construction explicit

And the disadvantages are:

  • it makes the library more verbose, users now need to construct gauges themselves
  • users may forget to register gauges

This PR follows #498.

from opencensus.metrics.export import metric_producer


class Registry(metric_producer.MetricProducer):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to name this class as opencensus.metrics.GaugeRegistry or opencensus.metrics.gauge.Registry? opencensus.metrics.Registry is a very generic name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, gauge.Registry works for me.

"""
now = datetime.now()
metrics = set()
with self._gauges_lock:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on whether it is CPython or something else, we might not need the threading.Lock while getting values().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:return: A set of `Metric`s, one for each registered gauge.
"""
now = datetime.now()
metrics = set()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a preference over set than tuple?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference, just to emphasize that it's unordered. The java client returns an ArrayList but the order isn't fixed.

@c24t c24t requested a review from a team as a code owner February 19, 2019 20:24
@c24t c24t merged commit 2d4c863 into census-instrumentation:master Feb 19, 2019
@c24t c24t deleted the gauge-registry branch February 19, 2019 20:59
@c24t c24t mentioned this pull request Apr 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants