Metrics: add MetricsProducer and convert stats#476
Metrics: add MetricsProducer and convert stats#476c24t merged 8 commits intocensus-instrumentation:masterfrom
Conversation
|
|
||
|
|
||
| class Stats(object): | ||
| class Stats(MetricProducer): |
There was a problem hiding this comment.
Stats here is effectively both the java library's StatsManager and MetricProducerImpl classes.
MetricProducer doesn't provide anything here other than to signify that Stats is in fact a metrics producer. It's not clear to me that this is the best approach. Our other options are (1) duck typing: remove MetricProducer and rely on get_metrics to signify that this is a metrics producer, and (2) composition: add a separate StatsMetricProducer that wraps Stats.
See PEP 3119 for a justification for using abstract base classes like these in python.
| self._view_manager = ViewManager() | ||
|
|
||
| @property | ||
| def stats_recorder(self): |
There was a problem hiding this comment.
I'd understand making an attribute protected and then exposing it as a property if we were returning a copy, but we don't seem to be doing that here.
Point conversions previously failed for DistributionAggregationDatas with histograms.
c24t
force-pushed
the
metrics-wrap-stats
branch
from
9ed23b5 to
4e3b562
Compare
|
|
||
| from datetime import datetime | ||
|
|
||
| from opencensus.metrics.export.metric_producer import MetricProducer |
There was a problem hiding this comment.
I kept this consistent with other imports, but we should decide on class- or module-level imports across the library.
| with self.mp_lock: | ||
| self.metric_producers.remove(metric_producer) | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
getAllMetricProducer is missing?
There was a problem hiding this comment.
Because the set of MPs is available as manager.metric_producers.
That said, you might want a method that gives you a copy of the set so another thread can't add/remove a MP as you're iterating through it. I don't know how much consideration to give threadsafety since most stats classes ignore it completely.
There was a problem hiding this comment.
Added in fdd002f, if we go to the trouble of adding a lock to the class we might as well use it here.
| :type metric_producer: :class: 'MetricProducer' | ||
| :param metric_producer: The metric producer to remove. | ||
| """ | ||
| if metric_producer is None: |
There was a problem hiding this comment.
I am not sure If we want to do None check here...
There was a problem hiding this comment.
There's a null check in the java client here (https://github.com/c24t/opencensus-java/blob/e8cba95228e9f104b325a99b3499d0da4be84e8d/api/src/main/java/io/opencensus/metrics/export/MetricProducerManager.java#L80), but I'm happy either way.
3 participants
This PR adds a
MetricProducerclass and adds the plumbing to allowStatsto produce metrics, building on #469. It also fixes a bug convertingDistributionAggregationDatas without bucket bounds into metricsPoints.Closes #335.