From a3253e584559322e0f120b534734610dfa8696ec Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Thu, 11 Jun 2020 17:14:25 +0000 Subject: [PATCH 1/2] Rename CounterAggregator -> SumAggregator --- .../exporter/cloud_monitoring/__init__.py | 4 +- .../tests/test_cloud_monitoring.py | 30 +++---- .../test_otcollector_metrics_exporter.py | 6 +- .../tests/test_prometheus_exporter.py | 6 +- .../sdk/metrics/export/aggregate.py | 2 +- .../sdk/metrics/export/batcher.py | 6 +- .../tests/metrics/export/test_export.py | 89 +++++++++---------- .../tests/metrics/test_metrics.py | 8 +- 8 files changed, 74 insertions(+), 77 deletions(-) diff --git a/ext/opentelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py b/ext/opentelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py index 6d6af26677a..5620b00a5fa 100644 --- a/ext/opentelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py +++ b/ext/opentelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py @@ -12,7 +12,7 @@ MetricsExporter, MetricsExportResult, ) -from opentelemetry.sdk.metrics.export.aggregate import CounterAggregator +from opentelemetry.sdk.metrics.export.aggregate import SumAggregator logger = logging.getLogger(__name__) MAX_BATCH_WRITE = 200 @@ -96,7 +96,7 @@ def _get_metric_descriptor( logger.warning( "Label value %s is not a string, bool or integer", value ) - if isinstance(record.aggregator, CounterAggregator): + if isinstance(record.aggregator, SumAggregator): descriptor["metric_kind"] = MetricDescriptor.MetricKind.GAUGE else: logger.warning( diff --git a/ext/opentelemetry-exporter-cloud-monitoring/tests/test_cloud_monitoring.py b/ext/opentelemetry-exporter-cloud-monitoring/tests/test_cloud_monitoring.py index d7f98e024a7..61ed7ce0103 100644 --- a/ext/opentelemetry-exporter-cloud-monitoring/tests/test_cloud_monitoring.py +++ b/ext/opentelemetry-exporter-cloud-monitoring/tests/test_cloud_monitoring.py @@ -25,7 +25,7 @@ CloudMonitoringMetricsExporter, ) from opentelemetry.sdk.metrics.export import MetricRecord -from opentelemetry.sdk.metrics.export.aggregate import CounterAggregator +from opentelemetry.sdk.metrics.export.aggregate import SumAggregator class UnsupportedAggregator: @@ -110,7 +110,7 @@ def test_get_metric_descriptor(self): ) record = MetricRecord( - CounterAggregator(), (("label1", "value1"),), MockMetric() + SumAggregator(), (("label1", "value1"),), MockMetric() ) metric_descriptor = exporter._get_metric_descriptor(record) client.create_metric_descriptor.assert_called_with( @@ -138,7 +138,7 @@ def test_get_metric_descriptor(self): # Drop labels with values that aren't string, int or bool exporter._get_metric_descriptor( MetricRecord( - CounterAggregator(), + SumAggregator(), ( ("label1", "value1"), ("label2", dict()), @@ -198,18 +198,18 @@ def test_export(self): } ) - counter_one = CounterAggregator() - counter_one.checkpoint = 1 - counter_one.last_update_timestamp = (WRITE_INTERVAL + 1) * 1e9 + sum_agg_one = SumAggregator() + sum_agg_one.checkpoint = 1 + sum_agg_one.last_update_timestamp = (WRITE_INTERVAL + 1) * 1e9 exporter.export( [ MetricRecord( - counter_one, + sum_agg_one, (("label1", "value1"), ("label2", 1),), MockMetric(), ), MetricRecord( - counter_one, + sum_agg_one, (("label1", "value2"), ("label2", 2),), MockMetric(), ), @@ -239,18 +239,18 @@ def test_export(self): # Attempting to export too soon after another export with the exact # same labels leads to it being dropped - counter_two = CounterAggregator() - counter_two.checkpoint = 1 - counter_two.last_update_timestamp = (WRITE_INTERVAL + 2) * 1e9 + sum_agg_two = SumAggregator() + sum_agg_two.checkpoint = 1 + sum_agg_two.last_update_timestamp = (WRITE_INTERVAL + 2) * 1e9 exporter.export( [ MetricRecord( - counter_two, + sum_agg_two, (("label1", "value1"), ("label2", 1),), MockMetric(), ), MetricRecord( - counter_two, + sum_agg_two, (("label1", "value2"), ("label2", 2),), MockMetric(), ), @@ -259,11 +259,11 @@ def test_export(self): self.assertEqual(client.create_time_series.call_count, 1) # But exporting with different labels is fine - counter_two.checkpoint = 2 + sum_agg_two.checkpoint = 2 exporter.export( [ MetricRecord( - counter_two, + sum_agg_two, (("label1", "changed_label"), ("label2", 2),), MockMetric(), ), diff --git a/ext/opentelemetry-ext-opencensusexporter/tests/test_otcollector_metrics_exporter.py b/ext/opentelemetry-ext-opencensusexporter/tests/test_otcollector_metrics_exporter.py index f9070126476..f538e5acecd 100644 --- a/ext/opentelemetry-ext-opencensusexporter/tests/test_otcollector_metrics_exporter.py +++ b/ext/opentelemetry-ext-opencensusexporter/tests/test_otcollector_metrics_exporter.py @@ -81,7 +81,7 @@ def test_get_collector_metric_type(self): self.assertIs(result, metrics_pb2.MetricDescriptor.UNSPECIFIED) def test_get_collector_point(self): - aggregator = aggregate.CounterAggregator() + aggregator = aggregate.SumAggregator() int_counter = self._meter.create_metric( "testName", "testDescription", "unit", int, Counter ) @@ -122,7 +122,7 @@ def test_export(self): "testname", "testdesc", "unit", int, Counter, ["environment"] ) record = MetricRecord( - test_metric, self._key_labels, aggregate.CounterAggregator(), + test_metric, self._key_labels, aggregate.SumAggregator(), ) result = collector_exporter.export([record]) @@ -144,7 +144,7 @@ def test_translate_to_collector(self): test_metric = self._meter.create_metric( "testname", "testdesc", "unit", int, Counter, ["environment"] ) - aggregator = aggregate.CounterAggregator() + aggregator = aggregate.SumAggregator() aggregator.update(123) aggregator.take_checkpoint() record = MetricRecord(test_metric, self._key_labels, aggregator,) diff --git a/ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py b/ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py index 1862f789c0c..2ba6b701215 100644 --- a/ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py +++ b/ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py @@ -24,7 +24,7 @@ from opentelemetry.metrics import get_meter_provider, set_meter_provider from opentelemetry.sdk import metrics from opentelemetry.sdk.metrics.export import MetricRecord, MetricsExportResult -from opentelemetry.sdk.metrics.export.aggregate import CounterAggregator +from opentelemetry.sdk.metrics.export.aggregate import SumAggregator class TestPrometheusMetricExporter(unittest.TestCase): @@ -67,7 +67,7 @@ def test_shutdown(self): def test_export(self): with self._registry_register_patch: record = MetricRecord( - self._test_metric, self._labels_key, CounterAggregator(), + self._test_metric, self._labels_key, SumAggregator(), ) exporter = PrometheusMetricsExporter() result = exporter.export([record]) @@ -87,7 +87,7 @@ def test_counter_to_prometheus(self): ) labels = {"environment@": "staging", "os": "Windows"} key_labels = metrics.get_labels_as_key(labels) - aggregator = CounterAggregator() + aggregator = SumAggregator() aggregator.update(123) aggregator.take_checkpoint() record = MetricRecord(metric, key_labels, aggregator) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py index ad728d8c502..cfea391019d 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py @@ -43,7 +43,7 @@ def merge(self, other): """Combines two aggregator values.""" -class CounterAggregator(Aggregator): +class SumAggregator(Aggregator): """Aggregator for Counter metrics.""" def __init__(self): diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/batcher.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/batcher.py index 0741082d999..527760d51b5 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/batcher.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/batcher.py @@ -27,9 +27,9 @@ from opentelemetry.sdk.metrics.export import MetricRecord from opentelemetry.sdk.metrics.export.aggregate import ( Aggregator, - CounterAggregator, LastValueAggregator, MinMaxSumCountAggregator, + SumAggregator, ValueObserverAggregator, ) @@ -57,7 +57,7 @@ def aggregator_for(self, instrument_type: Type[InstrumentT]) -> Aggregator: """ # pylint:disable=R0201 if issubclass(instrument_type, (Counter, UpDownCounter)): - return CounterAggregator() + return SumAggregator() if issubclass(instrument_type, (SumObserver, UpDownSumObserver)): return LastValueAggregator() if issubclass(instrument_type, ValueRecorder): @@ -65,7 +65,7 @@ def aggregator_for(self, instrument_type: Type[InstrumentT]) -> Aggregator: if issubclass(instrument_type, ValueObserver): return ValueObserverAggregator() # TODO: Add other aggregators - return CounterAggregator() + return SumAggregator() def checkpoint_set(self) -> Sequence[MetricRecord]: """Returns a list of MetricRecords used for exporting. diff --git a/opentelemetry-sdk/tests/metrics/export/test_export.py b/opentelemetry-sdk/tests/metrics/export/test_export.py index c9a8f64d646..2f251f4976c 100644 --- a/opentelemetry-sdk/tests/metrics/export/test_export.py +++ b/opentelemetry-sdk/tests/metrics/export/test_export.py @@ -24,8 +24,8 @@ MetricRecord, ) from opentelemetry.sdk.metrics.export.aggregate import ( - CounterAggregator, MinMaxSumCountAggregator, + SumAggregator, ValueObserverAggregator, ) from opentelemetry.sdk.metrics.export.batcher import UngroupedBatcher @@ -47,7 +47,7 @@ def test_export(self): ("environment",), ) labels = {"environment": "staging"} - aggregator = CounterAggregator() + aggregator = SumAggregator() record = MetricRecord(metric, labels, aggregator) result = '{}(data="{}", labels="{}", value={})'.format( ConsoleMetricsExporter.__name__, @@ -64,17 +64,14 @@ class TestBatcher(unittest.TestCase): def test_aggregator_for_counter(self): batcher = UngroupedBatcher(True) self.assertTrue( - isinstance( - batcher.aggregator_for(metrics.Counter), CounterAggregator - ) + isinstance(batcher.aggregator_for(metrics.Counter), SumAggregator) ) def test_aggregator_for_updowncounter(self): batcher = UngroupedBatcher(True) self.assertTrue( isinstance( - batcher.aggregator_for(metrics.UpDownCounter), - CounterAggregator, + batcher.aggregator_for(metrics.UpDownCounter), SumAggregator, ) ) @@ -83,7 +80,7 @@ def test_aggregator_for_updowncounter(self): def test_checkpoint_set(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(True) - aggregator = CounterAggregator() + aggregator = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -111,7 +108,7 @@ def test_checkpoint_set_empty(self): def test_finished_collection_stateless(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(False) - aggregator = CounterAggregator() + aggregator = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -131,7 +128,7 @@ def test_finished_collection_stateless(self): def test_finished_collection_stateful(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(True) - aggregator = CounterAggregator() + aggregator = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -152,8 +149,8 @@ def test_finished_collection_stateful(self): def test_ungrouped_batcher_process_exists(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(True) - aggregator = CounterAggregator() - aggregator2 = CounterAggregator() + aggregator = SumAggregator() + aggregator2 = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -179,7 +176,7 @@ def test_ungrouped_batcher_process_exists(self): def test_ungrouped_batcher_process_not_exists(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(True) - aggregator = CounterAggregator() + aggregator = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -204,7 +201,7 @@ def test_ungrouped_batcher_process_not_exists(self): def test_ungrouped_batcher_process_not_stateful(self): meter = metrics.MeterProvider().get_meter(__name__) batcher = UngroupedBatcher(True) - aggregator = CounterAggregator() + aggregator = SumAggregator() metric = metrics.Counter( "available memory", "available memory", @@ -227,67 +224,67 @@ def test_ungrouped_batcher_process_not_stateful(self): ) -class TestCounterAggregator(unittest.TestCase): +class TestSumAggregator(unittest.TestCase): @staticmethod - def call_update(counter): + def call_update(sum_agg): update_total = 0 for _ in range(0, 100000): val = random.getrandbits(32) - counter.update(val) + sum_agg.update(val) update_total += val return update_total @mock.patch("opentelemetry.sdk.metrics.export.aggregate.time_ns") def test_update(self, time_mock): time_mock.return_value = 123 - counter = CounterAggregator() - counter.update(1.0) - counter.update(2.0) - self.assertEqual(counter.current, 3.0) - self.assertEqual(counter.last_update_timestamp, 123) + sum_agg = SumAggregator() + sum_agg.update(1.0) + sum_agg.update(2.0) + self.assertEqual(sum_agg.current, 3.0) + self.assertEqual(sum_agg.last_update_timestamp, 123) def test_checkpoint(self): - counter = CounterAggregator() - counter.update(2.0) - counter.take_checkpoint() - self.assertEqual(counter.current, 0) - self.assertEqual(counter.checkpoint, 2.0) + sum_agg = SumAggregator() + sum_agg.update(2.0) + sum_agg.take_checkpoint() + self.assertEqual(sum_agg.current, 0) + self.assertEqual(sum_agg.checkpoint, 2.0) def test_merge(self): - counter = CounterAggregator() - counter2 = CounterAggregator() - counter.checkpoint = 1.0 - counter2.checkpoint = 3.0 - counter2.last_update_timestamp = 123 - counter.merge(counter2) - self.assertEqual(counter.checkpoint, 4.0) - self.assertEqual(counter.last_update_timestamp, 123) + sum_agg = SumAggregator() + sum_agg2 = SumAggregator() + sum_agg.checkpoint = 1.0 + sum_agg2.checkpoint = 3.0 + sum_agg2.last_update_timestamp = 123 + sum_agg.merge(sum_agg2) + self.assertEqual(sum_agg.checkpoint, 4.0) + self.assertEqual(sum_agg.last_update_timestamp, 123) def test_concurrent_update(self): - counter = CounterAggregator() + sum_agg = SumAggregator() with concurrent.futures.ThreadPoolExecutor(max_workers=2) as executor: - fut1 = executor.submit(self.call_update, counter) - fut2 = executor.submit(self.call_update, counter) + fut1 = executor.submit(self.call_update, sum_agg) + fut2 = executor.submit(self.call_update, sum_agg) updapte_total = fut1.result() + fut2.result() - counter.take_checkpoint() - self.assertEqual(updapte_total, counter.checkpoint) + sum_agg.take_checkpoint() + self.assertEqual(updapte_total, sum_agg.checkpoint) def test_concurrent_update_and_checkpoint(self): - counter = CounterAggregator() + sum_agg = SumAggregator() checkpoint_total = 0 with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: - fut = executor.submit(self.call_update, counter) + fut = executor.submit(self.call_update, sum_agg) while not fut.done(): - counter.take_checkpoint() - checkpoint_total += counter.checkpoint + sum_agg.take_checkpoint() + checkpoint_total += sum_agg.checkpoint - counter.take_checkpoint() - checkpoint_total += counter.checkpoint + sum_agg.take_checkpoint() + checkpoint_total += sum_agg.checkpoint self.assertEqual(fut.result(), checkpoint_total) diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index cba10726b20..ae07c233413 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -568,27 +568,27 @@ def test_run_exception(self, logger_mock): class TestBoundCounter(unittest.TestCase): def test_add(self): - aggregator = export.aggregate.CounterAggregator() + aggregator = export.aggregate.SumAggregator() bound_metric = metrics.BoundCounter(int, True, aggregator) bound_metric.add(3) self.assertEqual(bound_metric.aggregator.current, 3) def test_add_disabled(self): - aggregator = export.aggregate.CounterAggregator() + aggregator = export.aggregate.SumAggregator() bound_counter = metrics.BoundCounter(int, False, aggregator) bound_counter.add(3) self.assertEqual(bound_counter.aggregator.current, 0) @mock.patch("opentelemetry.sdk.metrics.logger") def test_add_incorrect_type(self, logger_mock): - aggregator = export.aggregate.CounterAggregator() + aggregator = export.aggregate.SumAggregator() bound_counter = metrics.BoundCounter(int, True, aggregator) bound_counter.add(3.0) self.assertEqual(bound_counter.aggregator.current, 0) self.assertTrue(logger_mock.warning.called) def test_update(self): - aggregator = export.aggregate.CounterAggregator() + aggregator = export.aggregate.SumAggregator() bound_counter = metrics.BoundCounter(int, True, aggregator) bound_counter.update(4.0) self.assertEqual(bound_counter.aggregator.current, 4.0) From b3d38ef946b47d3f558c5228ce384af3b65c85da Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Fri, 12 Jun 2020 22:24:12 +0000 Subject: [PATCH 2/2] update changelog --- opentelemetry-sdk/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index b2ddc0d01f6..7c077dbf5ec 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- Rename CounterAggregator -> SumAggregator + ([#816](https://github.com/open-telemetry/opentelemetry-python/pull/816)) + ## 0.9b0 Released 2020-06-10