-
Notifications
You must be signed in to change notification settings - Fork 247
Metrics: add MetricsProducer and convert stats #476
Changes from all commits
b6f3b5c
5a22b26
4a598ab
4e3b562
e603cfd
02d8a3e
66bcd14
fdd002f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # Copyright 2019, OpenCensus Authors | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import threading | ||
|
|
||
|
|
||
| class MetricProducer(object): | ||
| """Produces a set of metrics for export.""" | ||
|
|
||
| def get_metrics(self): | ||
| """Get a set of metrics to be exported. | ||
|
|
||
| :rtype: set(:class: `opencensus.metrics.export.metric.Metric`) | ||
| :return: A set of metrics to be exported. | ||
| """ | ||
| raise NotImplementedError # pragma: NO COVER | ||
|
|
||
|
|
||
| class MetricProducerManager(object): | ||
| """Container class for MetricProducers to be used by exporters. | ||
|
|
||
| :type metric_producers: iterable(class: 'MetricProducer') | ||
| :param metric_producers: Optional initial metric producers. | ||
| """ | ||
|
|
||
| def __init__(self, metric_producers=None): | ||
| if metric_producers is None: | ||
| self.metric_producers = set() | ||
| else: | ||
| self.metric_producers = set(metric_producers) | ||
| self.mp_lock = threading.Lock() | ||
|
|
||
| def add(self, metric_producer): | ||
| """Add a metric producer. | ||
|
|
||
| :type metric_producer: :class: 'MetricProducer' | ||
| :param metric_producer: The metric producer to add. | ||
| """ | ||
| if metric_producer is None: | ||
| raise ValueError | ||
| with self.mp_lock: | ||
| self.metric_producers.add(metric_producer) | ||
|
|
||
| def remove(self, metric_producer): | ||
| """Remove a metric producer. | ||
|
|
||
| :type metric_producer: :class: 'MetricProducer' | ||
| :param metric_producer: The metric producer to remove. | ||
| """ | ||
| if metric_producer is None: | ||
| raise ValueError | ||
| try: | ||
| with self.mp_lock: | ||
| self.metric_producers.remove(metric_producer) | ||
| except KeyError: | ||
| pass | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the set of MPs is available as 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, ok.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in fdd002f, if we go to the trouble of adding a lock to the class we might as well use it here. |
||
|
|
||
| def get_all(self): | ||
| """Get the set of all metric producers. | ||
|
|
||
| Get a copy of `metric_producers`. Prefer this method to using the | ||
| attribute directly to avoid other threads adding/removing producers | ||
| while you're reading it. | ||
|
|
||
| :rtype: set(:class: `MetricProducer`) | ||
| :return: A set of all metric producers at the time of the call. | ||
| """ | ||
| with self.mp_lock: | ||
| mps_copy = set(self.metric_producers) | ||
| return mps_copy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,24 +12,29 @@ | |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from datetime import datetime | ||
|
|
||
| from opencensus.metrics.export.metric_producer import MetricProducer | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept this consistent with other imports, but we should decide on class- or module-level imports across the library.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| from opencensus.stats.stats_recorder import StatsRecorder | ||
| from opencensus.stats.view_manager import ViewManager | ||
|
|
||
|
|
||
| class Stats(object): | ||
| class Stats(MetricProducer): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See PEP 3119 for a justification for using abstract base classes like these in python. |
||
| """Stats defines a View Manager and a Stats Recorder in order for the | ||
| collection of Stats | ||
| """ | ||
|
|
||
| def __init__(self): | ||
| self._stats_recorder = StatsRecorder() | ||
| self._view_manager = ViewManager() | ||
|
|
||
| @property | ||
| def stats_recorder(self): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| """the current stats recorder for Stats""" | ||
| return self._stats_recorder | ||
|
|
||
| @property | ||
| def view_manager(self): | ||
| """the current view manager for Stats""" | ||
| return self._view_manager | ||
| self.stats_recorder = StatsRecorder() | ||
| self.view_manager = ViewManager() | ||
|
|
||
| def get_metrics(self): | ||
| """Get a Metric for each of the view manager's registered views. | ||
|
|
||
| Convert each registered view's associated `ViewData` into a `Metric` to | ||
| be exported, using the current time for metric conversions. | ||
|
|
||
| :rtype: Iterator[:class: `opencensus.metrics.export.metric.Metric`] | ||
| """ | ||
| return self.view_manager.measure_to_view_map.get_metrics( | ||
| datetime.now()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| # Copyright 2019, OpenCensus Authors | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| try: | ||
| from mock import Mock | ||
| except ImportError: | ||
| from unittest.mock import Mock | ||
|
|
||
| import unittest | ||
|
|
||
| from opencensus.metrics.export import metric_producer | ||
|
|
||
|
|
||
| class TestMetricProducerManager(unittest.TestCase): | ||
| def test_init(self): | ||
| mpm1 = metric_producer.MetricProducerManager() | ||
| self.assertEqual(mpm1.metric_producers, set()) | ||
|
|
||
| mock_mp = Mock() | ||
| mpm2 = metric_producer.MetricProducerManager([mock_mp]) | ||
| self.assertEqual(mpm2.metric_producers, set([mock_mp])) | ||
|
|
||
| def test_add_remove(self): | ||
| mpm = metric_producer.MetricProducerManager() | ||
| self.assertEqual(mpm.metric_producers, set()) | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| mpm.add(None) | ||
| mock_mp = Mock() | ||
| mpm.add(mock_mp) | ||
| self.assertEqual(mpm.metric_producers, set([mock_mp])) | ||
| mpm.add(mock_mp) | ||
| self.assertEqual(mpm.metric_producers, set([mock_mp])) | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| mpm.remove(None) | ||
| another_mock_mp = Mock() | ||
| mpm.remove(another_mock_mp) | ||
| self.assertEqual(mpm.metric_producers, set([mock_mp])) | ||
| mpm.remove(mock_mp) | ||
| self.assertEqual(mpm.metric_producers, set()) | ||
|
|
||
| def test_get_all(self): | ||
| mp1 = Mock() | ||
| mp2 = Mock() | ||
| mpm = metric_producer.MetricProducerManager([mp1, mp2]) | ||
| got = mpm.get_all() | ||
| mpm.remove(mp1) | ||
| self.assertIn(mp1, got) | ||
| self.assertIn(mp2, got) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure If we want to do None check here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.