From 0e4197150a9754169fba23f4b5fe2319d4111098 Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Thu, 1 Nov 2018 17:19:54 -0700 Subject: [PATCH 01/10] Drop distribution buckets with non-positive bounds --- opencensus/stats/aggregation_data.py | 22 ++++++ tests/unit/stats/test_aggregation_data.py | 91 +++++++++++++++++++---- 2 files changed, 98 insertions(+), 15 deletions(-) diff --git a/opencensus/stats/aggregation_data.py b/opencensus/stats/aggregation_data.py index 5e79ee9fe..cb901dcf2 100644 --- a/opencensus/stats/aggregation_data.py +++ b/opencensus/stats/aggregation_data.py @@ -12,9 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. +import logging + from opencensus.stats import bucket_boundaries +logger = logging.getLogger(__name__) + + class BaseAggregationData(object): """Aggregation Data represents an aggregated value from a collection @@ -129,10 +134,27 @@ def __init__(self, if counts_per_bucket is None: counts_per_bucket = [0 for ii in range(len(bounds) + 1)] + elif any(cc < 0 for cc in counts_per_bucket): + raise ValueError("Bucket counts may not be negative") elif len(counts_per_bucket) != len(bounds) + 1: raise ValueError("counts_per_bucket length does not match bounds " "length") + if bounds: + if not all(bounds[ii] < bounds[ii + 1] + for ii in range(len(bounds) - 1)): + raise ValueError("bounds must be sorted in increasing order") + + dropped = [bb for bb in bounds if bb <= 0] + if dropped: + logger.warn("Bucket boundaries must be positive. Dropping " + "boundaries {}" + .format(dropped)) + bounds = bounds[len(dropped):] + dropped_counts = counts_per_bucket[:len(dropped):] + counts_per_bucket = counts_per_bucket[len(dropped):] + counts_per_bucket[0] += sum(dropped_counts) + self._counts_per_bucket = counts_per_bucket self._bounds = bucket_boundaries.BucketBoundaries( boundaries=bounds).boundaries diff --git a/tests/unit/stats/test_aggregation_data.py b/tests/unit/stats/test_aggregation_data.py index 30766e4c2..1f2cdc7f8 100644 --- a/tests/unit/stats/test_aggregation_data.py +++ b/tests/unit/stats/test_aggregation_data.py @@ -88,8 +88,8 @@ def test_constructor(self): _min = 0 _max = 1 sum_of_sqd_deviations = mock.Mock() - counts_per_bucket = [1, 1, 1, 1] - bounds = [0, 1.0 / 2.0, 1] + counts_per_bucket = [1, 1, 1] + bounds = [1.0 / 2.0, 1] dist_agg_data = aggregation_data_module.DistributionAggregationData( mean_data=mean_data, @@ -106,8 +106,8 @@ def test_constructor(self): self.assertEqual(1, dist_agg_data.max) self.assertEqual(sum_of_sqd_deviations, dist_agg_data.sum_of_sqd_deviations) - self.assertEqual([1, 1, 1, 1], dist_agg_data.counts_per_bucket) - self.assertEqual([0, 1.0 / 2.0, 1], dist_agg_data.bounds) + self.assertEqual([1, 1, 1], dist_agg_data.counts_per_bucket) + self.assertEqual([1.0 / 2.0, 1], dist_agg_data.bounds) self.assertIsNotNone(dist_agg_data.sum) self.assertEqual(0, dist_agg_data.variance) @@ -124,6 +124,17 @@ def test_init_bad_bucket_counts(self): counts_per_bucket=[0, 0, 0], bounds=[0, 1, 2]) + # Check that counts aren't negative + with self.assertRaises(ValueError): + aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[0, 2, -2, 0], + bounds=[0, 1, 2]) + # And check that we don't throw given the right args aggregation_data_module.DistributionAggregationData( mean_data=mock.Mock(), @@ -134,6 +145,56 @@ def test_init_bad_bucket_counts(self): counts_per_bucket=[0, 0, 0, 0], bounds=[0, 1, 2]) + def test_init_bad_bounds(self): + # Check that bounds are unique + with self.assertRaises(ValueError): + aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[0, 0, 0, 0], + bounds=[0, 1, 1]) + + # Check that bounds are sorted + with self.assertRaises(ValueError): + aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[0, 0, 0, 0], + bounds=[0, 2, 1]) + + def test_init_negative_bounds(self): + # Check that non-positive-bound counts are summed + da1 = aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[3, 4, 5, 6, 7], + bounds=[-1, 0, 1, 10]) + + self.assertEqual(da1.bounds, [1, 10]) + self.assertEqual(da1.counts_per_bucket, [12, 6, 7]) + + # Check the same for all non-positive bounds + da2 = aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[3, 4, 5], + bounds=[-1, 0]) + + self.assertEqual(da2.bounds, []) + self.assertEqual(da2.counts_per_bucket, [12]) + def test_constructor_with_exemplar(self): timestamp = time.time() attachments = {"One": "one", "Two": "two"} @@ -146,8 +207,8 @@ def test_constructor_with_exemplar(self): _min = 0 _max = 1 sum_of_sqd_deviations = mock.Mock() - counts_per_bucket = [1, 1, 1, 1] - bounds = [0, 1.0 / 2.0, 1] + counts_per_bucket = [1, 1, 1] + bounds = [1.0 / 2.0, 1] exemplars = [exemplar_1, exemplar_2] dist_agg_data = aggregation_data_module.DistributionAggregationData( @@ -166,9 +227,9 @@ def test_constructor_with_exemplar(self): self.assertEqual(1, dist_agg_data.max) self.assertEqual(sum_of_sqd_deviations, dist_agg_data.sum_of_sqd_deviations) - self.assertEqual([1, 1, 1, 1], dist_agg_data.counts_per_bucket) - self.assertEqual([exemplar_1, exemplar_2], dist_agg_data.exemplars[3]) - self.assertEqual([0, 1.0 / 2.0, 1], dist_agg_data.bounds) + self.assertEqual([1, 1, 1], dist_agg_data.counts_per_bucket) + self.assertEqual([exemplar_1, exemplar_2], dist_agg_data.exemplars[2]) + self.assertEqual([1.0 / 2.0, 1], dist_agg_data.bounds) self.assertIsNotNone(dist_agg_data.sum) self.assertEqual(0, dist_agg_data.variance) @@ -261,8 +322,8 @@ def test_add_sample(self): _min = 0 _max = 1 sum_of_sqd_deviations = 2 - counts_per_bucket = [1, 1, 1, 1, 1] - bounds = [0, 0.5, 1, 1.5] + counts_per_bucket = [1, 1, 1, 1] + bounds = [0.5, 1, 1.5] value = 3 @@ -307,8 +368,8 @@ def test_add_sample_attachment(self): _min = 0 _max = 1 sum_of_sqd_deviations = 2 - counts_per_bucket = [1, 1, 1, 1, 1] - bounds = [0, 0.5, 1, 1.5] + counts_per_bucket = [1, 1, 1, 1] + bounds = [0.5, 1, 1.5] value = 3 timestamp = time.time() @@ -326,14 +387,14 @@ def test_add_sample_attachment(self): bounds=bounds, exemplars=exemplar_1) - self.assertEqual({4: exemplar_1}, dist_agg_data.exemplars) + self.assertEqual({3: exemplar_1}, dist_agg_data.exemplars) dist_agg_data.add_sample(value, timestamp, attachments) self.assertEqual(0, dist_agg_data.min) self.assertEqual(3, dist_agg_data.max) self.assertEqual(2, dist_agg_data.count_data) self.assertEqual(2.0, dist_agg_data.mean_data) - self.assertEqual(3, dist_agg_data.exemplars[4].value) + self.assertEqual(3, dist_agg_data.exemplars[3].value) count_data = 4 dist_agg_data = aggregation_data_module.DistributionAggregationData( From 3a3c449b6d6aa1d4d3f6f549db515e5c879b154b Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Fri, 2 Nov 2018 15:37:45 -0700 Subject: [PATCH 02/10] Fix hanging async tests and a few incidental lint errors. --- tests/unit/common/transports/test_async.py | 29 ++++++++++++++-------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/unit/common/transports/test_async.py b/tests/unit/common/transports/test_async.py index 6cf0d366b..b4d55e62d 100644 --- a/tests/unit/common/transports/test_async.py +++ b/tests/unit/common/transports/test_async.py @@ -18,6 +18,12 @@ from opencensus.common.transports import async_ + +# Don't let workers wait between exports in testing +wait_period_patch = mock.patch( + 'opencensus.common.transports.async_._WAIT_PERIOD', 0) + + class Test_Worker(unittest.TestCase): def _start_worker(self, worker): @@ -35,7 +41,7 @@ def test_constructor(self): max_batch_size = 20 worker = async_._Worker(exporter, grace_period=grace_period, - max_batch_size=max_batch_size) + max_batch_size=max_batch_size) self.assertEqual(worker.exporter, exporter) self.assertEqual(worker._grace_period, grace_period) @@ -67,8 +73,6 @@ def test_stop(self): mock_thread, mock_atexit = self._start_worker(worker) - thread = worker._thread - worker.stop() self.assertEqual(worker._queue.qsize(), 1) @@ -129,7 +133,8 @@ def test__thread_main(self): worker.enqueue(trace2) worker._queue.put_nowait(async_._WORKER_TERMINATOR) - worker._thread_main() + with wait_period_patch: + worker._thread_main() self.assertTrue(worker.exporter.emit.called) self.assertEqual(worker._queue.qsize(), 0) @@ -164,7 +169,8 @@ def test__thread_main_batches(self): worker._queue.put_nowait(async_._WORKER_TERMINATOR) - worker._thread_main() + with wait_period_patch: + worker._thread_main() self.assertEqual(worker._queue.qsize(), 0) @@ -193,7 +199,8 @@ def emit(self, span): worker.enqueue(span_data1) worker.enqueue(span_data2) - worker._thread_main() + with wait_period_patch: + worker._thread_main() self.assertEqual(exporter.exported, [span_data1]) @@ -225,7 +232,8 @@ def emit(self, span): worker.enqueue(span_data2) worker.enqueue(async_._WORKER_TERMINATOR) - worker._thread_main() + with wait_period_patch: + worker._thread_main() # Span 2 should throw an exception, only span 0 and 1 are left self.assertEqual(exporter.exported, span_data0 + span_data1) @@ -238,7 +246,6 @@ def emit(self, span): # and the data was dropped. self.assertEqual(worker._queue.qsize(), 0) - def test_flush(self): from six.moves import queue @@ -259,7 +266,7 @@ def test_constructor(self): autospec=True) exporter = mock.Mock() - with patch_worker as mock_worker: + with patch_worker: transport = async_.AsyncTransport(exporter) self.assertTrue(transport.worker.start.called) @@ -271,7 +278,7 @@ def test_export(self): autospec=True) exporter = mock.Mock() - with patch_worker as mock_worker: + with patch_worker: transport = async_.AsyncTransport(exporter) trace = { @@ -289,7 +296,7 @@ def test_flush(self): autospec=True) exporter = mock.Mock() - with patch_worker as mock_worker: + with patch_worker: transport = async_.AsyncTransport(exporter) transport.flush() From 407b2bb88a015c1813d73c9cef48e1656401acdf Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 15:25:30 -0800 Subject: [PATCH 03/10] Update prometheus exporter for missing 0 bound and update tests. --- .../stats/exporters/prometheus_exporter.py | 30 +- .../stats/exporter/test_prometheus_stats.py | 125 +++--- .../stats/exporter/test_stackdriver_stats.py | 395 ++++++++++-------- tests/unit/stats/test_aggregation.py | 8 +- 4 files changed, 297 insertions(+), 261 deletions(-) diff --git a/opencensus/stats/exporters/prometheus_exporter.py b/opencensus/stats/exporters/prometheus_exporter.py index e23a0d4d6..715b8904e 100644 --- a/opencensus/stats/exporters/prometheus_exporter.py +++ b/opencensus/stats/exporters/prometheus_exporter.py @@ -164,31 +164,13 @@ def to_metric(self, desc, view): labels=labels) elif isinstance(agg_data, aggregation_data_module.DistributionAggregationData): + + assert(agg_data.bounds == sorted(agg_data.bounds)) points = {} - # Histograms are cumulative in Prometheus. - # 1. Sort buckets in ascending order but, retain - # their indices for reverse lookup later on. - # TODO: If there is a guarantee that distribution elements - # are always sorted, then skip the sorting. - indices_map = {} - buckets = [] - i = 0 - for boundarie in view.aggregation.boundaries.boundaries: - if boundarie not in indices_map \ - or indices_map == {}: # pragma: NO COVER - indices_map[str(boundarie)] = i - buckets.append(str(boundarie)) - i += 1 - - buckets.sort() - - # 2. Now that the buckets are sorted by magnitude - # we can create cumulative indicesmap them back by reverse index cum_count = 0 - for bucket in buckets: - i = indices_map[bucket] - cum_count += int(agg_data.counts_per_bucket[i]) - points[bucket] = cum_count + for ii, bound in enumerate(agg_data.bounds): + cum_count += agg_data.counts_per_bucket[ii] + points[str(bound)] = cum_count labels = desc['labels'] if points is None else None return HistogramMetricFamily(name=desc['name'], documentation=desc['documentation'], @@ -217,7 +199,7 @@ def to_metric(self, desc, view): % type(agg_data)) def collect(self): # pragma: NO COVER - """ Collect fetches the statistics from OpenCensus + """Collect fetches the statistics from OpenCensus and delivers them as Prometheus Metrics. Collect is invoked everytime a prometheus.Gatherer is run for example when the HTTP endpoint is invoked by Prometheus. diff --git a/tests/unit/stats/exporter/test_prometheus_stats.py b/tests/unit/stats/exporter/test_prometheus_stats.py index 02e11e7c4..5237ecf50 100644 --- a/tests/unit/stats/exporter/test_prometheus_stats.py +++ b/tests/unit/stats/exporter/test_prometheus_stats.py @@ -12,20 +12,20 @@ # See the License for the specific language governing permissions and # limitations under the License. -import unittest +from datetime import datetime import mock +import unittest -from datetime import datetime -from opencensus.stats import stats as stats_module -from opencensus.tags import tag_value as tag_value_module -from opencensus.stats.exporters import prometheus_exporter as prometheus -from opencensus.tags import tag_map as tag_map_module from opencensus.stats import aggregation as aggregation_module from opencensus.stats import measure as measure_module +from opencensus.stats import stats as stats_module from opencensus.stats import view as view_module from opencensus.stats import view_data as view_data_module +from opencensus.stats.exporters import prometheus_exporter as prometheus from opencensus.tags import tag_key as tag_key_module -from prometheus_client.core import CollectorRegistry +from opencensus.tags import tag_map as tag_map_module +from opencensus.tags import tag_value as tag_value_module + MiB = 1 << 20 FRONTEND_KEY = tag_key_module.TagKey("my.org/keys/frontend") @@ -37,44 +37,47 @@ "my.org/measure/video_size_test2", "size of processed videos", "By") VIDEO_SIZE_MEASURE_FLOAT = measure_module.MeasureFloat( - "my.org/measure/video_size_test-float", "size of processed videos-float", "By") + "my.org/measure/video_size_test-float", "size of processed videos-float", + "By") VIDEO_SIZE_VIEW_NAME = "my.org/views/video_size_test2" VIDEO_SIZE_DISTRIBUTION = aggregation_module.DistributionAggregation( - [0.0, 16.0 * MiB, 256.0 * MiB]) -VIDEO_SIZE_VIEW = view_module.View(VIDEO_SIZE_VIEW_NAME, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - VIDEO_SIZE_DISTRIBUTION) -REGISTERED_VIEW = {'test1_my.org/views/video_size_test2-my.org/keys/frontend': - {'documentation': 'processed video size over time', - 'labels': ['my.org/keys/frontend'], - 'name': 'test1_my.org/views/video_size_test2'}} - -REGISTERED_VIEW2 = {'opencensus_my.org/views/video_size_test2-my.org/keys/frontend': - {'documentation': 'processed video size over time', - 'labels': ['my.org/keys/frontend'], - 'name': 'opencensus_my.org/views/video_size_test2'}} + [16.0 * MiB, 256.0 * MiB]) +VIDEO_SIZE_VIEW = view_module.View( + VIDEO_SIZE_VIEW_NAME, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE, VIDEO_SIZE_DISTRIBUTION) +REGISTERED_VIEW = { + 'test1_my.org/views/video_size_test2-my.org/keys/frontend': { + 'documentation': 'processed video size over time', + 'labels': ['my.org/keys/frontend'], + 'name': 'test1_my.org/views/video_size_test2' + } +} + +REGISTERED_VIEW2 = { + 'opencensus_my.org/views/video_size_test2-my.org/keys/frontend': { + 'documentation': 'processed video size over time', + 'labels': ['my.org/keys/frontend'], + 'name': 'opencensus_my.org/views/video_size_test2' + } +} class TestOptionsPrometheus(unittest.TestCase): - def test_options_constructor(self): option = prometheus.Options("test1") self.assertEqual(option.namespace, "test1") def test_options_constructor_with_params(self): registry = mock.Mock() - option = prometheus.Options("test1",8001,"localhost",registry) + option = prometheus.Options("test1", 8001, "localhost", registry) self.assertEqual(option.namespace, "test1") - self.assertEqual(option.port,8001) - self.assertEqual(option.address,"localhost") + self.assertEqual(option.port, 8001) + self.assertEqual(option.address, "localhost") self.assertEqual(option.registry, registry) class TestCollectorPrometheus(unittest.TestCase): - def test_collector_constructor(self): options = prometheus.Options("test1") self.assertEqual(options.namespace, "test1") @@ -110,9 +113,8 @@ def test_collector_add_view_data(self): registry = mock.Mock() start_time = datetime.utcnow() end_time = datetime.utcnow() - view_data = view_data_module.ViewData(view=VIDEO_SIZE_VIEW, - start_time=start_time, - end_time=end_time) + view_data = view_data_module.ViewData( + view=VIDEO_SIZE_VIEW, start_time=start_time, end_time=end_time) options = prometheus.Options("test1", 8001, "localhost", registry) collector = prometheus.Collector(options=options, view_data={}) collector.register_view(VIDEO_SIZE_VIEW) @@ -121,14 +123,11 @@ def test_collector_add_view_data(self): collector.collect() self.assertEqual(v_data, collector.view_data) - def test_collector_to_metric_count(self): agg = aggregation_module.CountAggregation(256) view = view_module.View(VIDEO_SIZE_VIEW_NAME, "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg) + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg) registry = mock.Mock() view_data = mock.Mock() options = prometheus.Options("test1", 8001, "localhost", registry) @@ -146,9 +145,7 @@ def test_collector_to_metric_sum(self): agg = aggregation_module.SumAggregation(256.0) view = view_module.View(VIDEO_SIZE_VIEW_NAME, "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg) + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg) registry = mock.Mock() view_data = mock.Mock() options = prometheus.Options("test1", 8001, "localhost", registry) @@ -166,9 +163,7 @@ def test_collector_to_metric_last_value(self): agg = aggregation_module.LastValueAggregation(256) view = view_module.View(VIDEO_SIZE_VIEW_NAME, "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg) + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg) registry = mock.Mock() view_data = mock.Mock() options = prometheus.Options("test1", 8001, "localhost", registry) @@ -194,15 +189,13 @@ def test_collector_to_metric_histogram(self): self.assertEqual(desc['name'], metric.name) self.assertEqual(desc['documentation'], metric.documentation) self.assertEqual('histogram', metric.type) - self.assertEqual(5, len(metric.samples)) + self.assertEqual(4, len(metric.samples)) def test_collector_to_metric_invalid_dist(self): agg = mock.Mock() view = view_module.View(VIDEO_SIZE_VIEW_NAME, "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg) + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg) registry = mock.Mock() view_data = mock.Mock() options = prometheus.Options("test1", 8001, "localhost", registry) @@ -210,22 +203,22 @@ def test_collector_to_metric_invalid_dist(self): collector.register_view(view) desc = collector.registered_views[list(REGISTERED_VIEW)[0]] - with self.assertRaisesRegexp(ValueError, 'unsupported aggregation type '): + with self.assertRaisesRegexp( + ValueError, + 'unsupported aggregation type '): collector.to_metric(desc=desc, view=view) def test_collector_collect(self): agg = aggregation_module.LastValueAggregation(256) - view = view_module.View("new_view", - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg) + view = view_module.View("new_view", "processed video size over time", + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg) registry = mock.Mock() view_data = mock.Mock() options = prometheus.Options("test2", 8001, "localhost", registry) collector = prometheus.Collector(options=options, view_data=view_data) collector.register_view(view) - desc = collector.registered_views['test2_new_view-my.org/keys/frontend'] + desc = collector.registered_views[ + 'test2_new_view-my.org/keys/frontend'] collector.to_metric(desc=desc, view=view) registry = mock.Mock() @@ -239,13 +232,13 @@ def test_collector_collect(self): self.assertEqual(desc['name'], metric.name) self.assertEqual(desc['documentation'], metric.documentation) self.assertEqual('histogram', metric.type) - self.assertEqual(5, len(metric.samples)) + self.assertEqual(4, len(metric.samples)) class TestPrometheusStatsExporter(unittest.TestCase): - def test_exporter_constructor_no_namespace(self): - with self.assertRaisesRegexp(ValueError, 'Namespace can not be empty string.'): + with self.assertRaisesRegexp(ValueError, + 'Namespace can not be empty string.'): prometheus.new_stats_exporter(prometheus.Options()) def test_emit(self): @@ -262,11 +255,16 @@ def test_emit(self): measure_map = stats_recorder.new_measurement_map() measure_map.measure_int_put(VIDEO_SIZE_MEASURE, 25 * MiB) measure_map.record(tag_map) - exporter.export([exporter.collector.view_data['opencensus_my.org/views/video_size_test2-my.org/keys/frontend']]) + exporter.export([ + exporter.collector.view_data[( + 'opencensus_my.org/views/video_size_test2-my.org' + '/keys/frontend')] + ]) self.assertIsInstance( - exporter.collector.view_data['opencensus_my.org/views/video_size_test2-my.org/keys/frontend'], - view_data_module.ViewData) + exporter.collector.view_data[( + 'opencensus_my.org/views/video_size_test2-my.org' + '/keys/frontend')], view_data_module.ViewData) self.assertEqual(REGISTERED_VIEW2, exporter.collector.registered_views) self.assertEqual(options, exporter.options) self.assertEqual(options.registry, exporter.gatherer) @@ -279,7 +277,8 @@ def test_tag_keys_to_labels(self): self.assertEqual(tags, labels) def test_view_name(self): - view_name = prometheus.view_name(namespace="opencensus", view=VIDEO_SIZE_VIEW) + view_name = prometheus.view_name( + namespace="opencensus", view=VIDEO_SIZE_VIEW) self.assertEqual("opencensus_my.org/views/video_size_test2", view_name) def test_view_name_without_namespace(self): @@ -287,7 +286,7 @@ def test_view_name_without_namespace(self): self.assertEqual("my.org/views/video_size_test2", view_name) def test_view_signature(self): - view_signature = prometheus.view_signature(namespace="", view=VIDEO_SIZE_VIEW) - self.assertEqual("my.org/views/video_size_test2-my.org/keys/frontend", view_signature) - - + view_signature = prometheus.view_signature( + namespace="", view=VIDEO_SIZE_VIEW) + self.assertEqual("my.org/views/video_size_test2-my.org/keys/frontend", + view_signature) diff --git a/tests/unit/stats/exporter/test_stackdriver_stats.py b/tests/unit/stats/exporter/test_stackdriver_stats.py index 1b1b7fc8c..fb8d4738b 100644 --- a/tests/unit/stats/exporter/test_stackdriver_stats.py +++ b/tests/unit/stats/exporter/test_stackdriver_stats.py @@ -28,7 +28,6 @@ from opencensus.tags import tag_map as tag_map_module from opencensus.tags import tag_value as tag_value_module - MiB = 1 << 20 FRONTEND_KEY = tag_key_module.TagKey("my.org/keys/frontend") FRONTEND_KEY_FLOAT = tag_key_module.TagKey("my.org/keys/frontend-FLOAT") @@ -39,16 +38,15 @@ "my.org/measure/video_size_test2", "size of processed videos", "By") VIDEO_SIZE_MEASURE_FLOAT = measure_module.MeasureFloat( - "my.org/measure/video_size_test-float", "size of processed videos-float", "By") + "my.org/measure/video_size_test-float", "size of processed videos-float", + "By") VIDEO_SIZE_VIEW_NAME = "my.org/views/video_size_test2" VIDEO_SIZE_DISTRIBUTION = aggregation_module.DistributionAggregation( - [0.0, 16.0 * MiB, 256.0 * MiB]) -VIDEO_SIZE_VIEW = view_module.View(VIDEO_SIZE_VIEW_NAME, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - VIDEO_SIZE_DISTRIBUTION) + [16.0 * MiB, 256.0 * MiB]) +VIDEO_SIZE_VIEW = view_module.View( + VIDEO_SIZE_VIEW_NAME, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE, VIDEO_SIZE_DISTRIBUTION) class _Client(object): @@ -57,7 +55,6 @@ def __init__(self, client_info=None): class TestOptions(unittest.TestCase): - def test_options_blank(self): option = stackdriver.Options() @@ -65,7 +62,8 @@ def test_options_blank(self): self.assertEqual(option.resource, "") def test_options_parameters(self): - option = stackdriver.Options(project_id="project-id", metric_prefix="sample") + option = stackdriver.Options( + project_id="project-id", metric_prefix="sample") self.assertEqual(option.project_id, "project-id") self.assertEqual(option.metric_prefix, "sample") @@ -74,13 +72,12 @@ def test_default_monitoring_labels_blank(self): self.assertIsNone(option.default_monitoring_labels) def test_default_monitoring_labels(self): - default_labels = {'key1':'value1'} + default_labels = {'key1': 'value1'} option = stackdriver.Options(default_monitoring_labels=default_labels) self.assertEqual(option.default_monitoring_labels, default_labels) class TestStackdriverStatsExporter(unittest.TestCase): - def test_constructor(self): exporter = stackdriver.StackdriverStatsExporter() @@ -88,25 +85,29 @@ def test_constructor(self): def test_constructor_param(self): project_id = 1 - default_labels = {'key1':'value1'} + default_labels = {'key1': 'value1'} exporter = stackdriver.StackdriverStatsExporter( - options=stackdriver.Options(project_id=project_id), - default_labels=default_labels) + options=stackdriver.Options(project_id=project_id), + default_labels=default_labels) - self.assertEqual(exporter.options.project_id,project_id) - self.assertEqual(exporter.default_labels,default_labels) + self.assertEqual(exporter.options.project_id, project_id) + self.assertEqual(exporter.default_labels, default_labels) def test_blank_project(self): - self.assertRaises(Exception, stackdriver.new_stats_exporter, stackdriver.Options(project_id="")) + self.assertRaises(Exception, stackdriver.new_stats_exporter, + stackdriver.Options(project_id="")) def test_not_blank_project(self): patch_client = mock.patch( - 'opencensus.stats.exporters.stackdriver_exporter.monitoring_v3.MetricServiceClient', _Client) + ('opencensus.stats.exporters.stackdriver_exporter' + '.monitoring_v3.MetricServiceClient'), _Client) with patch_client: - exporter_created = stackdriver.new_stats_exporter(stackdriver.Options(project_id=1)) + exporter_created = stackdriver.new_stats_exporter( + stackdriver.Options(project_id=1)) - self.assertIsInstance(exporter_created, stackdriver.StackdriverStatsExporter) + self.assertIsInstance(exporter_created, + stackdriver.StackdriverStatsExporter) def test_get_user_agent_slug(self): self.assertIn(__version__, stackdriver.get_user_agent_slug()) @@ -120,8 +121,7 @@ def test_client_info_user_agent(self): """ patch_client = mock.patch( 'opencensus.stats.exporters.stackdriver_exporter.monitoring_v3' - '.MetricServiceClient', - _Client) + '.MetricServiceClient', _Client) with patch_client: exporter = stackdriver.new_stats_exporter( @@ -153,13 +153,16 @@ def test_remove_invalid_chars(self): self.assertEqual(result, "abc") def test_singleton_with_params(self): - default_labels = {'key1':'value1'} + default_labels = {'key1': 'value1'} patch_client = mock.patch( - 'opencensus.stats.exporters.stackdriver_exporter.monitoring_v3.MetricServiceClient', + ('opencensus.stats.exporters.stackdriver_exporter' + '.monitoring_v3.MetricServiceClient'), _Client) with patch_client: - exporter_created = stackdriver.new_stats_exporter(stackdriver.Options(project_id=1,default_monitoring_labels=default_labels)) + exporter_created = stackdriver.new_stats_exporter( + stackdriver.Options( + project_id=1, default_monitoring_labels=default_labels)) self.assertEqual(exporter_created.default_labels, default_labels) @@ -168,48 +171,52 @@ def test_get_task_value(self): self.assertNotEqual(task_value, "") def test_set_default_labels(self): - labels = {'key':'value'} + labels = {'key': 'value'} exporter = stackdriver.StackdriverStatsExporter() exporter.set_default_labels(labels) self.assertEqual(exporter.default_labels, labels) def test_new_label_descriptors(self): - defaults = {'key1':'value1'} + defaults = {'key1': 'value1'} keys = [FRONTEND_KEY] - output = stackdriver.new_label_descriptors(defaults,keys) - self.assertEqual(len(output),2) + output = stackdriver.new_label_descriptors(defaults, keys) + self.assertEqual(len(output), 2) def test_namespacedviews(self): view_name = "view-1" - expected_view_name_namespaced = "custom.googleapis.com/opencensus/%s" % view_name + expected_view_name_namespaced = ("custom.googleapis.com/opencensus/{}" + .format(view_name)) view_name_namespaced = stackdriver.namespaced_view_name(view_name, "") self.assertEqual(expected_view_name_namespaced, view_name_namespaced) expected_view_name_namespaced = "kubernetes.io/myorg/%s" % view_name - view_name_namespaced = stackdriver.namespaced_view_name(view_name, "kubernetes.io/myorg") + view_name_namespaced = stackdriver.namespaced_view_name( + view_name, "kubernetes.io/myorg") self.assertEqual(expected_view_name_namespaced, view_name_namespaced) def test_on_register_view(self): client = mock.Mock() view_none = None option = stackdriver.Options(project_id="project-test") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) exporter.on_register_view(VIDEO_SIZE_VIEW) exporter.on_register_view(view_none) self.assertTrue(client.create_metric_descriptor.called) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_emit(self, monitor_resource_mock): client = mock.Mock() start_time = datetime.utcnow() end_time = datetime.utcnow() - v_data = view_data_module.ViewData(view=VIDEO_SIZE_VIEW, - start_time=start_time, - end_time=end_time) + v_data = view_data_module.ViewData( + view=VIDEO_SIZE_VIEW, start_time=start_time, end_time=end_time) view_data = [v_data] option = stackdriver.Options(project_id="project-test") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) exporter.emit(view_data) exporter.emit(None) self.assertTrue(client.create_time_series.called) @@ -228,9 +235,8 @@ def test_export_with_data(self): transport = mock.Mock() start_time = datetime.utcnow() end_time = datetime.utcnow() - v_data = view_data_module.ViewData(view=VIDEO_SIZE_VIEW, - start_time=start_time, - end_time=end_time) + v_data = view_data_module.ViewData( + view=VIDEO_SIZE_VIEW, start_time=start_time, end_time=end_time) view_data = [v_data] option = stackdriver.Options(project_id="project-test") exporter = stackdriver.StackdriverStatsExporter( @@ -241,39 +247,41 @@ def test_export_with_data(self): def test_handle_upload_no_data(self): client = mock.Mock() option = stackdriver.Options(project_id="project-test") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) exporter.handle_upload(None) self.assertFalse(client.create_time_series.called) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_handle_upload_with_data(self, monitor_resource_mock): client = mock.Mock() start_time = datetime.utcnow() end_time = datetime.utcnow() - v_data = view_data_module.ViewData(view=VIDEO_SIZE_VIEW, - start_time=start_time, - end_time=end_time) + v_data = view_data_module.ViewData( + view=VIDEO_SIZE_VIEW, start_time=start_time, end_time=end_time) view_data = [v_data] option = stackdriver.Options(project_id="project-test") - exporter = stackdriver.StackdriverStatsExporter(options=option, - client=client) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) exporter.handle_upload(view_data) self.assertTrue(client.create_time_series.called) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_make_request(self, monitor_resource_mock): client = mock.Mock() start_time = datetime.utcnow() end_time = datetime.utcnow() - v_data = view_data_module.ViewData(view=VIDEO_SIZE_VIEW, - start_time=start_time, - end_time=end_time) + v_data = view_data_module.ViewData( + view=VIDEO_SIZE_VIEW, start_time=start_time, end_time=end_time) view_data = [v_data] option = stackdriver.Options(project_id="project-test") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) - requests = exporter.make_request(view_data,1) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) + requests = exporter.make_request(view_data, 1) self.assertEqual(len(requests), 1) def test_stackdriver_register_exporter(self): @@ -282,7 +290,8 @@ def test_stackdriver_register_exporter(self): exporter = mock.Mock() if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) registered_exporters = len(view_manager.measure_to_view_map.exporters) @@ -290,19 +299,23 @@ def test_stackdriver_register_exporter(self): self.assertEqual(registered_exporters, 1) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_create_timeseries(self, monitor_resource_mock): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", resource="global") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", resource="global") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) stats = stats_module.Stats() view_manager = stats.view_manager stats_recorder = stats.stats_recorder if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) @@ -316,32 +329,39 @@ def test_create_timeseries(self, monitor_resource_mock): measure_map.record(tag_map) - v_data = measure_map.measure_to_view_map.get_view(VIDEO_SIZE_VIEW_NAME, None) + v_data = measure_map.measure_to_view_map.get_view( + VIDEO_SIZE_VIEW_NAME, None) time_series = exporter.create_time_series_list(v_data, "", "") self.assertEquals(time_series.resource.type, "global") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - time_series = exporter.create_time_series_list(v_data, "global", "kubernetes.io/myorg") - self.assertEquals(time_series.metric.type, "kubernetes.io/myorg/my.org/views/video_size_test2") + time_series = exporter.create_time_series_list(v_data, "global", + "kubernetes.io/myorg") + self.assertEquals(time_series.metric.type, + "kubernetes.io/myorg/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' 'MonitoredResourceUtil.get_instance') def test_create_timeseries_with_resource(self, monitor_resource_mock): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", resource="global") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", resource="global") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) stats = stats_module.Stats() view_manager = stats.view_manager stats_recorder = stats.stats_recorder if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) @@ -355,7 +375,8 @@ def test_create_timeseries_with_resource(self, monitor_resource_mock): measure_map.record(tag_map) - v_data = measure_map.measure_to_view_map.get_view(VIDEO_SIZE_VIEW_NAME, None) + v_data = measure_map.measure_to_view_map.get_view( + VIDEO_SIZE_VIEW_NAME, None) # check for gce_instance monitored resource mocked_labels = { @@ -368,18 +389,25 @@ def test_create_timeseries_with_resource(self, monitor_resource_mock): monitor_resource_mock.return_value = mock.Mock() monitor_resource_mock.return_value.resource_type = 'gce_instance' - monitor_resource_mock.return_value.get_resource_labels.return_value = mocked_labels + monitor_resource_mock.return_value.get_resource_labels.return_value =\ + mocked_labels time_series = exporter.create_time_series_list(v_data, "", "") self.assertEquals(time_series.resource.type, "gce_instance") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - self.assertEquals(time_series.resource.labels['instance_id'], 'my-instance') - self.assertEquals(time_series.resource.labels['project_id'], 'my-project') + self.assertEquals(time_series.resource.labels['instance_id'], + 'my-instance') + self.assertEquals(time_series.resource.labels['project_id'], + 'my-project') self.assertEquals(time_series.resource.labels['zone'], 'us-east1') time_series = exporter.create_time_series_list(v_data, "global", "") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) # check for gke_container monitored resource @@ -394,16 +422,21 @@ def test_create_timeseries_with_resource(self, monitor_resource_mock): monitor_resource_mock.return_value = mock.Mock() monitor_resource_mock.return_value.resource_type = 'gke_container' - monitor_resource_mock.return_value.get_resource_labels.return_value = mocked_labels + monitor_resource_mock.return_value.get_resource_labels.return_value =\ + mocked_labels time_series = exporter.create_time_series_list(v_data, "", "") self.assertEquals(time_series.resource.type, "k8s_container") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - self.assertEquals(time_series.resource.labels['project_id'], 'my-project') + self.assertEquals(time_series.resource.labels['project_id'], + 'my-project') self.assertEquals(time_series.resource.labels['location'], 'us-east1') self.assertEquals(time_series.resource.labels['pod_name'], 'localhost') - self.assertEquals(time_series.resource.labels['namespace_name'], 'namespace') + self.assertEquals(time_series.resource.labels['namespace_name'], + 'namespace') self.assertEquals(time_series.resource.labels['container_name'], '') # check for aws_ec2_instance monitored resource @@ -415,51 +448,61 @@ def test_create_timeseries_with_resource(self, monitor_resource_mock): monitor_resource_mock.return_value = mock.Mock() monitor_resource_mock.return_value.resource_type = 'aws_ec2_instance' - monitor_resource_mock.return_value.get_resource_labels.return_value = mocked_labels + monitor_resource_mock.return_value.get_resource_labels.return_value =\ + mocked_labels time_series = exporter.create_time_series_list(v_data, "", "") self.assertEquals(time_series.resource.type, "aws_ec2_instance") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - self.assertEquals(time_series.resource.labels['instance_id'], 'my-instance') - self.assertEquals(time_series.resource.labels['aws_account'], 'my-project') - self.assertEquals(time_series.resource.labels['region'], 'aws:us-east1') + self.assertEquals(time_series.resource.labels['instance_id'], + 'my-instance') + self.assertEquals(time_series.resource.labels['aws_account'], + 'my-project') + self.assertEquals(time_series.resource.labels['region'], + 'aws:us-east1') # check for out of box monitored resource monitor_resource_mock.return_value = mock.Mock() monitor_resource_mock.return_value.resource_type = '' - monitor_resource_mock.return_value.get_resource_labels.return_value = mock.Mock() + monitor_resource_mock.return_value.get_resource_labels.return_value =\ + mock.Mock() time_series = exporter.create_time_series_list(v_data, "", "") self.assertEquals(time_series.resource.type, 'global') - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/my.org/views/video_size_test2") + self.assertEquals( + time_series.metric.type, + "custom.googleapis.com/opencensus/my.org/views/video_size_test2") self.assertIsNotNone(time_series) - @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_create_timeseries_str_tagvalue(self, monitor_resource_mock): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", resource="global") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", resource="global") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) stats = stats_module.Stats() view_manager = stats.view_manager stats_recorder = stats.stats_recorder if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) agg_1 = aggregation_module.LastValueAggregation(value=2) view_name1 = "view-name1" - new_view1 = view_module.View(view_name1, - "processed video size over time", - [FRONTEND_KEY_INT], - VIDEO_SIZE_MEASURE, - agg_1) + new_view1 = view_module.View( + view_name1, "processed video size over time", [FRONTEND_KEY_INT], + VIDEO_SIZE_MEASURE, agg_1) view_manager.register_view(new_view1) @@ -476,34 +519,39 @@ def test_create_timeseries_str_tagvalue(self, monitor_resource_mock): v_data = measure_map.measure_to_view_map.get_view(view_name1, None) - time_series = exporter.create_time_series_list(v_data, "global", "kubernetes.io/myorg/") - self.assertEquals(time_series.metric.type, "kubernetes.io/myorg/view-name1") + time_series = exporter.create_time_series_list(v_data, "global", + "kubernetes.io/myorg/") + self.assertEquals(time_series.metric.type, + "kubernetes.io/myorg/view-name1") self.assertIsNotNone(time_series) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) - def test_create_timeseries_last_value_float_tagvalue(self, monitor_resource_mock): + 'MonitoredResourceUtil.get_instance', + return_value=None) + def test_create_timeseries_last_value_float_tagvalue( + self, monitor_resource_mock): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", resource="global") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", resource="global") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) stats = stats_module.Stats() view_manager = stats.view_manager stats_recorder = stats.stats_recorder if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) agg_1 = aggregation_module.LastValueAggregation(value=2) view_name1 = "view-name1" - new_view1 = view_module.View(view_name1, - "processed video size over time", - [FRONTEND_KEY_FLOAT], - VIDEO_SIZE_MEASURE_FLOAT, - agg_1) + new_view1 = view_module.View( + view_name1, "processed video size over time", [FRONTEND_KEY_FLOAT], + VIDEO_SIZE_MEASURE_FLOAT, agg_1) view_manager.register_view(new_view1) @@ -520,34 +568,38 @@ def test_create_timeseries_last_value_float_tagvalue(self, monitor_resource_mock v_data = measure_map.measure_to_view_map.get_view(view_name1, None) - time_series = exporter.create_time_series_list(v_data,"global", "kubernetes.io/myorg") - self.assertEquals(time_series.metric.type, "kubernetes.io/myorg/view-name1") + time_series = exporter.create_time_series_list(v_data, "global", + "kubernetes.io/myorg") + self.assertEquals(time_series.metric.type, + "kubernetes.io/myorg/view-name1") self.assertIsNotNone(time_series) @mock.patch('opencensus.stats.exporters.stackdriver_exporter.' - 'MonitoredResourceUtil.get_instance', return_value=None) + 'MonitoredResourceUtil.get_instance', + return_value=None) def test_create_timeseries_float_tagvalue(self, monitor_resource_mock): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", resource="global") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", resource="global") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) stats = stats_module.Stats() view_manager = stats.view_manager stats_recorder = stats.stats_recorder if len(view_manager.measure_to_view_map.exporters) > 0: - view_manager.unregister_exporter(view_manager.measure_to_view_map.exporters[0]) + view_manager.unregister_exporter( + view_manager.measure_to_view_map.exporters[0]) view_manager.register_exporter(exporter) agg_2 = aggregation_module.SumAggregation(sum=2.2) view_name2 = "view-name2" - new_view2 = view_module.View(view_name2, - "processed video size over time", - [FRONTEND_KEY_FLOAT], - VIDEO_SIZE_MEASURE_FLOAT, - agg_2) + new_view2 = view_module.View( + view_name2, "processed video size over time", [FRONTEND_KEY_FLOAT], + VIDEO_SIZE_MEASURE_FLOAT, agg_2) view_manager.register_view(new_view2) @@ -565,101 +617,104 @@ def test_create_timeseries_float_tagvalue(self, monitor_resource_mock): v_data = measure_map.measure_to_view_map.get_view(view_name2, None) time_series = exporter.create_time_series_list(v_data, "global", "") - self.assertEquals(time_series.metric.type, "custom.googleapis.com/opencensus/view-name2") + self.assertEquals(time_series.metric.type, + "custom.googleapis.com/opencensus/view-name2") self.assertIsNotNone(time_series) def test_create_metric_descriptor_count(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") - view_name_count= "view-count" + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") + view_name_count = "view-count" agg_count = aggregation_module.CountAggregation(count=2) - view_count = view_module.View(view_name_count, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg_count) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + view_count = view_module.View( + view_name_count, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE, agg_count) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(view_count) self.assertIsNotNone(desc) def test_create_metric_descriptor_sum_int(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") - view_name_sum_int= "view-sum-int" + view_name_sum_int = "view-sum-int" agg_sum = aggregation_module.SumAggregation(sum=2) - view_sum_int = view_module.View(view_name_sum_int, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg_sum) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + view_sum_int = view_module.View( + view_name_sum_int, "processed video size over time", + [FRONTEND_KEY], VIDEO_SIZE_MEASURE, agg_sum) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(view_sum_int) self.assertIsNotNone(desc) def test_create_metric_descriptor_sum_float(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") - view_name_sum_float= "view-sum-float" + view_name_sum_float = "view-sum-float" agg_sum = aggregation_module.SumAggregation(sum=2) - view_sum_float = view_module.View(view_name_sum_float, - "processed video size over time", - [FRONTEND_KEY_FLOAT], - VIDEO_SIZE_MEASURE_FLOAT, - agg_sum) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + view_sum_float = view_module.View( + view_name_sum_float, "processed video size over time", + [FRONTEND_KEY_FLOAT], VIDEO_SIZE_MEASURE_FLOAT, agg_sum) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(view_sum_float) self.assertIsNotNone(desc) def test_create_metric_descriptor(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(VIDEO_SIZE_VIEW) self.assertIsNotNone(desc) - def test_create_metric_descriptor_last_value_int(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") - view_name_base= "view-base" + view_name_base = "view-base" agg_base = aggregation_module.LastValueAggregation() - view_base = view_module.View(view_name_base, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg_base) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + view_base = view_module.View( + view_name_base, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE, agg_base) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(view_base) self.assertIsNotNone(desc) def test_create_metric_descriptor_last_value_float(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") - view_name_base= "view-base" + view_name_base = "view-base" agg_base = aggregation_module.LastValueAggregation() - view_base = view_module.View(view_name_base, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE_FLOAT, - agg_base) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) + view_base = view_module.View( + view_name_base, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE_FLOAT, agg_base) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) desc = exporter.create_metric_descriptor(view_base) self.assertIsNotNone(desc) def test_create_metric_descriptor_base(self): client = mock.Mock() - option = stackdriver.Options(project_id="project-test", metric_prefix="teste") + option = stackdriver.Options( + project_id="project-test", metric_prefix="teste") - view_name_base= "view-base" + view_name_base = "view-base" agg_base = aggregation_module.BaseAggregation() - view_base = view_module.View(view_name_base, - "processed video size over time", - [FRONTEND_KEY], - VIDEO_SIZE_MEASURE, - agg_base) - exporter = stackdriver.StackdriverStatsExporter(options=option, client=client) - self.assertRaises(Exception, exporter.create_metric_descriptor, view_base) + view_base = view_module.View( + view_name_base, "processed video size over time", [FRONTEND_KEY], + VIDEO_SIZE_MEASURE, agg_base) + exporter = stackdriver.StackdriverStatsExporter( + options=option, client=client) + self.assertRaises(Exception, exporter.create_metric_descriptor, + view_base) diff --git a/tests/unit/stats/test_aggregation.py b/tests/unit/stats/test_aggregation.py index 71b2d546d..936dde057 100644 --- a/tests/unit/stats/test_aggregation.py +++ b/tests/unit/stats/test_aggregation.py @@ -100,14 +100,14 @@ def test_constructor_defaults(self): distribution_aggregation.aggregation_type) def test_constructor_explicit(self): - boundaries = ["test"] - distribution = {1: "test"} + boundaries = [1, 2] + distribution = [0, 1, 2] distribution_aggregation = aggregation_module.DistributionAggregation( boundaries=boundaries, distribution=distribution) - self.assertEqual(["test"], + self.assertEqual([1, 2], distribution_aggregation.boundaries.boundaries) - self.assertEqual({1: "test"}, distribution_aggregation.distribution) + self.assertEqual([0, 1, 2], distribution_aggregation.distribution) self.assertEqual(aggregation_module.Type.DISTRIBUTION, distribution_aggregation.aggregation_type) From 1d1ba0911d807857de5e40972e4503251ef1e2ff Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 15:32:09 -0800 Subject: [PATCH 04/10] Update aggregation tests for 0 bounds --- tests/unit/stats/test_aggregation_data.py | 41 ++++------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/tests/unit/stats/test_aggregation_data.py b/tests/unit/stats/test_aggregation_data.py index 1f2cdc7f8..a869b1850 100644 --- a/tests/unit/stats/test_aggregation_data.py +++ b/tests/unit/stats/test_aggregation_data.py @@ -122,7 +122,7 @@ def test_init_bad_bucket_counts(self): max_=mock.Mock(), sum_of_sqd_deviations=mock.Mock(), counts_per_bucket=[0, 0, 0], - bounds=[0, 1, 2]) + bounds=[1, 2, 3]) # Check that counts aren't negative with self.assertRaises(ValueError): @@ -133,7 +133,7 @@ def test_init_bad_bucket_counts(self): max_=mock.Mock(), sum_of_sqd_deviations=mock.Mock(), counts_per_bucket=[0, 2, -2, 0], - bounds=[0, 1, 2]) + bounds=[1, 2, 3]) # And check that we don't throw given the right args aggregation_data_module.DistributionAggregationData( @@ -143,7 +143,7 @@ def test_init_bad_bucket_counts(self): max_=mock.Mock(), sum_of_sqd_deviations=mock.Mock(), counts_per_bucket=[0, 0, 0, 0], - bounds=[0, 1, 2]) + bounds=[1, 2, 3]) def test_init_bad_bounds(self): # Check that bounds are unique @@ -155,7 +155,7 @@ def test_init_bad_bounds(self): max_=mock.Mock(), sum_of_sqd_deviations=mock.Mock(), counts_per_bucket=[0, 0, 0, 0], - bounds=[0, 1, 1]) + bounds=[1, 2, 2]) # Check that bounds are sorted with self.assertRaises(ValueError): @@ -166,34 +166,7 @@ def test_init_bad_bounds(self): max_=mock.Mock(), sum_of_sqd_deviations=mock.Mock(), counts_per_bucket=[0, 0, 0, 0], - bounds=[0, 2, 1]) - - def test_init_negative_bounds(self): - # Check that non-positive-bound counts are summed - da1 = aggregation_data_module.DistributionAggregationData( - mean_data=mock.Mock(), - count_data=mock.Mock(), - min_=mock.Mock(), - max_=mock.Mock(), - sum_of_sqd_deviations=mock.Mock(), - counts_per_bucket=[3, 4, 5, 6, 7], - bounds=[-1, 0, 1, 10]) - - self.assertEqual(da1.bounds, [1, 10]) - self.assertEqual(da1.counts_per_bucket, [12, 6, 7]) - - # Check the same for all non-positive bounds - da2 = aggregation_data_module.DistributionAggregationData( - mean_data=mock.Mock(), - count_data=mock.Mock(), - min_=mock.Mock(), - max_=mock.Mock(), - sum_of_sqd_deviations=mock.Mock(), - counts_per_bucket=[3, 4, 5], - bounds=[-1, 0]) - - self.assertEqual(da2.bounds, []) - self.assertEqual(da2.counts_per_bucket, [12]) + bounds=[1, 3, 2]) def test_constructor_with_exemplar(self): timestamp = time.time() @@ -292,8 +265,8 @@ def test_variance(self): _min = mock.Mock() _max = mock.Mock() sum_of_sqd_deviations = mock.Mock() - counts_per_bucket = [1, 1, 1, 1] - bounds = [0, 1.0 / 2.0, 1] + counts_per_bucket = [1, 1, 1] + bounds = [1.0 / 2.0, 1] dist_agg_data = aggregation_data_module.DistributionAggregationData( mean_data=mean_data, count_data=count_data, From e4374fa37711f0eb0f411d22e6c071b4ee89abca Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 16:12:44 -0800 Subject: [PATCH 05/10] Drop measurement maps with negative values at record time. --- opencensus/stats/measure_to_view_map.py | 17 ++++++++++++++--- tests/unit/stats/test_measure_to_view_map.py | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/opencensus/stats/measure_to_view_map.py b/opencensus/stats/measure_to_view_map.py index de366fce7..9107f7b05 100644 --- a/opencensus/stats/measure_to_view_map.py +++ b/opencensus/stats/measure_to_view_map.py @@ -12,10 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -from opencensus.stats.view_data import ViewData from collections import defaultdict -import logging import copy +import logging + +from opencensus.stats import view_data as view_data_module + + +logger = logging.getLogger(__name__) class MeasureToViewMap(object): @@ -91,10 +95,17 @@ def register_view(self, view, timestamp): if registered_measure is None: self._registered_measures[measure.name] = measure self._measure_to_view_data_list_map[view.measure.name].append( - ViewData(view=view, start_time=timestamp, end_time=timestamp)) + view_data_module.ViewData(view=view, start_time=timestamp, + end_time=timestamp)) def record(self, tags, measurement_map, timestamp, attachments=None): """records stats with a set of tags""" + for measure, value in measurement_map.items(): + if value < 0: + logger.warning("Recorded values must be non-negative, " + "dropping values") + return + for measure, value in measurement_map.items(): if measure != self._registered_measures.get(measure.name): return diff --git a/tests/unit/stats/test_measure_to_view_map.py b/tests/unit/stats/test_measure_to_view_map.py index 738bf9133..d9e0b1116 100644 --- a/tests/unit/stats/test_measure_to_view_map.py +++ b/tests/unit/stats/test_measure_to_view_map.py @@ -303,6 +303,26 @@ def test_record(self): attachments=None) self.assertIsNone(record) + def test_record_negative_value(self): + """Check that we warn and drop negative measures at record time.""" + measure = mock.Mock() + view_data = mock.Mock() + measure_to_view_map = measure_to_view_map_module.MeasureToViewMap() + measure_to_view_map._registered_measures = {measure.name: measure} + measure_to_view_map._measure_to_view_data_list_map = { + measure.name: [view_data] + } + with mock.patch('opencensus.stats.measure_to_view_map.logger') \ + as mock_logger: + record = measure_to_view_map.record( + tags=mock.Mock(), + measurement_map={measure: -1}, + timestamp=mock.Mock(), + attachments=None) + self.assertIsNone(record) + mock_logger.warning.assert_called_once() + view_data.record.assert_not_called() + def test_record_with_exporter(self): exporter = mock.Mock() measure_name = "test_measure" From c77429a1532e35892a27dc1fc8e8fd194824a6a1 Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 18:57:48 -0800 Subject: [PATCH 06/10] Move boundary checks to DistributionAggregation and out of DistributionAggregationData. --- opencensus/stats/aggregation.py | 16 +++++++++---- opencensus/stats/aggregation_data.py | 28 +++++++---------------- tests/unit/stats/test_aggregation.py | 13 +++++++++++ tests/unit/stats/test_aggregation_data.py | 8 +++---- 4 files changed, 36 insertions(+), 29 deletions(-) diff --git a/opencensus/stats/aggregation.py b/opencensus/stats/aggregation.py index f2bdc10ec..4599283f2 100644 --- a/opencensus/stats/aggregation.py +++ b/opencensus/stats/aggregation.py @@ -123,11 +123,17 @@ class DistributionAggregation(BaseAggregation): :param aggregation_type: represents the type of this aggregation """ - def __init__( - self, - boundaries=None, - distribution=None, - aggregation_type=Type.DISTRIBUTION): + + def __init__(self, + boundaries=None, + distribution=None, + aggregation_type=Type.DISTRIBUTION): + if boundaries: + if not all(boundaries[ii] < boundaries[ii + 1] + for ii in range(len(boundaries) - 1)): + raise ValueError("bounds must be sorted in increasing order") + boundaries = [bb for bb in boundaries if bb > 0] + super(DistributionAggregation, self).__init__( buckets=boundaries, aggregation_type=aggregation_type) self._boundaries = bucket_boundaries.BucketBoundaries(boundaries) diff --git a/opencensus/stats/aggregation_data.py b/opencensus/stats/aggregation_data.py index cb901dcf2..78f867b93 100644 --- a/opencensus/stats/aggregation_data.py +++ b/opencensus/stats/aggregation_data.py @@ -131,29 +131,17 @@ def __init__(self, self._sum_of_sqd_deviations = sum_of_sqd_deviations if bounds is None: bounds = [] + else: + assert bounds == list(sorted(set(bounds))) if counts_per_bucket is None: counts_per_bucket = [0 for ii in range(len(bounds) + 1)] - elif any(cc < 0 for cc in counts_per_bucket): - raise ValueError("Bucket counts may not be negative") - elif len(counts_per_bucket) != len(bounds) + 1: - raise ValueError("counts_per_bucket length does not match bounds " - "length") - - if bounds: - if not all(bounds[ii] < bounds[ii + 1] - for ii in range(len(bounds) - 1)): - raise ValueError("bounds must be sorted in increasing order") - - dropped = [bb for bb in bounds if bb <= 0] - if dropped: - logger.warn("Bucket boundaries must be positive. Dropping " - "boundaries {}" - .format(dropped)) - bounds = bounds[len(dropped):] - dropped_counts = counts_per_bucket[:len(dropped):] - counts_per_bucket = counts_per_bucket[len(dropped):] - counts_per_bucket[0] += sum(dropped_counts) + else: + assert all(cc >= 0 for cc in counts_per_bucket) + assert len(counts_per_bucket) == len(bounds) + 1 + + assert bounds == sorted(bounds) + assert all(bb > 0 for bb in bounds) self._counts_per_bucket = counts_per_bucket self._bounds = bucket_boundaries.BucketBoundaries( diff --git a/tests/unit/stats/test_aggregation.py b/tests/unit/stats/test_aggregation.py index 936dde057..79bb4e93c 100644 --- a/tests/unit/stats/test_aggregation.py +++ b/tests/unit/stats/test_aggregation.py @@ -122,3 +122,16 @@ def test_min_max(self): self.assertEqual(da.aggregation_data.min, -10) self.assertEqual(da.aggregation_data.max, 10) + + def test_init_bad_boundaries(self): + """Check that boundaries must be sorted and unique.""" + with self.assertRaises(ValueError): + aggregation_module.DistributionAggregation([1, 3, 2]) + with self.assertRaises(ValueError): + aggregation_module.DistributionAggregation([1, 1, 2]) + + def test_init_negative_boundaries(self): + """Check that non-positive boundaries are dropped.""" + da = aggregation_module.DistributionAggregation([-2, -1, 0, 1, 2]) + self.assertEqual(da.boundaries.boundaries, [1, 2]) + self.assertEqual(da.aggregation_data.bounds, [1, 2]) diff --git a/tests/unit/stats/test_aggregation_data.py b/tests/unit/stats/test_aggregation_data.py index a869b1850..a8c13bca8 100644 --- a/tests/unit/stats/test_aggregation_data.py +++ b/tests/unit/stats/test_aggregation_data.py @@ -114,7 +114,7 @@ def test_constructor(self): def test_init_bad_bucket_counts(self): # Check that len(counts_per_bucket) == len(bounds) + 1 - with self.assertRaises(ValueError): + with self.assertRaises(AssertionError): aggregation_data_module.DistributionAggregationData( mean_data=mock.Mock(), count_data=mock.Mock(), @@ -125,7 +125,7 @@ def test_init_bad_bucket_counts(self): bounds=[1, 2, 3]) # Check that counts aren't negative - with self.assertRaises(ValueError): + with self.assertRaises(AssertionError): aggregation_data_module.DistributionAggregationData( mean_data=mock.Mock(), count_data=mock.Mock(), @@ -147,7 +147,7 @@ def test_init_bad_bucket_counts(self): def test_init_bad_bounds(self): # Check that bounds are unique - with self.assertRaises(ValueError): + with self.assertRaises(AssertionError): aggregation_data_module.DistributionAggregationData( mean_data=mock.Mock(), count_data=mock.Mock(), @@ -158,7 +158,7 @@ def test_init_bad_bounds(self): bounds=[1, 2, 2]) # Check that bounds are sorted - with self.assertRaises(ValueError): + with self.assertRaises(AssertionError): aggregation_data_module.DistributionAggregationData( mean_data=mock.Mock(), count_data=mock.Mock(), From 1a4756e929b2578315fc954008923798a44551fd Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 19:27:16 -0800 Subject: [PATCH 07/10] Add a test for mixed negative bounds --- tests/unit/stats/test_aggregation_data.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/stats/test_aggregation_data.py b/tests/unit/stats/test_aggregation_data.py index a8c13bca8..4c86e5240 100644 --- a/tests/unit/stats/test_aggregation_data.py +++ b/tests/unit/stats/test_aggregation_data.py @@ -168,6 +168,17 @@ def test_init_bad_bounds(self): counts_per_bucket=[0, 0, 0, 0], bounds=[1, 3, 2]) + # Check that all bounds are positive + with self.assertRaises(AssertionError): + aggregation_data_module.DistributionAggregationData( + mean_data=mock.Mock(), + count_data=mock.Mock(), + min_=mock.Mock(), + max_=mock.Mock(), + sum_of_sqd_deviations=mock.Mock(), + counts_per_bucket=[0, 0, 0, 0], + bounds=[-1, 1, 2]) + def test_constructor_with_exemplar(self): timestamp = time.time() attachments = {"One": "one", "Two": "two"} From bce908cdc88972fe5c3b97d46f6cde199d410dd0 Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Wed, 7 Nov 2018 20:01:22 -0800 Subject: [PATCH 08/10] Warn on < 0 bounds in DistributionAggregation --- opencensus/stats/aggregation.py | 16 +++++++++++++++- tests/unit/stats/test_aggregation.py | 4 ++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/opencensus/stats/aggregation.py b/opencensus/stats/aggregation.py index 4599283f2..4a771a045 100644 --- a/opencensus/stats/aggregation.py +++ b/opencensus/stats/aggregation.py @@ -12,10 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +import logging + from opencensus.stats import bucket_boundaries from opencensus.stats import aggregation_data +logger = logging.getLogger(__name__) + + class Type(object): """ The type of aggregation function used on a View. @@ -132,7 +137,16 @@ def __init__(self, if not all(boundaries[ii] < boundaries[ii + 1] for ii in range(len(boundaries) - 1)): raise ValueError("bounds must be sorted in increasing order") - boundaries = [bb for bb in boundaries if bb > 0] + for ii, bb in enumerate(boundaries): + if bb > 0: + break + else: + ii += 1 + if ii: + logger.warning("Dropping {} negative bucket boundaries, the " + "values must be strictly > 0" + .format(ii)) + boundaries = boundaries[ii:] super(DistributionAggregation, self).__init__( buckets=boundaries, aggregation_type=aggregation_type) diff --git a/tests/unit/stats/test_aggregation.py b/tests/unit/stats/test_aggregation.py index 79bb4e93c..d384a7669 100644 --- a/tests/unit/stats/test_aggregation.py +++ b/tests/unit/stats/test_aggregation.py @@ -135,3 +135,7 @@ def test_init_negative_boundaries(self): da = aggregation_module.DistributionAggregation([-2, -1, 0, 1, 2]) self.assertEqual(da.boundaries.boundaries, [1, 2]) self.assertEqual(da.aggregation_data.bounds, [1, 2]) + + da2 = aggregation_module.DistributionAggregation([-2, -1]) + self.assertEqual(da2.boundaries.boundaries, []) + self.assertEqual(da2.aggregation_data.bounds, []) From 87e21f366fb33009925b215b43a5b845a645933e Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Thu, 8 Nov 2018 19:25:02 -0800 Subject: [PATCH 09/10] Move negative value checks to MeasurementMap --- opencensus/stats/measure_to_view_map.py | 10 +--- opencensus/stats/measurement_map.py | 27 +++++++++- tests/unit/stats/test_measure_to_view_map.py | 10 ++-- tests/unit/stats/test_measurement_map.py | 53 +++++++++++++++++--- 4 files changed, 77 insertions(+), 23 deletions(-) diff --git a/opencensus/stats/measure_to_view_map.py b/opencensus/stats/measure_to_view_map.py index 9107f7b05..83b13a531 100644 --- a/opencensus/stats/measure_to_view_map.py +++ b/opencensus/stats/measure_to_view_map.py @@ -19,9 +19,6 @@ from opencensus.stats import view_data as view_data_module -logger = logging.getLogger(__name__) - - class MeasureToViewMap(object): """Measure To View Map stores a map from names of Measures to specific View Datas @@ -100,12 +97,7 @@ def register_view(self, view, timestamp): def record(self, tags, measurement_map, timestamp, attachments=None): """records stats with a set of tags""" - for measure, value in measurement_map.items(): - if value < 0: - logger.warning("Recorded values must be non-negative, " - "dropping values") - return - + assert all(vv >= 0 for vv in measurement_map.values()) for measure, value in measurement_map.items(): if measure != self._registered_measures.get(measure.name): return diff --git a/opencensus/stats/measurement_map.py b/opencensus/stats/measurement_map.py index 956f25a99..446f501c8 100644 --- a/opencensus/stats/measurement_map.py +++ b/opencensus/stats/measurement_map.py @@ -13,9 +13,14 @@ # limitations under the License. from datetime import datetime +import logging + from opencensus.tags import execution_context +logger = logging.getLogger(__name__) + + class MeasurementMap(object): """Measurement Map is a map from Measures to measured values to be recorded at the same time @@ -33,6 +38,10 @@ def __init__(self, measure_to_view_map, attachments=None): self._measurement_map = {} self._measure_to_view_map = measure_to_view_map self._attachments = attachments + # If the user tries to record a negative value for any measurement, + # refuse to record all measurements from this map. Recording negative + # measurements will become an error in a later release. + self._invalid = False @property def measurement_map(self): @@ -75,11 +84,27 @@ def measure_put_attachment(self, key, value): self._attachments[key] = value - def record(self, tag_map_tags=execution_context.get_current_tag_map()): + def record(self, tag_map_tags=None): """records all the measures at the same time with a tag_map. tag_map could either be explicitly passed to the method, or implicitly read from current execution context. """ + if tag_map_tags is None: + tag_map_tags = execution_context.get_current_tag_map() + if self._invalid: + logger.warning("Measurement map has included negative value " + "measurements, refusing to record") + return + for measure, value in self.measurement_map.items(): + if value < 0: + self._invalid = True + logger.warning("Dropping values, value to record must be " + "non-negative") + logger.info("Measure '{}' has negative value ({}), refusing " + "to record measurements from {}" + .format(measure.name, value, self)) + return + self.measure_to_view_map.record( tags=tag_map_tags, measurement_map=self.measurement_map, diff --git a/tests/unit/stats/test_measure_to_view_map.py b/tests/unit/stats/test_measure_to_view_map.py index d9e0b1116..60a8bb0d4 100644 --- a/tests/unit/stats/test_measure_to_view_map.py +++ b/tests/unit/stats/test_measure_to_view_map.py @@ -312,15 +312,11 @@ def test_record_negative_value(self): measure_to_view_map._measure_to_view_data_list_map = { measure.name: [view_data] } - with mock.patch('opencensus.stats.measure_to_view_map.logger') \ - as mock_logger: - record = measure_to_view_map.record( + with self.assertRaises(AssertionError): + measure_to_view_map.record( tags=mock.Mock(), measurement_map={measure: -1}, - timestamp=mock.Mock(), - attachments=None) - self.assertIsNone(record) - mock_logger.warning.assert_called_once() + timestamp=mock.Mock()) view_data.record.assert_not_called() def test_record_with_exporter(self): diff --git a/tests/unit/stats/test_measurement_map.py b/tests/unit/stats/test_measurement_map.py index 0aa23c405..94c132bda 100644 --- a/tests/unit/stats/test_measurement_map.py +++ b/tests/unit/stats/test_measurement_map.py @@ -12,15 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -import unittest import mock +import unittest + from opencensus.stats import measurement_map as measurement_map_module -from opencensus.stats.measure_to_view_map import MeasureToViewMap from opencensus.tags import execution_context -from opencensus.stats.measure import BaseMeasure -from opencensus.stats.measure import MeasureInt -from opencensus.stats import measure_to_view_map as measure_to_view_map_module -from opencensus.stats.view import View + + +logger_patch = mock.patch('opencensus.stats.measurement_map.logger') class TestMeasurementMap(unittest.TestCase): @@ -137,3 +136,45 @@ def test_record_against_implicit_tag_map(self): execution_context.set_current_tag_map(tags) measurement_map.record() self.assertTrue(measure_to_view_map.record.called) + + def test_record_negative_value(self): + """Check that we refuse to record negative measurements.""" + measurement_map = measurement_map_module.MeasurementMap(mock.Mock()) + measurement_map.measure_int_put(mock.Mock(), 1) + measurement_map.measure_int_put(mock.Mock(), -1) + + with logger_patch as mock_logger: + measurement_map.record() + + self.assertTrue(measurement_map._invalid) + measurement_map._measure_to_view_map.record.assert_not_called() + mock_logger.warning.assert_called_once() + + def test_record_previous_negative_value(self): + """Check that negative measurements poison the map.""" + measurement_map = measurement_map_module.MeasurementMap(mock.Mock()) + measure = mock.Mock() + measurement_map.measure_int_put(measure, 1) + + measurement_map.record() + self.assertFalse(measurement_map._invalid) + measurement_map._measure_to_view_map.record.assert_called_once() + + measurement_map.measure_int_put(measure, -1) + measurement_map._measure_to_view_map = mock.Mock() + + with logger_patch as mock_logger: + measurement_map.record() + + self.assertTrue(measurement_map._invalid) + measurement_map._measure_to_view_map.record.assert_not_called() + mock_logger.warning.assert_called_once() + + measurement_map.measure_int_put(measure, 1) + + with logger_patch as another_mock_logger: + measurement_map.record() + + self.assertTrue(measurement_map._invalid) + measurement_map._measure_to_view_map.record.assert_not_called() + another_mock_logger.warning.assert_called_once() From ec6bb8b545eb27ef5ed222fa5cf39870be9e9634 Mon Sep 17 00:00:00 2001 From: Chris Kleinknecht Date: Thu, 8 Nov 2018 19:34:00 -0800 Subject: [PATCH 10/10] Warn against negative measurements on put --- opencensus/stats/measurement_map.py | 6 ++++++ tests/unit/stats/test_measurement_map.py | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/opencensus/stats/measurement_map.py b/opencensus/stats/measurement_map.py index 446f501c8..2c1dab23a 100644 --- a/opencensus/stats/measurement_map.py +++ b/opencensus/stats/measurement_map.py @@ -60,10 +60,16 @@ def attachments(self): def measure_int_put(self, measure, value): """associates the measure of type Int with the given value""" + if value < 0: + # Should be an error in a later release. + logger.warning("Cannot record negative values") self._measurement_map[measure] = value def measure_float_put(self, measure, value): """associates the measure of type Float with the given value""" + if value < 0: + # Should be an error in a later release. + logger.warning("Cannot record negative values") self._measurement_map[measure] = value def measure_put_attachment(self, key, value): diff --git a/tests/unit/stats/test_measurement_map.py b/tests/unit/stats/test_measurement_map.py index 94c132bda..23741f893 100644 --- a/tests/unit/stats/test_measurement_map.py +++ b/tests/unit/stats/test_measurement_map.py @@ -178,3 +178,15 @@ def test_record_previous_negative_value(self): self.assertTrue(measurement_map._invalid) measurement_map._measure_to_view_map.record.assert_not_called() another_mock_logger.warning.assert_called_once() + + def test_log_negative_puts(self): + """Check that we warn against negative measurements on put.""" + measurement_map = measurement_map_module.MeasurementMap(mock.Mock()) + + with logger_patch as mock_logger: + measurement_map.measure_int_put(mock.Mock(), -1) + mock_logger.warning.assert_called_once() + + with logger_patch as another_mock_logger: + measurement_map.measure_float_put(mock.Mock(), -1.0) + another_mock_logger.warning.assert_called_once()