From 3d684b52ad241fe1269ef1371624681566019c72 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 5 Jun 2023 16:41:46 +0200 Subject: [PATCH 01/14] Remove console exporter metric output if empty Fixes #3198 --- CHANGELOG.md | 4 ++- .../sdk/metrics/_internal/export/__init__.py | 10 +++---- .../metrics/_internal/measurement_consumer.py | 8 +++--- .../_internal/metric_reader_storage.py | 28 ++++++++++--------- .../integration_test/test_console_exporter.py | 16 +++++++++++ 5 files changed, 43 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f1cd0c79dd..da5f08b272c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- Fix handling of empty metric collection cycles + ([#3335](https://github.com/open-telemetry/opentelemetry-python/pull/3335)) - Fix error when no LoggerProvider configured for LoggingHandler ([#3423](https://github.com/open-telemetry/opentelemetry-python/pull/3423)) - + ## Version 1.20.0/0.41b0 (2023-09-04) - Modify Prometheus exporter to translate non-monotonic Sums into Gauges diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py index 5bd94d5aacc..7096034d5fe 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py @@ -344,7 +344,7 @@ def _set_collect_callback( @abstractmethod def _receive_metrics( self, - metrics_data: "opentelemetry.sdk.metrics.export.MetricsData", + metrics_data: Optional[MetricsData], timeout_millis: float = 10_000, **kwargs, ) -> None: @@ -386,9 +386,9 @@ def __init__( preferred_aggregation=preferred_aggregation, ) self._lock = RLock() - self._metrics_data: ( + self._metrics_data: Optional[ "opentelemetry.sdk.metrics.export.MetricsData" - ) = None + ] = None def get_metrics_data( self, @@ -402,7 +402,7 @@ def get_metrics_data( def _receive_metrics( self, - metrics_data: "opentelemetry.sdk.metrics.export.MetricsData", + metrics_data: Optional[MetricsData], timeout_millis: float = 10_000, **kwargs, ) -> None: @@ -511,7 +511,7 @@ def _ticker(self) -> None: def _receive_metrics( self, - metrics_data: MetricsData, + metrics_data: Optional[MetricsData], timeout_millis: float = 10_000, **kwargs, ) -> None: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py index 9daf1eff461..a0499dc3e82 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py @@ -17,7 +17,7 @@ from abc import ABC, abstractmethod from threading import Lock from time import time_ns -from typing import Iterable, List, Mapping +from typing import List, Mapping, Optional # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics @@ -29,7 +29,7 @@ from opentelemetry.sdk.metrics._internal.metric_reader_storage import ( MetricReaderStorage, ) -from opentelemetry.sdk.metrics._internal.point import Metric +from opentelemetry.sdk.metrics._internal.point import MetricsData class MeasurementConsumer(ABC): @@ -51,7 +51,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Iterable[Metric]: + ) -> Optional[MetricsData]: pass @@ -94,7 +94,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Iterable[Metric]: + ) -> Optional[MetricsData]: with self._lock: metric_reader_storage = self._reader_storages[metric_reader] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py index bef57eaab09..f2110559a38 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py @@ -15,7 +15,7 @@ from logging import getLogger from threading import RLock from time import time_ns -from typing import Dict, List +from typing import Dict, List, Optional from opentelemetry.metrics import ( Asynchronous, @@ -119,7 +119,7 @@ def consume_measurement(self, measurement: Measurement) -> None: ): view_instrument_match.consume_measurement(measurement) - def collect(self) -> MetricsData: + def collect(self) -> Optional[MetricsData]: # Use a list instead of yielding to prevent a slow reader from holding # SDK locks @@ -231,17 +231,19 @@ def collect(self) -> MetricsData: instrument.instrumentation_scope ].metrics.extend(metrics) - return MetricsData( - resource_metrics=[ - ResourceMetrics( - resource=self._sdk_config.resource, - scope_metrics=list( - instrumentation_scope_scope_metrics.values() - ), - schema_url=self._sdk_config.resource.schema_url, - ) - ] - ) + scope_metrics = list(instrumentation_scope_scope_metrics.values()) + + if scope_metrics: + + return MetricsData( + resource_metrics=[ + ResourceMetrics( + resource=self._sdk_config.resource, + scope_metrics=scope_metrics, + schema_url=self._sdk_config.resource.schema_url, + ) + ] + ) def _handle_view_instrument_match( self, diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py index 60c4227c3b2..1b3283717ae 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py @@ -72,3 +72,19 @@ def test_console_exporter(self): self.assertEqual(metrics["attributes"], {"a": "b"}) self.assertEqual(metrics["value"], 1) + + def test_console_exporter_no_export(self): + + output = StringIO() + exporter = ConsoleMetricExporter(out=output) + reader = PeriodicExportingMetricReader( + exporter, export_interval_millis=100 + ) + provider = MeterProvider(metric_readers=[reader]) + provider.shutdown() + + output.seek(0) + actual = "".join(output.readlines()) + expected = "" + + self.assertEqual(actual, expected) From c55f8fa02fb9765769260ac062c4ff38b84ba7e2 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 15 Aug 2023 16:48:49 +0200 Subject: [PATCH 02/14] Revert "Remove console exporter metric output if empty" This reverts commit 3407110f18e2fa4d55a6aa4bf2ee92ba45b11501. --- CHANGELOG.md | 1 + .../sdk/metrics/_internal/export/__init__.py | 10 +++---- .../metrics/_internal/measurement_consumer.py | 8 +++--- .../_internal/metric_reader_storage.py | 28 +++++++++---------- .../integration_test/test_console_exporter.py | 16 ----------- 5 files changed, 23 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da5f08b272c..f5a7a983987 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix error when no LoggerProvider configured for LoggingHandler ([#3423](https://github.com/open-telemetry/opentelemetry-python/pull/3423)) + ## Version 1.20.0/0.41b0 (2023-09-04) - Modify Prometheus exporter to translate non-monotonic Sums into Gauges diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py index 7096034d5fe..5bd94d5aacc 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py @@ -344,7 +344,7 @@ def _set_collect_callback( @abstractmethod def _receive_metrics( self, - metrics_data: Optional[MetricsData], + metrics_data: "opentelemetry.sdk.metrics.export.MetricsData", timeout_millis: float = 10_000, **kwargs, ) -> None: @@ -386,9 +386,9 @@ def __init__( preferred_aggregation=preferred_aggregation, ) self._lock = RLock() - self._metrics_data: Optional[ + self._metrics_data: ( "opentelemetry.sdk.metrics.export.MetricsData" - ] = None + ) = None def get_metrics_data( self, @@ -402,7 +402,7 @@ def get_metrics_data( def _receive_metrics( self, - metrics_data: Optional[MetricsData], + metrics_data: "opentelemetry.sdk.metrics.export.MetricsData", timeout_millis: float = 10_000, **kwargs, ) -> None: @@ -511,7 +511,7 @@ def _ticker(self) -> None: def _receive_metrics( self, - metrics_data: Optional[MetricsData], + metrics_data: MetricsData, timeout_millis: float = 10_000, **kwargs, ) -> None: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py index a0499dc3e82..9daf1eff461 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py @@ -17,7 +17,7 @@ from abc import ABC, abstractmethod from threading import Lock from time import time_ns -from typing import List, Mapping, Optional +from typing import Iterable, List, Mapping # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics @@ -29,7 +29,7 @@ from opentelemetry.sdk.metrics._internal.metric_reader_storage import ( MetricReaderStorage, ) -from opentelemetry.sdk.metrics._internal.point import MetricsData +from opentelemetry.sdk.metrics._internal.point import Metric class MeasurementConsumer(ABC): @@ -51,7 +51,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Optional[MetricsData]: + ) -> Iterable[Metric]: pass @@ -94,7 +94,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Optional[MetricsData]: + ) -> Iterable[Metric]: with self._lock: metric_reader_storage = self._reader_storages[metric_reader] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py index f2110559a38..bef57eaab09 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py @@ -15,7 +15,7 @@ from logging import getLogger from threading import RLock from time import time_ns -from typing import Dict, List, Optional +from typing import Dict, List from opentelemetry.metrics import ( Asynchronous, @@ -119,7 +119,7 @@ def consume_measurement(self, measurement: Measurement) -> None: ): view_instrument_match.consume_measurement(measurement) - def collect(self) -> Optional[MetricsData]: + def collect(self) -> MetricsData: # Use a list instead of yielding to prevent a slow reader from holding # SDK locks @@ -231,19 +231,17 @@ def collect(self) -> Optional[MetricsData]: instrument.instrumentation_scope ].metrics.extend(metrics) - scope_metrics = list(instrumentation_scope_scope_metrics.values()) - - if scope_metrics: - - return MetricsData( - resource_metrics=[ - ResourceMetrics( - resource=self._sdk_config.resource, - scope_metrics=scope_metrics, - schema_url=self._sdk_config.resource.schema_url, - ) - ] - ) + return MetricsData( + resource_metrics=[ + ResourceMetrics( + resource=self._sdk_config.resource, + scope_metrics=list( + instrumentation_scope_scope_metrics.values() + ), + schema_url=self._sdk_config.resource.schema_url, + ) + ] + ) def _handle_view_instrument_match( self, diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py index 1b3283717ae..60c4227c3b2 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py @@ -72,19 +72,3 @@ def test_console_exporter(self): self.assertEqual(metrics["attributes"], {"a": "b"}) self.assertEqual(metrics["value"], 1) - - def test_console_exporter_no_export(self): - - output = StringIO() - exporter = ConsoleMetricExporter(out=output) - reader = PeriodicExportingMetricReader( - exporter, export_interval_millis=100 - ) - provider = MeterProvider(metric_readers=[reader]) - provider.shutdown() - - output.seek(0) - actual = "".join(output.readlines()) - expected = "" - - self.assertEqual(actual, expected) From 8c2a43ae568ec05281f89d8e2f97fc0802dae83f Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 16 Aug 2023 18:52:46 +0200 Subject: [PATCH 03/14] Speed up test execution --- opentelemetry-api/pyproject.toml | 4 +--- opentelemetry-sdk/pyproject.toml | 4 +--- opentelemetry-semantic-conventions/pyproject.toml | 4 +--- tox.ini | 3 +-- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/opentelemetry-api/pyproject.toml b/opentelemetry-api/pyproject.toml index adf9512cf04..9e2ba332796 100644 --- a/opentelemetry-api/pyproject.toml +++ b/opentelemetry-api/pyproject.toml @@ -1,12 +1,10 @@ [build-system] -requires = ["hatchling"] -build-backend = "hatchling.build" +requires = [] [project] name = "opentelemetry-api" description = "OpenTelemetry Python API" readme = "README.rst" -license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/opentelemetry-sdk/pyproject.toml b/opentelemetry-sdk/pyproject.toml index 603a9b1fbf9..94cf9e31ead 100644 --- a/opentelemetry-sdk/pyproject.toml +++ b/opentelemetry-sdk/pyproject.toml @@ -1,13 +1,11 @@ [build-system] -requires = ["hatchling"] -build-backend = "hatchling.build" +requires = [] [project] name = "opentelemetry-sdk" dynamic = ["version"] description = "OpenTelemetry Python SDK" readme = "README.rst" -license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/opentelemetry-semantic-conventions/pyproject.toml b/opentelemetry-semantic-conventions/pyproject.toml index 8ad02bb35cf..5586a28d5a8 100644 --- a/opentelemetry-semantic-conventions/pyproject.toml +++ b/opentelemetry-semantic-conventions/pyproject.toml @@ -1,13 +1,11 @@ [build-system] -requires = ["hatchling"] -build-backend = "hatchling.build" +requires = [] [project] name = "opentelemetry-semantic-conventions" dynamic = ["version"] description = "OpenTelemetry Semantic Conventions" readme = "README.rst" -license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/tox.ini b/tox.ini index ce37337290c..603bad04fb6 100644 --- a/tox.ini +++ b/tox.ini @@ -125,10 +125,9 @@ changedir = commands_pre = ; Install without -e to test the actual installation - py3{7,8,9,10,11}: python -m pip install -U pip setuptools wheel ; Install common packages for all the tests. These are not needed in all the ; cases but it saves a lot of boilerplate in this file. - opentelemetry: pip install {toxinidir}/opentelemetry-api {toxinidir}/opentelemetry-semantic-conventions {toxinidir}/opentelemetry-sdk {toxinidir}/tests/opentelemetry-test-utils + opentelemetry: pip install --no-deps --no-build-isolation -e {toxinidir}/opentelemetry-sdk protobuf: pip install {toxinidir}/opentelemetry-proto From a54b10030eefba530c506516ebe89c36204a65a8 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 16 Aug 2023 18:53:08 +0200 Subject: [PATCH 04/14] Do not produce any metric output without metric data Fixes #3198 --- .../_internal/_view_instrument_match.py | 8 +- .../sdk/metrics/_internal/export/__init__.py | 15 ++-- .../metrics/_internal/measurement_consumer.py | 8 +- .../_internal/metric_reader_storage.py | 81 ++++++++++--------- .../integration_test/test_console_exporter.py | 16 ++++ .../test_disable_default_views.py | 10 +-- .../test_exporter_concurrency.py | 14 ++++ .../metrics/test_metric_reader_storage.py | 10 +-- 8 files changed, 93 insertions(+), 69 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py index ab4645c82f3..d5d8064293e 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py @@ -16,7 +16,7 @@ from logging import getLogger from threading import Lock from time import time_ns -from typing import Dict, List, Sequence +from typing import Dict, List, Sequence, Optional from opentelemetry.metrics import Instrument from opentelemetry.sdk.metrics._internal.aggregation import ( @@ -126,7 +126,7 @@ def collect( self, aggregation_temporality: AggregationTemporality, collection_start_nanos: int, - ) -> Sequence[DataPointT]: + ) -> Optional[Sequence[DataPointT]]: data_points: List[DataPointT] = [] with self._lock: @@ -136,4 +136,6 @@ def collect( ) if data_point is not None: data_points.append(data_point) - return data_points + if data_points: + return data_points + return None diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py index 5bd94d5aacc..0568270ae6b 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py @@ -322,10 +322,14 @@ def collect(self, timeout_millis: float = 10_000) -> None: ) return - self._receive_metrics( - self._collect(self, timeout_millis=timeout_millis), - timeout_millis=timeout_millis, - ) + metrics = self._collect(self, timeout_millis=timeout_millis) + + if metrics is not None: + + self._receive_metrics( + metrics, + timeout_millis=timeout_millis, + ) @final def _set_collect_callback( @@ -515,8 +519,7 @@ def _receive_metrics( timeout_millis: float = 10_000, **kwargs, ) -> None: - if metrics_data is None: - return + token = attach(set_value(_SUPPRESS_INSTRUMENTATION_KEY, True)) try: with self._export_lock: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py index 9daf1eff461..fe9936f51fc 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py @@ -17,7 +17,7 @@ from abc import ABC, abstractmethod from threading import Lock from time import time_ns -from typing import Iterable, List, Mapping +from typing import Iterable, List, Mapping, Optional # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics @@ -94,7 +94,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Iterable[Metric]: + ) -> Optional[Iterable[Metric]]: with self._lock: metric_reader_storage = self._reader_storages[metric_reader] @@ -123,4 +123,6 @@ def collect( for measurement in measurements: metric_reader_storage.consume_measurement(measurement) - return self._reader_storages[metric_reader].collect() + result = self._reader_storages[metric_reader].collect() + + return result diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py index bef57eaab09..453d11a4ed5 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py @@ -152,6 +152,13 @@ def collect(self) -> MetricsData: for view_instrument_match in view_instrument_matches: + data_points = view_instrument_match.collect( + aggregation_temporality, collection_start_nanos + ) + + if data_points is None: + continue + if isinstance( # pylint: disable=protected-access view_instrument_match._aggregation, @@ -159,9 +166,7 @@ def collect(self) -> MetricsData: ): data = Sum( aggregation_temporality=aggregation_temporality, - data_points=view_instrument_match.collect( - aggregation_temporality, collection_start_nanos - ), + data_points=data_points, is_monotonic=isinstance( instrument, (Counter, ObservableCounter) ), @@ -171,20 +176,14 @@ def collect(self) -> MetricsData: view_instrument_match._aggregation, _LastValueAggregation, ): - data = Gauge( - data_points=view_instrument_match.collect( - aggregation_temporality, collection_start_nanos - ) - ) + data = Gauge(data_points=data_points) elif isinstance( # pylint: disable=protected-access view_instrument_match._aggregation, _ExplicitBucketHistogramAggregation, ): data = Histogram( - data_points=view_instrument_match.collect( - aggregation_temporality, collection_start_nanos - ), + data_points=data_points, aggregation_temporality=aggregation_temporality, ) elif isinstance( @@ -200,9 +199,7 @@ def collect(self) -> MetricsData: _ExponentialBucketHistogramAggregation, ): data = ExponentialHistogram( - data_points=view_instrument_match.collect( - aggregation_temporality, collection_start_nanos - ), + data_points=data_points, aggregation_temporality=aggregation_temporality, ) @@ -216,32 +213,38 @@ def collect(self) -> MetricsData: ) ) - if instrument.instrumentation_scope not in ( - instrumentation_scope_scope_metrics - ): - instrumentation_scope_scope_metrics[ - instrument.instrumentation_scope - ] = ScopeMetrics( - scope=instrument.instrumentation_scope, - metrics=metrics, - schema_url=instrument.instrumentation_scope.schema_url, + if metrics: + + if instrument.instrumentation_scope not in ( + instrumentation_scope_scope_metrics + ): + instrumentation_scope_scope_metrics[ + instrument.instrumentation_scope + ] = ScopeMetrics( + scope=instrument.instrumentation_scope, + metrics=metrics, + schema_url=instrument.instrumentation_scope.schema_url, + ) + else: + instrumentation_scope_scope_metrics[ + instrument.instrumentation_scope + ].metrics.extend(metrics) + + if instrumentation_scope_scope_metrics: + + return MetricsData( + resource_metrics=[ + ResourceMetrics( + resource=self._sdk_config.resource, + scope_metrics=list( + instrumentation_scope_scope_metrics.values() + ), + schema_url=self._sdk_config.resource.schema_url, ) - else: - instrumentation_scope_scope_metrics[ - instrument.instrumentation_scope - ].metrics.extend(metrics) - - return MetricsData( - resource_metrics=[ - ResourceMetrics( - resource=self._sdk_config.resource, - scope_metrics=list( - instrumentation_scope_scope_metrics.values() - ), - schema_url=self._sdk_config.resource.schema_url, - ) - ] - ) + ] + ) + + return None def _handle_view_instrument_match( self, diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py index 60c4227c3b2..1b3283717ae 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_console_exporter.py @@ -72,3 +72,19 @@ def test_console_exporter(self): self.assertEqual(metrics["attributes"], {"a": "b"}) self.assertEqual(metrics["value"], 1) + + def test_console_exporter_no_export(self): + + output = StringIO() + exporter = ConsoleMetricExporter(out=output) + reader = PeriodicExportingMetricReader( + exporter, export_interval_millis=100 + ) + provider = MeterProvider(metric_readers=[reader]) + provider.shutdown() + + output.seek(0) + actual = "".join(output.readlines()) + expected = "" + + self.assertEqual(actual, expected) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py b/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py index ad90fe9a298..d022456415b 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py @@ -31,15 +31,7 @@ def test_disable_default_views(self): counter.add(10, {"label": "value1"}) counter.add(10, {"label": "value2"}) counter.add(10, {"label": "value3"}) - self.assertEqual( - ( - reader.get_metrics_data() - .resource_metrics[0] - .scope_metrics[0] - .metrics - ), - [], - ) + self.assertIsNone(reader.get_metrics_data()) def test_disable_default_views_add_custom(self): reader = InMemoryMetricReader() diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_exporter_concurrency.py b/opentelemetry-sdk/tests/metrics/integration_test/test_exporter_concurrency.py index 045afe0b298..bbc67eac309 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_exporter_concurrency.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_exporter_concurrency.py @@ -74,6 +74,15 @@ class TestExporterConcurrency(ConcurrencyTestBase): > be called again only after the current call returns. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exportbatch + + This test also tests that a thread that calls the a + ``MetricReader.collect`` method using an asynchronous instrument is able + to perform two actions in the same thread lock space (without it being + interrupted by another thread): + + 1. Consume the measurement produced by the callback associated to the + asynchronous instrument. + 2. Export the measurement mentioned in the step above. """ def test_exporter_not_called_concurrently(self): @@ -84,7 +93,11 @@ def test_exporter_not_called_concurrently(self): ) meter_provider = MeterProvider(metric_readers=[reader]) + counter_cb_counter = 0 + def counter_cb(options: CallbackOptions): + nonlocal counter_cb_counter + counter_cb_counter += 1 yield Observation(2) meter_provider.get_meter(__name__).create_observable_counter( @@ -97,6 +110,7 @@ def test_many_threads(): self.run_with_many_threads(test_many_threads, num_threads=100) + self.assertEqual(counter_cb_counter, 100) # no thread should be in export() now self.assertEqual(exporter.count_in_export, 0) # should be one call for each thread diff --git a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py index 97b5532feae..1da6d5bcf60 100644 --- a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py +++ b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py @@ -314,15 +314,7 @@ def test_drop_aggregation(self): ) metric_reader_storage.consume_measurement(Measurement(1, counter)) - self.assertEqual( - [], - ( - metric_reader_storage.collect() - .resource_metrics[0] - .scope_metrics[0] - .metrics - ), - ) + self.assertIsNone(metric_reader_storage.collect()) def test_same_collection_start(self): From 1aeaf0cccbc8cc0d9b50f7c4304a43354fe3ed18 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 17 Aug 2023 17:07:25 +0200 Subject: [PATCH 05/14] Revert "Speed up test execution" This reverts commit 1ea9c69e5bf2ff3e1df900ce1e2f36ea8ee9bdb3. --- opentelemetry-api/pyproject.toml | 4 +++- opentelemetry-sdk/pyproject.toml | 4 +++- opentelemetry-semantic-conventions/pyproject.toml | 4 +++- tox.ini | 3 ++- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/opentelemetry-api/pyproject.toml b/opentelemetry-api/pyproject.toml index 9e2ba332796..adf9512cf04 100644 --- a/opentelemetry-api/pyproject.toml +++ b/opentelemetry-api/pyproject.toml @@ -1,10 +1,12 @@ [build-system] -requires = [] +requires = ["hatchling"] +build-backend = "hatchling.build" [project] name = "opentelemetry-api" description = "OpenTelemetry Python API" readme = "README.rst" +license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/opentelemetry-sdk/pyproject.toml b/opentelemetry-sdk/pyproject.toml index 94cf9e31ead..603a9b1fbf9 100644 --- a/opentelemetry-sdk/pyproject.toml +++ b/opentelemetry-sdk/pyproject.toml @@ -1,11 +1,13 @@ [build-system] -requires = [] +requires = ["hatchling"] +build-backend = "hatchling.build" [project] name = "opentelemetry-sdk" dynamic = ["version"] description = "OpenTelemetry Python SDK" readme = "README.rst" +license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/opentelemetry-semantic-conventions/pyproject.toml b/opentelemetry-semantic-conventions/pyproject.toml index 5586a28d5a8..8ad02bb35cf 100644 --- a/opentelemetry-semantic-conventions/pyproject.toml +++ b/opentelemetry-semantic-conventions/pyproject.toml @@ -1,11 +1,13 @@ [build-system] -requires = [] +requires = ["hatchling"] +build-backend = "hatchling.build" [project] name = "opentelemetry-semantic-conventions" dynamic = ["version"] description = "OpenTelemetry Semantic Conventions" readme = "README.rst" +license = "Apache-2.0" requires-python = ">=3.7" authors = [ { name = "OpenTelemetry Authors", email = "cncf-opentelemetry-contributors@lists.cncf.io" }, diff --git a/tox.ini b/tox.ini index 603bad04fb6..ce37337290c 100644 --- a/tox.ini +++ b/tox.ini @@ -125,9 +125,10 @@ changedir = commands_pre = ; Install without -e to test the actual installation + py3{7,8,9,10,11}: python -m pip install -U pip setuptools wheel ; Install common packages for all the tests. These are not needed in all the ; cases but it saves a lot of boilerplate in this file. - opentelemetry: pip install --no-deps --no-build-isolation -e {toxinidir}/opentelemetry-sdk + opentelemetry: pip install {toxinidir}/opentelemetry-api {toxinidir}/opentelemetry-semantic-conventions {toxinidir}/opentelemetry-sdk {toxinidir}/tests/opentelemetry-test-utils protobuf: pip install {toxinidir}/opentelemetry-proto From 032e98fccddade7b64ebdb5442274c8bc17b338d Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 17 Aug 2023 17:19:36 +0200 Subject: [PATCH 06/14] Fix lint --- .../sdk/metrics/_internal/_view_instrument_match.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py index d5d8064293e..c339cdb1d8f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py @@ -16,7 +16,7 @@ from logging import getLogger from threading import Lock from time import time_ns -from typing import Dict, List, Sequence, Optional +from typing import Dict, List, Optional, Sequence from opentelemetry.metrics import Instrument from opentelemetry.sdk.metrics._internal.aggregation import ( From c731316aed06f672672ba2b20c786da8e72c7801 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 21 Aug 2023 16:53:30 +0200 Subject: [PATCH 07/14] Add another test for histograms --- .../sdk/metrics/_internal/aggregation.py | 2 + .../integration_test/test_histogram_export.py | 85 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index ae21db907dd..1d9a40a0b73 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -140,6 +140,7 @@ def collect( Atomically return a point for the current value of the metric and reset the aggregation value. """ + if self._instrument_temporality is AggregationTemporality.DELTA: with self._lock: @@ -289,6 +290,7 @@ def collect( Atomically return a point for the current value of the metric. """ with self._lock: + if not any(self._bucket_counts): return None diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py new file mode 100644 index 00000000000..330530922c5 --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py @@ -0,0 +1,85 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from unittest import TestCase + +from opentelemetry.metrics import set_meter_provider +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import InMemoryMetricReader +from opentelemetry.sdk.resources import SERVICE_NAME, Resource + +in_memory_metric_reader = InMemoryMetricReader() + +provider = MeterProvider( + resource=Resource.create({SERVICE_NAME: "otel-test"}), + metric_readers=[in_memory_metric_reader], +) +set_meter_provider(provider) + +meter = provider.get_meter("my-meter") + +histogram = meter.create_histogram("my_histogram") +counter = meter.create_counter("my_counter") + + +class TestHistogramExport(TestCase): + def test_histogram_counter_collection(self): + + histogram.record(5, {"attribute": "value"}) + counter.add(1, {"attribute": "value_counter"}) + + metric_data = in_memory_metric_reader.get_metrics_data() + + self.assertEqual( + len(metric_data.resource_metrics[0].scope_metrics[0].metrics), 2 + ) + + self.assertEqual( + ( + metric_data.resource_metrics[0] + .scope_metrics[0] + .metrics[0] + .data.data_points[0] + .bucket_counts + ), + (0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), + ) + self.assertEqual( + ( + metric_data.resource_metrics[0] + .scope_metrics[0] + .metrics[1] + .data.data_points[0] + .value + ), + 1, + ) + + metric_data = in_memory_metric_reader.get_metrics_data() + + # FIXME ExplicitBucketHistogramAggregation is resetting counts to zero + # even if aggregation temporality is cumulative. + self.assertEqual( + len(metric_data.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + self.assertEqual( + ( + metric_data.resource_metrics[0] + .scope_metrics[0] + .metrics[0] + .data.data_points[0] + .value + ), + 1, + ) From 7ffc2b5bcad41f38935e209234ad8cd0f838db6c Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 24 Aug 2023 12:37:25 +0200 Subject: [PATCH 08/14] Remove added spacing --- .../src/opentelemetry/sdk/metrics/_internal/aggregation.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 1d9a40a0b73..ae21db907dd 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -140,7 +140,6 @@ def collect( Atomically return a point for the current value of the metric and reset the aggregation value. """ - if self._instrument_temporality is AggregationTemporality.DELTA: with self._lock: @@ -290,7 +289,6 @@ def collect( Atomically return a point for the current value of the metric. """ with self._lock: - if not any(self._bucket_counts): return None From 721d012bbde6476739b9bdb06eddd9c9f1db1bdf Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 24 Aug 2023 14:08:18 +0200 Subject: [PATCH 09/14] Update typing --- .../sdk/metrics/_internal/metric_reader_storage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py index 453d11a4ed5..ae57bd7d064 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py @@ -15,7 +15,7 @@ from logging import getLogger from threading import RLock from time import time_ns -from typing import Dict, List +from typing import Dict, List, Optional from opentelemetry.metrics import ( Asynchronous, @@ -119,7 +119,7 @@ def consume_measurement(self, measurement: Measurement) -> None: ): view_instrument_match.consume_measurement(measurement) - def collect(self) -> MetricsData: + def collect(self) -> Optional[MetricsData]: # Use a list instead of yielding to prevent a slow reader from holding # SDK locks From d9a078cbadd124d426a2ca4c8e6ae98b006b7f06 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 24 Aug 2023 14:11:26 +0200 Subject: [PATCH 10/14] Fix typing for collect --- .../opentelemetry/sdk/metrics/_internal/measurement_consumer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py index fe9936f51fc..c5e81678dcb 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py @@ -51,7 +51,7 @@ def collect( self, metric_reader: "opentelemetry.sdk.metrics.MetricReader", timeout_millis: float = 10_000, - ) -> Iterable[Metric]: + ) -> Optional[Iterable[Metric]]: pass From 401cba9da1014e495934e83ac64d7ad4385b6e96 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 24 Aug 2023 14:19:02 +0200 Subject: [PATCH 11/14] Fix histogram test --- .../integration_test/test_histogram_export.py | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py index 330530922c5..81d419819a4 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py @@ -14,28 +14,25 @@ from unittest import TestCase -from opentelemetry.metrics import set_meter_provider from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import InMemoryMetricReader from opentelemetry.sdk.resources import SERVICE_NAME, Resource -in_memory_metric_reader = InMemoryMetricReader() -provider = MeterProvider( - resource=Resource.create({SERVICE_NAME: "otel-test"}), - metric_readers=[in_memory_metric_reader], -) -set_meter_provider(provider) - -meter = provider.get_meter("my-meter") +class TestHistogramExport(TestCase): + def test_histogram_counter_collection(self): -histogram = meter.create_histogram("my_histogram") -counter = meter.create_counter("my_counter") + in_memory_metric_reader = InMemoryMetricReader() + provider = MeterProvider( + resource=Resource.create({SERVICE_NAME: "otel-test"}), + metric_readers=[in_memory_metric_reader], + ) -class TestHistogramExport(TestCase): - def test_histogram_counter_collection(self): + meter = provider.get_meter("my-meter") + histogram = meter.create_histogram("my_histogram") + counter = meter.create_counter("my_counter") histogram.record(5, {"attribute": "value"}) counter.add(1, {"attribute": "value_counter"}) From 0aafae50e7d667258446bbf6c6a85eeabe7d9a12 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 7 Sep 2023 10:56:52 +0200 Subject: [PATCH 12/14] Make return more concise --- .../sdk/metrics/_internal/_view_instrument_match.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py index c339cdb1d8f..ece723dfedc 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py @@ -136,6 +136,4 @@ def collect( ) if data_point is not None: data_points.append(data_point) - if data_points: - return data_points - return None + return data_points or None From cb97ad326bd9ad2df6ea182d86413751aeb70348 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 7 Sep 2023 11:33:35 +0200 Subject: [PATCH 13/14] Move return inside the lock --- .../_internal/metric_reader_storage.py | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py index ae57bd7d064..700ace87204 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py @@ -230,21 +230,21 @@ def collect(self) -> Optional[MetricsData]: instrument.instrumentation_scope ].metrics.extend(metrics) - if instrumentation_scope_scope_metrics: - - return MetricsData( - resource_metrics=[ - ResourceMetrics( - resource=self._sdk_config.resource, - scope_metrics=list( - instrumentation_scope_scope_metrics.values() - ), - schema_url=self._sdk_config.resource.schema_url, - ) - ] - ) + if instrumentation_scope_scope_metrics: + + return MetricsData( + resource_metrics=[ + ResourceMetrics( + resource=self._sdk_config.resource, + scope_metrics=list( + instrumentation_scope_scope_metrics.values() + ), + schema_url=self._sdk_config.resource.schema_url, + ) + ] + ) - return None + return None def _handle_view_instrument_match( self, From 58d12160be65df25fec90a21788b1798c85ca0a5 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 7 Sep 2023 20:24:14 +0200 Subject: [PATCH 14/14] Add explanatory comment --- .../sdk/metrics/_internal/_view_instrument_match.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py index ece723dfedc..110f963a486 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py @@ -136,4 +136,8 @@ def collect( ) if data_point is not None: data_points.append(data_point) + + # Returning here None instead of an empty list because the caller + # does not consume a sequence and to be consistent with the rest of + # collect methods that also return None. return data_points or None