From c058564e84a5a06aea8db0765d17fa978b8d4f5b Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 2 Dec 2024 17:11:59 +0100 Subject: [PATCH 01/41] opentelemetry-api: add advisory parameters to Histograms --- .../metrics/_internal/__init__.py | 13 +++++--- .../metrics/_internal/instrument.py | 33 +++++++++++++++++-- .../src/opentelemetry/util/types.py | 8 +++-- opentelemetry-api/tests/metrics/test_meter.py | 2 +- .../tests/metrics/test_meter_provider.py | 2 +- 5 files changed, 47 insertions(+), 11 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 84b4a6a8747..74bf635b263 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -74,7 +74,7 @@ ) from opentelemetry.util._once import Once from opentelemetry.util._providers import _load_provider -from opentelemetry.util.types import Attributes +from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory _logger = getLogger(__name__) @@ -379,6 +379,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + advisory: MetricsInstrumentAdvisory = None, ) -> Histogram: """Creates a :class:`~opentelemetry.metrics.Histogram` instrument @@ -526,13 +527,14 @@ def create_histogram( name: str, unit: str = "", description: str = "", + advisory: MetricsInstrumentAdvisory = None, ) -> Histogram: with self._lock: if self._real_meter: return self._real_meter.create_histogram( - name, unit, description + name, unit, description, advisory ) - proxy = _ProxyHistogram(name, unit, description) + proxy = _ProxyHistogram(name, unit, description, advisory) self._instruments.append(proxy) return proxy @@ -686,6 +688,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + advisory: MetricsInstrumentAdvisory = None, ) -> Histogram: """Returns a no-op Histogram.""" if self._is_instrument_registered( @@ -699,7 +702,9 @@ def create_histogram( unit, description, ) - return NoOpHistogram(name, unit=unit, description=description) + return NoOpHistogram( + name, unit=unit, description=description, advisory=advisory + ) def create_observable_gauge( self, diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 2250c8b6fdd..5d422589fa6 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -35,7 +35,7 @@ from opentelemetry import metrics from opentelemetry.context import Context from opentelemetry.metrics._internal.observation import Observation -from opentelemetry.util.types import Attributes +from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory _logger = getLogger(__name__) @@ -330,6 +330,16 @@ class Histogram(Synchronous): histograms, summaries, and percentile. """ + @abstractmethod + def __init__( + self, + name: str, + unit: str = "", + description: str = "", + advisory: MetricsInstrumentAdvisory = None, + ) -> None: + pass + @abstractmethod def record( self, @@ -348,8 +358,11 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: MetricsInstrumentAdvisory = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) def record( self, @@ -361,6 +374,20 @@ def record( class _ProxyHistogram(_ProxyInstrument[Histogram], Histogram): + # pylint: disable=super-init-not-called + def __init__( + self, + name: str, + unit: str = "", + description: str = "", + advisory: MetricsInstrumentAdvisory = None, + ) -> None: + self._name = name + self._unit = unit + self._description = description + self._advisory = advisory + self._real_instrument: Optional[InstrumentT] = None + def record( self, amount: Union[int, float], @@ -372,7 +399,7 @@ def record( def _create_real_instrument(self, meter: "metrics.Meter") -> Histogram: return meter.create_histogram( - self._name, self._unit, self._description + self._name, self._unit, self._description, self._advisory ) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index d9490ff08c9..84b7d5b87a6 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -12,8 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. - -from typing import Mapping, Optional, Sequence, Tuple, Union +from typing import Literal, Mapping, Optional, Sequence, Tuple, Union # This is the implementation of the "Any" type as specified by the specifications of OpenTelemetry data model for logs. # For more details, refer to the OTel specification: @@ -56,3 +55,8 @@ ], ..., ] + +MetricsInstrumentAdvisoryKey = Literal["ExplicitBucketBoundaries"] +MetricsInstrumentAdvisory = Optional[ + Mapping[MetricsInstrumentAdvisoryKey, AnyValue] +] diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 4fb9e047b60..356c8a6b46f 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -39,7 +39,7 @@ def create_observable_counter( name, callbacks, unit=unit, description=description ) - def create_histogram(self, name, unit="", description=""): + def create_histogram(self, name, unit="", description="", advisory=None): super().create_histogram(name, unit=unit, description=description) def create_gauge(self, name, unit="", description=""): diff --git a/opentelemetry-api/tests/metrics/test_meter_provider.py b/opentelemetry-api/tests/metrics/test_meter_provider.py index 6f1989e208a..37910def0fa 100644 --- a/opentelemetry-api/tests/metrics/test_meter_provider.py +++ b/opentelemetry-api/tests/metrics/test_meter_provider.py @@ -274,7 +274,7 @@ def test_proxy_meter(self): name, unit, description ) real_meter.create_histogram.assert_called_once_with( - name, unit, description + name, unit, description, None ) real_meter.create_gauge.assert_called_once_with( name, unit, description From c435220c0cc0ad2fe7ac17adeef0ea4eb6e9acce Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 2 Dec 2024 17:24:24 +0100 Subject: [PATCH 02/41] Update sdk --- .../sdk/metrics/_internal/__init__.py | 31 +++++++- .../sdk/metrics/_internal/aggregation.py | 70 ++++++++++--------- .../sdk/metrics/_internal/instrument.py | 19 +++++ .../tests/metrics/test_aggregation.py | 17 +++++ .../tests/metrics/test_metrics.py | 32 +++++++++ 5 files changed, 132 insertions(+), 37 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 334dd01b10a..0945d2d8206 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -11,13 +11,15 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + import weakref from atexit import register, unregister +from collections.abc import Sequence from logging import getLogger from os import environ from threading import Lock from time import time_ns -from typing import Optional, Sequence +from typing import Optional # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics @@ -63,7 +65,7 @@ from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.util.instrumentation import InstrumentationScope from opentelemetry.util._once import Once -from opentelemetry.util.types import Attributes +from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory _logger = getLogger(__name__) @@ -196,7 +198,29 @@ def create_observable_counter( self._instrument_id_instrument[instrument_id] = instrument return instrument - def create_histogram(self, name, unit="", description="") -> APIHistogram: + def create_histogram( + self, + name: str, + unit: str = "", + description: str = "", + advisory: MetricsInstrumentAdvisory = None, + ) -> APIHistogram: + if advisory is not None: + raise_error = False + try: + boundaries = advisory["ExplicitBucketBoundaries"] + raise_error = not ( + boundaries + and all(isinstance(e, (int, float)) for e in boundaries) + ) + except (KeyError, TypeError): + raise_error = True + + if raise_error: + raise ValueError( + "Advisory must be a dict with ExplicitBucketBoundaries key containing a sequence of numbers" + ) + ( is_instrument_registered, instrument_id, @@ -223,6 +247,7 @@ def create_histogram(self, name, unit="", description="") -> APIHistogram: self._measurement_consumer, unit, description, + advisory, ) with self._instrument_id_instrument_lock: self._instrument_id_instrument[instrument_id] = instrument diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index a579e3072e9..fd592c0d304 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -437,6 +437,25 @@ def collect( ) +_DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES: Sequence[float] = ( + 0.0, + 5.0, + 10.0, + 25.0, + 50.0, + 75.0, + 100.0, + 250.0, + 500.0, + 750.0, + 1000.0, + 2500.0, + 5000.0, + 7500.0, + 10000.0, +) + + class _ExplicitBucketHistogramAggregation(_Aggregation[HistogramPoint]): def __init__( self, @@ -444,25 +463,14 @@ def __init__( instrument_aggregation_temporality: AggregationTemporality, start_time_unix_nano: int, reservoir_builder: ExemplarReservoirBuilder, - boundaries: Sequence[float] = ( - 0.0, - 5.0, - 10.0, - 25.0, - 50.0, - 75.0, - 100.0, - 250.0, - 500.0, - 750.0, - 1000.0, - 2500.0, - 5000.0, - 7500.0, - 10000.0, - ), + boundaries: Optional[Sequence[float]] = None, record_min_max: bool = True, ): + if boundaries is None: + boundaries = ( + _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES + ) + super().__init__( attributes, reservoir_builder=partial( @@ -1268,6 +1276,11 @@ def _create_aggregation( ) if isinstance(instrument, Histogram): + boundaries: Optional[Sequence[float]] = ( + instrument.advisory.get("ExplicitBucketBoundaries") + if instrument.advisory is not None + else None + ) return _ExplicitBucketHistogramAggregation( attributes, reservoir_builder=reservoir_factory( @@ -1276,6 +1289,7 @@ def _create_aggregation( instrument_aggregation_temporality=( AggregationTemporality.DELTA ), + boundaries=boundaries, start_time_unix_nano=start_time_unix_nano, ) @@ -1347,25 +1361,13 @@ class ExplicitBucketHistogramAggregation(Aggregation): def __init__( self, - boundaries: Sequence[float] = ( - 0.0, - 5.0, - 10.0, - 25.0, - 50.0, - 75.0, - 100.0, - 250.0, - 500.0, - 750.0, - 1000.0, - 2500.0, - 5000.0, - 7500.0, - 10000.0, - ), + boundaries: Optional[Sequence[float]] = None, record_min_max: bool = True, ) -> None: + if boundaries is None: + boundaries = ( + _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES + ) self._boundaries = boundaries self._record_min_max = record_min_max diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index c93f83a4e62..a898f151fe6 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -34,6 +34,7 @@ from opentelemetry.metrics._internal.instrument import CallbackOptions from opentelemetry.sdk.metrics._internal.measurement import Measurement from opentelemetry.sdk.util.instrumentation import InstrumentationScope +from opentelemetry.util.types import MetricsInstrumentAdvisory _logger = getLogger(__name__) @@ -219,6 +220,24 @@ def __new__(cls, *args, **kwargs): class Histogram(_Synchronous, APIHistogram): + def __init__( + self, + name: str, + instrumentation_scope: InstrumentationScope, + measurement_consumer: "opentelemetry.sdk.metrics.MeasurementConsumer", + unit: str = "", + description: str = "", + advisory: MetricsInstrumentAdvisory = None, + ): + super().__init__( + name, + unit=unit, + description=description, + instrumentation_scope=instrumentation_scope, + measurement_consumer=measurement_consumer, + ) + self.advisory = advisory + def __new__(cls, *args, **kwargs): if cls is Histogram: raise TypeError("Histogram must be instantiated via a meter.") diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 4a625908afb..48cc6e32d33 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -15,6 +15,7 @@ # pylint: disable=protected-access from math import inf +from random import randint from time import sleep, time_ns from typing import Union from unittest import TestCase @@ -628,6 +629,22 @@ def test_histogram(self): ) self.assertIsInstance(aggregation, _ExplicitBucketHistogramAggregation) + def test_histogram_with_advisory(self): + boundaries = [randint(0, 1000)] + aggregation = self.default_aggregation._create_aggregation( + _Histogram( + "name", + Mock(), + Mock(), + advisory={"ExplicitBucketBoundaries": boundaries}, + ), + Mock(), + _default_reservoir_factory, + 0, + ) + self.assertIsInstance(aggregation, _ExplicitBucketHistogramAggregation) + self.assertEqual(aggregation._boundaries, tuple(boundaries)) + def test_gauge(self): aggregation = self.default_aggregation._create_aggregation( _Gauge( diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 4ba0c2fde85..9b2d87bb192 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -500,6 +500,38 @@ def test_create_histogram(self): self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") + def test_create_histogram_with_advisory(self): + histogram = self.meter.create_histogram( + "name", + unit="unit", + description="description", + advisory={"ExplicitBucketBoundaries": [0, 1, 2]}, + ) + + self.assertIsInstance(histogram, Histogram) + self.assertEqual(histogram.name, "name") + self.assertEqual( + histogram.advisory, {"ExplicitBucketBoundaries": [0, 1, 2]} + ) + + def test_create_histogram_advisory_validation(self): + advisories = [ + {"ExplicitBucketBoundaries": None}, + {"ExplicitBucketBoundaries": []}, + {}, + [], + {"ExplicitBucketBoundaries": [1, 2.0, "3"]}, + ] + for advisory in advisories: + with self.subTest(advisory=advisory): + with self.assertRaises(ValueError): + self.meter.create_histogram( + "name", + unit="unit", + description="description", + advisory=advisory, + ) + def test_create_observable_gauge(self): observable_gauge = self.meter.create_observable_gauge( "name", callbacks=[Mock()], unit="unit", description="description" From 3287503eb48ed60de5a1326afc7891f17c0b3a2c Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 17 Dec 2024 15:16:47 +0100 Subject: [PATCH 03/41] No need to use Sequence from collections --- .../src/opentelemetry/sdk/metrics/_internal/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 0945d2d8206..46c6606685f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -14,12 +14,11 @@ import weakref from atexit import register, unregister -from collections.abc import Sequence from logging import getLogger from os import environ from threading import Lock from time import time_ns -from typing import Optional +from typing import Optional, Sequence # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics From 1ee046e0af99980eae5d6dd98b0e3bac1062e5af Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 17 Dec 2024 15:20:12 +0100 Subject: [PATCH 04/41] Fix typing in proxy --- .../src/opentelemetry/metrics/_internal/instrument.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 5d422589fa6..3d6ea8f136b 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -386,7 +386,7 @@ def __init__( self._unit = unit self._description = description self._advisory = advisory - self._real_instrument: Optional[InstrumentT] = None + self._real_instrument: Optional[Histogram] = None def record( self, From 83fd8173815aa4cf9fa86860358452d229288e33 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Thu, 19 Dec 2024 17:58:34 +0100 Subject: [PATCH 05/41] Rewrote MetricsInstrumentAdvisory as TypedDict --- .../src/opentelemetry/metrics/_internal/__init__.py | 6 +++--- .../src/opentelemetry/metrics/_internal/instrument.py | 6 +++--- opentelemetry-api/src/opentelemetry/util/types.py | 9 ++++----- .../src/opentelemetry/sdk/metrics/_internal/__init__.py | 2 +- .../opentelemetry/sdk/metrics/_internal/instrument.py | 2 +- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 74bf635b263..23a30e547d1 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -379,7 +379,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Histogram: """Creates a :class:`~opentelemetry.metrics.Histogram` instrument @@ -527,7 +527,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Histogram: with self._lock: if self._real_meter: @@ -688,7 +688,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Histogram: """Returns a no-op Histogram.""" if self._is_instrument_registered( diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 3d6ea8f136b..1759c10229d 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -336,7 +336,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: pass @@ -358,7 +358,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: super().__init__( name, unit=unit, description=description, advisory=advisory @@ -380,7 +380,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: self._name = name self._unit = unit diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index 84b7d5b87a6..b055d3e5584 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Literal, Mapping, Optional, Sequence, Tuple, Union +from typing import Mapping, Optional, Sequence, Tuple, TypedDict, Union # This is the implementation of the "Any" type as specified by the specifications of OpenTelemetry data model for logs. # For more details, refer to the OTel specification: @@ -56,7 +56,6 @@ ..., ] -MetricsInstrumentAdvisoryKey = Literal["ExplicitBucketBoundaries"] -MetricsInstrumentAdvisory = Optional[ - Mapping[MetricsInstrumentAdvisoryKey, AnyValue] -] + +class MetricsInstrumentAdvisory(TypedDict): + ExplicitBucketBoundaries: Optional[AnyValue] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 46c6606685f..20daf31ebe4 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -202,7 +202,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> APIHistogram: if advisory is not None: raise_error = False diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index a898f151fe6..51b3d3aa02b 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -227,7 +227,7 @@ def __init__( measurement_consumer: "opentelemetry.sdk.metrics.MeasurementConsumer", unit: str = "", description: str = "", - advisory: MetricsInstrumentAdvisory = None, + advisory: Optional[MetricsInstrumentAdvisory] = None, ): super().__init__( name, From e0330830ab8115137d4b2dafb5c923e2ead344f2 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 23 Dec 2024 15:24:10 +0100 Subject: [PATCH 06/41] Rename ExplicitBucketBoundaries to explicit_bucket_boundaries --- opentelemetry-api/src/opentelemetry/util/types.py | 2 +- .../opentelemetry/sdk/metrics/_internal/__init__.py | 4 ++-- .../opentelemetry/sdk/metrics/_internal/aggregation.py | 2 +- opentelemetry-sdk/tests/metrics/test_aggregation.py | 2 +- opentelemetry-sdk/tests/metrics/test_metrics.py | 10 +++++----- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index b055d3e5584..ad3799f4958 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -58,4 +58,4 @@ class MetricsInstrumentAdvisory(TypedDict): - ExplicitBucketBoundaries: Optional[AnyValue] + explicit_bucket_boundaries: Optional[AnyValue] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 20daf31ebe4..1d5511a6aee 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -207,7 +207,7 @@ def create_histogram( if advisory is not None: raise_error = False try: - boundaries = advisory["ExplicitBucketBoundaries"] + boundaries = advisory["explicit_bucket_boundaries"] raise_error = not ( boundaries and all(isinstance(e, (int, float)) for e in boundaries) @@ -217,7 +217,7 @@ def create_histogram( if raise_error: raise ValueError( - "Advisory must be a dict with ExplicitBucketBoundaries key containing a sequence of numbers" + "Advisory must be a dict with explicit_bucket_boundaries key containing a sequence of numbers" ) ( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index fd592c0d304..e8e07cb0fdf 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1277,7 +1277,7 @@ def _create_aggregation( if isinstance(instrument, Histogram): boundaries: Optional[Sequence[float]] = ( - instrument.advisory.get("ExplicitBucketBoundaries") + instrument.advisory.get("explicit_bucket_boundaries") if instrument.advisory is not None else None ) diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 48cc6e32d33..29a31e3eb95 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -636,7 +636,7 @@ def test_histogram_with_advisory(self): "name", Mock(), Mock(), - advisory={"ExplicitBucketBoundaries": boundaries}, + advisory={"explicit_bucket_boundaries": boundaries}, ), Mock(), _default_reservoir_factory, diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 9b2d87bb192..0910b414a0a 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -505,22 +505,22 @@ def test_create_histogram_with_advisory(self): "name", unit="unit", description="description", - advisory={"ExplicitBucketBoundaries": [0, 1, 2]}, + advisory={"explicit_bucket_boundaries": [0, 1, 2]}, ) self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( - histogram.advisory, {"ExplicitBucketBoundaries": [0, 1, 2]} + histogram.advisory, {"explicit_bucket_boundaries": [0, 1, 2]} ) def test_create_histogram_advisory_validation(self): advisories = [ - {"ExplicitBucketBoundaries": None}, - {"ExplicitBucketBoundaries": []}, + {"explicit_bucket_boundaries": None}, + {"explicit_bucket_boundaries": []}, {}, [], - {"ExplicitBucketBoundaries": [1, 2.0, "3"]}, + {"explicit_bucket_boundaries": [1, 2.0, "3"]}, ] for advisory in advisories: with self.subTest(advisory=advisory): From 1c9071848fb382a59d2331d06dcdb35afe4a760b Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 21 Jan 2025 14:53:18 +0100 Subject: [PATCH 07/41] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b79455a7f..0f4c00c6770 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [BREAKING] semantic-conventions: Remove `opentelemetry.semconv.attributes.network_attributes.NETWORK_INTERFACE_NAME` introduced by mistake in the wrong module. ([#4391](https://github.com/open-telemetry/opentelemetry-python/pull/4391)) +- Add support for `explicit_bucket_boundaries` advisory for Histograms + ([#4361](https://github.com/open-telemetry/opentelemetry-python/pull/4361)) ## Version 1.29.0/0.50b0 (2024-12-11) From 811459738e5a434cbf34fef32d9b8f6c7bc9f3dd Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 23 Dec 2024 16:58:38 +0100 Subject: [PATCH 08/41] Add an example in docs --- docs/examples/metrics/instruments/example.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index fde20308a21..f2c86bbd60a 100644 --- a/docs/examples/metrics/instruments/example.py +++ b/docs/examples/metrics/instruments/example.py @@ -58,5 +58,13 @@ def observable_gauge_func(options: CallbackOptions) -> Iterable[Observation]: histogram = meter.create_histogram("histogram") histogram.record(99.9) + +# Histogram with explicit_bucket_boundaries advisory +histogram = meter.create_histogram( + "histogram_with_advisory", + advisory={"explicit_bucket_boundaries": [0, 1, 2]}, +) +histogram.record(99.9) + # Async Gauge gauge = meter.create_observable_gauge("gauge", [observable_gauge_func]) From 05f974ab384ee4996cae0f6f6b967996f26bc45b Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 15 Jan 2025 17:14:59 +0100 Subject: [PATCH 09/41] parameter validation should just report an error, not fail --- .../opentelemetry/sdk/metrics/_internal/__init__.py | 11 ++++++----- opentelemetry-sdk/tests/metrics/test_metrics.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 1d5511a6aee..40c9dca8e54 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -205,18 +205,19 @@ def create_histogram( advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> APIHistogram: if advisory is not None: - raise_error = False + invalid_advisory = False try: boundaries = advisory["explicit_bucket_boundaries"] - raise_error = not ( + invalid_advisory = not ( boundaries and all(isinstance(e, (int, float)) for e in boundaries) ) except (KeyError, TypeError): - raise_error = True + invalid_advisory = True - if raise_error: - raise ValueError( + if invalid_advisory: + advisory = None + _logger.warning( "Advisory must be a dict with explicit_bucket_boundaries key containing a sequence of numbers" ) diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 0910b414a0a..21b7937614b 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -524,7 +524,7 @@ def test_create_histogram_advisory_validation(self): ] for advisory in advisories: with self.subTest(advisory=advisory): - with self.assertRaises(ValueError): + with self.assertLogs(level=WARNING): self.meter.create_histogram( "name", unit="unit", From 1012547258b58d6ce098d47e46fea19223142146 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 15 Jan 2025 17:19:02 +0100 Subject: [PATCH 10/41] Make test reproducible --- opentelemetry-sdk/tests/metrics/test_aggregation.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 29a31e3eb95..527d4bf80eb 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -15,7 +15,6 @@ # pylint: disable=protected-access from math import inf -from random import randint from time import sleep, time_ns from typing import Union from unittest import TestCase @@ -630,7 +629,7 @@ def test_histogram(self): self.assertIsInstance(aggregation, _ExplicitBucketHistogramAggregation) def test_histogram_with_advisory(self): - boundaries = [randint(0, 1000)] + boundaries = [1.0, 2.0, 3.0] aggregation = self.default_aggregation._create_aggregation( _Histogram( "name", From 3ebe6dfd0fcab3839577e9aebbb391511f98122a Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 15 Jan 2025 17:21:38 +0100 Subject: [PATCH 11/41] Fix type of MetricsInstrumentAdvisory.explicit_bucket_boundaries --- opentelemetry-api/src/opentelemetry/util/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index ad3799f4958..7b7d5270437 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -58,4 +58,4 @@ class MetricsInstrumentAdvisory(TypedDict): - explicit_bucket_boundaries: Optional[AnyValue] + explicit_bucket_boundaries: Optional[Union[Sequence[int], Sequence[float]]] From f7af6f4c2a36cf5f6901eca98af7703151ba638d Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 15 Jan 2025 17:27:46 +0100 Subject: [PATCH 12/41] Make advisory attribute private --- .../src/opentelemetry/sdk/metrics/_internal/aggregation.py | 4 ++-- .../src/opentelemetry/sdk/metrics/_internal/instrument.py | 2 +- opentelemetry-sdk/tests/metrics/test_metrics.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index e8e07cb0fdf..db4d73569ce 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1277,8 +1277,8 @@ def _create_aggregation( if isinstance(instrument, Histogram): boundaries: Optional[Sequence[float]] = ( - instrument.advisory.get("explicit_bucket_boundaries") - if instrument.advisory is not None + instrument._advisory.get("explicit_bucket_boundaries") + if instrument._advisory is not None else None ) return _ExplicitBucketHistogramAggregation( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index 51b3d3aa02b..6e2c62ce222 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -236,7 +236,7 @@ def __init__( instrumentation_scope=instrumentation_scope, measurement_consumer=measurement_consumer, ) - self.advisory = advisory + self._advisory = advisory def __new__(cls, *args, **kwargs): if cls is Histogram: diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 21b7937614b..9274b45e01d 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -511,7 +511,7 @@ def test_create_histogram_with_advisory(self): self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( - histogram.advisory, {"explicit_bucket_boundaries": [0, 1, 2]} + histogram._advisory, {"explicit_bucket_boundaries": [0, 1, 2]} ) def test_create_histogram_advisory_validation(self): From 58d24cf25719efe7b9d7dc6c64788e0ce0cf5851 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 15 Jan 2025 17:31:44 +0100 Subject: [PATCH 13/41] Permit also a sequence of ints as boundaries --- .../sdk/metrics/_internal/aggregation.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index db4d73569ce..84c1a69ffda 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -21,7 +21,16 @@ from logging import getLogger from math import inf from threading import Lock -from typing import Callable, Generic, List, Optional, Sequence, Type, TypeVar +from typing import ( + Callable, + Generic, + List, + Optional, + Sequence, + Type, + TypeVar, + Union, +) from opentelemetry.metrics import ( Asynchronous, @@ -463,7 +472,7 @@ def __init__( instrument_aggregation_temporality: AggregationTemporality, start_time_unix_nano: int, reservoir_builder: ExemplarReservoirBuilder, - boundaries: Optional[Sequence[float]] = None, + boundaries: Optional[Union[Sequence[float], Sequence[int]]] = None, record_min_max: bool = True, ): if boundaries is None: @@ -1276,7 +1285,7 @@ def _create_aggregation( ) if isinstance(instrument, Histogram): - boundaries: Optional[Sequence[float]] = ( + boundaries: Optional[Union[Sequence[float], Sequence[int]]] = ( instrument._advisory.get("explicit_bucket_boundaries") if instrument._advisory is not None else None @@ -1361,7 +1370,7 @@ class ExplicitBucketHistogramAggregation(Aggregation): def __init__( self, - boundaries: Optional[Sequence[float]] = None, + boundaries: Optional[Union[Sequence[float], Sequence[int]]] = None, record_min_max: bool = True, ) -> None: if boundaries is None: From 2e6be0bf407f2705884204b9784b1215543a3da7 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Thu, 16 Jan 2025 14:40:45 +0100 Subject: [PATCH 14/41] Rework conflicting instrument registration check First to take into account advisory and then to avoid printing warnings if there's not duplicate for already registered instrument. --- .../metrics/_internal/__init__.py | 156 +++++++++++++----- opentelemetry-api/tests/metrics/test_meter.py | 41 ++++- .../sdk/metrics/_internal/__init__.py | 107 +++++++----- .../tests/metrics/test_metrics.py | 48 +++++- 4 files changed, 261 insertions(+), 91 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 23a30e547d1..9e143ed6adc 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -42,10 +42,11 @@ import warnings from abc import ABC, abstractmethod +from dataclasses import dataclass from logging import getLogger from os import environ from threading import Lock -from typing import List, Optional, Sequence, Set, Tuple, Union, cast +from typing import Dict, List, Optional, Sequence, Tuple, Union, cast from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER from opentelemetry.metrics._internal.instrument import ( @@ -177,6 +178,13 @@ def on_set_meter_provider(self, meter_provider: MeterProvider) -> None: meter.on_set_meter_provider(meter_provider) +@dataclass +class _InstrumentRegistrationStatus: + already_registered: bool + conflict: bool + current_advisory: Optional[MetricsInstrumentAdvisory] + + class Meter(ABC): """Handles instrument creation. @@ -194,7 +202,7 @@ def __init__( self._name = name self._version = version self._schema_url = schema_url - self._instrument_ids: Set[str] = set() + self._instrument_ids: Dict[str, str] = {} self._instrument_ids_lock = Lock() @property @@ -218,31 +226,50 @@ def schema_url(self) -> Optional[str]: """ return self._schema_url - def _is_instrument_registered( - self, name: str, type_: type, unit: str, description: str - ) -> Tuple[bool, str]: + def _register_instrument( + self, + name: str, + type_: type, + unit: str, + description: str, + advisory: Optional[MetricsInstrumentAdvisory] = None, + ) -> Tuple[str, _InstrumentRegistrationStatus]: """ - Check if an instrument with the same name, type, unit and description - has been registered already. - - Returns a tuple. The first value is `True` if the instrument has been - registered already, `False` otherwise. The second value is the - instrument id. + Register an instrument with the name, type, unit and description as + identifying keys and the advisory as value. + + Returns a tuple. The first value is the instrument id. + The second value is an `_InstrumentRegistrationStatus` where + `already_registered` is `True` if the instrument has been registered + already. + If `conflict` is set to True the `current_advisory` attribute contains + the registered instrument advisory. """ instrument_id = ",".join( [name.strip().lower(), type_.__name__, unit, description] ) - result = False + already_registered, conflict = False, False + current_advisory = None with self._instrument_ids_lock: - if instrument_id in self._instrument_ids: - result = True + # we are not using get because None is a valid value + already_registered = instrument_id in self._instrument_ids + if already_registered: + current_advisory = self._instrument_ids[instrument_id] + conflict = current_advisory != advisory else: - self._instrument_ids.add(instrument_id) - - return (result, instrument_id) + self._instrument_ids[instrument_id] = advisory + + return ( + instrument_id, + _InstrumentRegistrationStatus( + already_registered=already_registered, + conflict=conflict, + current_advisory=current_advisory, + ), + ) @abstractmethod def create_counter( @@ -250,6 +277,7 @@ def create_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Counter: """Creates a `Counter` instrument @@ -266,6 +294,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> UpDownCounter: """Creates an `UpDownCounter` instrument @@ -283,6 +312,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableCounter: """Creates an `ObservableCounter` instrument @@ -395,6 +425,7 @@ def create_gauge( # type: ignore # pylint: disable=no-self-use name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Gauge: """Creates a ``Gauge`` instrument @@ -413,6 +444,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableGauge: """Creates an `ObservableGauge` instrument @@ -433,6 +465,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableUpDownCounter: """Creates an `ObservableUpDownCounter` instrument @@ -481,6 +514,7 @@ def create_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Counter: with self._lock: if self._real_meter: @@ -494,6 +528,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> UpDownCounter: with self._lock: if self._real_meter: @@ -510,6 +545,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableCounter: with self._lock: if self._real_meter: @@ -543,6 +579,7 @@ def create_gauge( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Gauge: with self._lock: if self._real_meter: @@ -557,6 +594,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableGauge: with self._lock: if self._real_meter: @@ -575,6 +613,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableUpDownCounter: with self._lock: if self._real_meter: @@ -602,18 +641,22 @@ def create_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Counter: """Returns a no-op Counter.""" - if self._is_instrument_registered( - name, NoOpCounter, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpCounter, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, Counter.__name__, unit, description, + status.current_advisory, ) return NoOpCounter(name, unit=unit, description=description) @@ -622,18 +665,22 @@ def create_gauge( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Gauge: """Returns a no-op Gauge.""" - if self._is_instrument_registered(name, NoOpGauge, unit, description)[ - 0 - ]: + _, status = self._register_instrument( + name, NoOpGauge, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, Gauge.__name__, unit, description, + status.current_advisory, ) return NoOpGauge(name, unit=unit, description=description) @@ -642,18 +689,22 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" - if self._is_instrument_registered( - name, NoOpUpDownCounter, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpUpDownCounter, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, UpDownCounter.__name__, unit, description, + status.current_advisory, ) return NoOpUpDownCounter(name, unit=unit, description=description) @@ -663,18 +714,22 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" - if self._is_instrument_registered( - name, NoOpObservableCounter, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpObservableCounter, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, ObservableCounter.__name__, unit, description, + status.current_advisory, ) return NoOpObservableCounter( name, @@ -691,16 +746,19 @@ def create_histogram( advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> Histogram: """Returns a no-op Histogram.""" - if self._is_instrument_registered( - name, NoOpHistogram, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpHistogram, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, Histogram.__name__, unit, description, + status.current_advisory, ) return NoOpHistogram( name, unit=unit, description=description, advisory=advisory @@ -712,18 +770,22 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" - if self._is_instrument_registered( - name, NoOpObservableGauge, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpObservableGauge, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, ObservableGauge.__name__, unit, description, + status.current_advisory, ) return NoOpObservableGauge( name, @@ -738,18 +800,22 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" - if self._is_instrument_registered( - name, NoOpObservableUpDownCounter, unit, description - )[0]: + _, status = self._register_instrument( + name, NoOpObservableUpDownCounter, unit, description, advisory + ) + if status.conflict: _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, ObservableUpDownCounter.__name__, unit, description, + status.current_advisory, ) return NoOpObservableUpDownCounter( name, diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 356c8a6b46f..4c9cdb6c140 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -84,7 +84,7 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertLogs(level=WARNING): + with self.assertNoLogs(level=WARNING): getattr(test_meter, f"create_{instrument_name}")( instrument_name ) @@ -94,11 +94,48 @@ def test_repeated_instrument_names(self): "observable_gauge", "observable_up_down_counter", ]: - with self.assertLogs(level=WARNING): + with self.assertNoLogs(level=WARNING): getattr(test_meter, f"create_{instrument_name}")( instrument_name, Mock() ) + def test_repeated_instrument_names_with_different_advisory(self): + try: + test_meter = NoOpMeter("name") + + test_meter.create_counter("counter") + test_meter.create_up_down_counter("up_down_counter") + test_meter.create_observable_counter("observable_counter", Mock()) + test_meter.create_histogram("histogram") + test_meter.create_gauge("gauge") + test_meter.create_observable_gauge("observable_gauge", Mock()) + test_meter.create_observable_up_down_counter( + "observable_up_down_counter", Mock() + ) + except Exception as error: # pylint: disable=broad-exception-caught + self.fail(f"Unexpected exception raised {error}") + + for instrument_name in [ + "counter", + "up_down_counter", + "histogram", + "gauge", + ]: + with self.assertLogs(level=WARNING): + getattr(test_meter, f"create_{instrument_name}")( + instrument_name, advisory=Mock() + ) + + for instrument_name in [ + "observable_counter", + "observable_gauge", + "observable_up_down_counter", + ]: + with self.assertLogs(level=WARNING): + getattr(test_meter, f"create_{instrument_name}")( + instrument_name, Mock(), advisory=Mock() + ) + def test_create_counter(self): """ Test that the meter provides a function to create a new Counter diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 40c9dca8e54..d22d40e71da 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -87,24 +87,31 @@ def __init__( self._instrument_id_instrument = {} self._instrument_id_instrument_lock = Lock() - def create_counter(self, name, unit="", description="") -> APICounter: + def create_counter( + self, name, unit="", description="", advisory=None + ) -> APICounter: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered(name, _Counter, unit, description) + status, + ) = self._register_instrument( + name, _Counter, unit, description, advisory + ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APICounter.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -121,27 +128,30 @@ def create_counter(self, name, unit="", description="") -> APICounter: return instrument def create_up_down_counter( - self, name, unit="", description="" + self, name, unit="", description="", advisory=None ) -> APIUpDownCounter: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered( - name, _UpDownCounter, unit, description + status, + ) = self._register_instrument( + name, _UpDownCounter, unit, description, advisory ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIUpDownCounter.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -158,27 +168,30 @@ def create_up_down_counter( return instrument def create_observable_counter( - self, name, callbacks=None, unit="", description="" + self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableCounter: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered( - name, _ObservableCounter, unit, description + status, + ) = self._register_instrument( + name, _ObservableCounter, unit, description, advisory ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIObservableCounter.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -222,22 +235,27 @@ def create_histogram( ) ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered(name, _Histogram, unit, description) + status, + ) = self._register_instrument( + name, _Histogram, unit, description, advisory + ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIHistogram.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -253,24 +271,31 @@ def create_histogram( self._instrument_id_instrument[instrument_id] = instrument return instrument - def create_gauge(self, name, unit="", description="") -> APIGauge: + def create_gauge( + self, name, unit="", description="", advisory=None + ) -> APIGauge: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered(name, _Gauge, unit, description) + status, + ) = self._register_instrument( + name, _Gauge, unit, description, advisory + ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIGauge.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -287,27 +312,30 @@ def create_gauge(self, name, unit="", description="") -> APIGauge: return instrument def create_observable_gauge( - self, name, callbacks=None, unit="", description="" + self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableGauge: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered( - name, _ObservableGauge, unit, description + status, + ) = self._register_instrument( + name, _ObservableGauge, unit, description, advisory ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIObservableGauge.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] @@ -327,27 +355,30 @@ def create_observable_gauge( return instrument def create_observable_up_down_counter( - self, name, callbacks=None, unit="", description="" + self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableUpDownCounter: ( - is_instrument_registered, instrument_id, - ) = self._is_instrument_registered( - name, _ObservableUpDownCounter, unit, description + status, + ) = self._register_instrument( + name, _ObservableUpDownCounter, unit, description, advisory ) - if is_instrument_registered: + if status.conflict: # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. _logger.warning( "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + "description %s has been created already with a " + "different advisory value %s.", name, APIObservableUpDownCounter.__name__, unit, description, + status.current_advisory, ) + if status.already_registered: with self._instrument_id_instrument_lock: return self._instrument_id_instrument[instrument_id] diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 9274b45e01d..ccae0dfa1a0 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -453,7 +453,7 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertLogs(level=WARNING): + with self.assertNoLogs(level=WARNING): getattr(self.meter, f"create_{instrument_name}")( instrument_name ) @@ -463,11 +463,48 @@ def test_repeated_instrument_names(self): "observable_gauge", "observable_up_down_counter", ]: - with self.assertLogs(level=WARNING): + with self.assertNoLogs(level=WARNING): getattr(self.meter, f"create_{instrument_name}")( instrument_name, callbacks=[Mock()] ) + def test_repeated_instrument_names_with_different_advisory(self): + with self.assertNotRaises(Exception): + self.meter.create_counter("counter") + self.meter.create_up_down_counter("up_down_counter") + self.meter.create_observable_counter( + "observable_counter", callbacks=[Mock()] + ) + self.meter.create_histogram("histogram") + self.meter.create_gauge("gauge") + self.meter.create_observable_gauge( + "observable_gauge", callbacks=[Mock()] + ) + self.meter.create_observable_up_down_counter( + "observable_up_down_counter", callbacks=[Mock()] + ) + + for instrument_name in [ + "counter", + "up_down_counter", + "histogram", + "gauge", + ]: + with self.assertLogs(level=WARNING): + getattr(self.meter, f"create_{instrument_name}")( + instrument_name, advisory=Mock() + ) + + for instrument_name in [ + "observable_counter", + "observable_gauge", + "observable_up_down_counter", + ]: + with self.assertLogs(level=WARNING): + getattr(self.meter, f"create_{instrument_name}")( + instrument_name, callbacks=[Mock()], advisory=Mock() + ) + def test_create_counter(self): counter = self.meter.create_counter( "name", unit="unit", description="description" @@ -621,10 +658,9 @@ def test_duplicate_instrument_aggregate_data(self): counter_0_0 = meter_0.create_counter( "counter", unit="unit", description="description" ) - with self.assertLogs(level=WARNING): - counter_0_1 = meter_0.create_counter( - "counter", unit="unit", description="description" - ) + counter_0_1 = meter_0.create_counter( + "counter", unit="unit", description="description" + ) counter_1_0 = meter_1.create_counter( "counter", unit="unit", description="description" ) From 2c3608d8c3fb0f3836b06521b5731c8b8583eff2 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Thu, 16 Jan 2025 15:53:39 +0100 Subject: [PATCH 15/41] Fix typing for api instrumentors --- .../metrics/_internal/__init__.py | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 9e143ed6adc..aea892ef9b9 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -202,7 +202,9 @@ def __init__( self._name = name self._version = version self._schema_url = schema_url - self._instrument_ids: Dict[str, str] = {} + self._instrument_ids: Dict[ + str, Optional[MetricsInstrumentAdvisory] + ] = {} self._instrument_ids_lock = Lock() @property @@ -277,7 +279,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Counter: """Creates a `Counter` instrument @@ -294,7 +296,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> UpDownCounter: """Creates an `UpDownCounter` instrument @@ -312,7 +314,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableCounter: """Creates an `ObservableCounter` instrument @@ -425,7 +427,7 @@ def create_gauge( # type: ignore # pylint: disable=no-self-use name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Gauge: """Creates a ``Gauge`` instrument @@ -444,7 +446,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableGauge: """Creates an `ObservableGauge` instrument @@ -465,7 +467,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableUpDownCounter: """Creates an `ObservableUpDownCounter` instrument @@ -514,7 +516,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Counter: with self._lock: if self._real_meter: @@ -528,7 +530,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> UpDownCounter: with self._lock: if self._real_meter: @@ -545,7 +547,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableCounter: with self._lock: if self._real_meter: @@ -579,7 +581,7 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Gauge: with self._lock: if self._real_meter: @@ -594,7 +596,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableGauge: with self._lock: if self._real_meter: @@ -613,7 +615,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableUpDownCounter: with self._lock: if self._real_meter: @@ -641,7 +643,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Counter: """Returns a no-op Counter.""" _, status = self._register_instrument( @@ -665,7 +667,7 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> Gauge: """Returns a no-op Gauge.""" _, status = self._register_instrument( @@ -689,7 +691,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" _, status = self._register_instrument( @@ -714,7 +716,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" _, status = self._register_instrument( @@ -770,7 +772,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" _, status = self._register_instrument( @@ -800,7 +802,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: None = None, ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" _, status = self._register_instrument( From c99664727e2d9489a769332dfdcc66781360fa8f Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Thu, 16 Jan 2025 16:41:59 +0100 Subject: [PATCH 16/41] Fixup lint --- opentelemetry-api/tests/metrics/test_meter.py | 46 +++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 4c9cdb6c140..e74ecbaf87c 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -24,39 +24,59 @@ class ChildMeter(Meter): # pylint: disable=signature-differs - def create_counter(self, name, unit="", description=""): - super().create_counter(name, unit=unit, description=description) + def create_counter(self, name, unit="", description="", advisory=None): + super().create_counter( + name, unit=unit, description=description, advisory=advisory + ) - def create_up_down_counter(self, name, unit="", description=""): + def create_up_down_counter( + self, name, unit="", description="", advisory=None + ): super().create_up_down_counter( - name, unit=unit, description=description + name, unit=unit, description=description, advisory=advisory ) def create_observable_counter( - self, name, callbacks, unit="", description="" + self, name, callbacks, unit="", description="", advisory=None ): super().create_observable_counter( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, ) def create_histogram(self, name, unit="", description="", advisory=None): - super().create_histogram(name, unit=unit, description=description) + super().create_histogram( + name, unit=unit, description=description, advisory=advisory + ) - def create_gauge(self, name, unit="", description=""): - super().create_gauge(name, unit=unit, description=description) + def create_gauge(self, name, unit="", description="", advisory=None): + super().create_gauge( + name, unit=unit, description=description, advisory=advisory + ) def create_observable_gauge( - self, name, callbacks, unit="", description="" + self, name, callbacks, unit="", description="", advisory=None ): super().create_observable_gauge( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, ) def create_observable_up_down_counter( - self, name, callbacks, unit="", description="" + self, name, callbacks, unit="", description="", advisory=None ): super().create_observable_up_down_counter( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, ) From dbf8a04f9a83b9eb47fd9dfc263cc86432de1862 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 10:38:17 +0100 Subject: [PATCH 17/41] Add integration tests --- ...est_histogram_advisory_explicit_buckets.py | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py new file mode 100644 index 00000000000..9b0b6409d7f --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -0,0 +1,104 @@ +# 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.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import InMemoryMetricReader +from opentelemetry.sdk.metrics.view import ( + ExplicitBucketHistogramAggregation, + View, +) + + +class TestHistogramAdvisory(TestCase): + def test_default(self): + reader = InMemoryMetricReader() + meter_provider = MeterProvider( + metric_readers=[reader], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + ) + histogram.record(1, {"label": "value"}) + histogram.record(2, {"label": "value"}) + histogram.record(3, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric.name, "testhistogram") + self.assertEqual(metric.data.data_points[0].explicit_bounds, (1, 2, 3)) + + def test_view_default_aggregation(self): + reader = InMemoryMetricReader() + view = View(instrument_name="testhistogram") + meter_provider = MeterProvider( + metric_readers=[reader], + views=[view], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + ) + histogram.record(1, {"label": "value"}) + histogram.record(2, {"label": "value"}) + histogram.record(3, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric.name, "testhistogram") + self.assertEqual(metric.data.data_points[0].explicit_bounds, (1, 2, 3)) + + def test_view_overrides_buckets(self): + reader = InMemoryMetricReader() + view = View( + instrument_name="testhistogram", + aggregation=ExplicitBucketHistogramAggregation( + boundaries=[10, 100, 1000] + ), + ) + meter_provider = MeterProvider( + metric_readers=[reader], + views=[view], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + ) + histogram.record(1, {"label": "value"}) + histogram.record(2, {"label": "value"}) + histogram.record(3, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric.name, "testhistogram") + self.assertEqual( + metric.data.data_points[0].explicit_bounds, (10, 100, 1000) + ) From f65939474c0dfed197dc1a377c3d2a10cd68ab10 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 11:10:20 +0100 Subject: [PATCH 18/41] Make explicit boundaries float only --- docs/examples/metrics/instruments/example.py | 2 +- .../src/opentelemetry/util/types.py | 2 +- .../sdk/metrics/_internal/__init__.py | 2 +- .../sdk/metrics/_internal/aggregation.py | 8 +++---- ...est_histogram_advisory_explicit_buckets.py | 21 ++++++++++++------- .../tests/metrics/test_metrics.py | 8 ++++--- 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index f2c86bbd60a..7d40d7f3c61 100644 --- a/docs/examples/metrics/instruments/example.py +++ b/docs/examples/metrics/instruments/example.py @@ -62,7 +62,7 @@ def observable_gauge_func(options: CallbackOptions) -> Iterable[Observation]: # Histogram with explicit_bucket_boundaries advisory histogram = meter.create_histogram( "histogram_with_advisory", - advisory={"explicit_bucket_boundaries": [0, 1, 2]}, + advisory={"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, ) histogram.record(99.9) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index 7b7d5270437..7bf11ad3f95 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -58,4 +58,4 @@ class MetricsInstrumentAdvisory(TypedDict): - explicit_bucket_boundaries: Optional[Union[Sequence[int], Sequence[float]]] + explicit_bucket_boundaries: Optional[Sequence[float]] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index d22d40e71da..70e480dba57 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -223,7 +223,7 @@ def create_histogram( boundaries = advisory["explicit_bucket_boundaries"] invalid_advisory = not ( boundaries - and all(isinstance(e, (int, float)) for e in boundaries) + and all(isinstance(e, float) for e in boundaries) ) except (KeyError, TypeError): invalid_advisory = True diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 84c1a69ffda..4b67a16ec9b 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -29,7 +29,6 @@ Sequence, Type, TypeVar, - Union, ) from opentelemetry.metrics import ( @@ -472,14 +471,13 @@ def __init__( instrument_aggregation_temporality: AggregationTemporality, start_time_unix_nano: int, reservoir_builder: ExemplarReservoirBuilder, - boundaries: Optional[Union[Sequence[float], Sequence[int]]] = None, + boundaries: Optional[Sequence[float]] = None, record_min_max: bool = True, ): if boundaries is None: boundaries = ( _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES ) - super().__init__( attributes, reservoir_builder=partial( @@ -1285,7 +1283,7 @@ def _create_aggregation( ) if isinstance(instrument, Histogram): - boundaries: Optional[Union[Sequence[float], Sequence[int]]] = ( + boundaries: Optional[Sequence[float]] = ( instrument._advisory.get("explicit_bucket_boundaries") if instrument._advisory is not None else None @@ -1370,7 +1368,7 @@ class ExplicitBucketHistogramAggregation(Aggregation): def __init__( self, - boundaries: Optional[Union[Sequence[float], Sequence[int]]] = None, + boundaries: Optional[Sequence[float]] = None, record_min_max: bool = True, ) -> None: if boundaries is None: diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 9b0b6409d7f..2dc78448ba3 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -30,7 +30,8 @@ def test_default(self): ) meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( - "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + "testhistogram", + advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -44,7 +45,9 @@ def test_default(self): ) metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] self.assertEqual(metric.name, "testhistogram") - self.assertEqual(metric.data.data_points[0].explicit_bounds, (1, 2, 3)) + self.assertEqual( + metric.data.data_points[0].explicit_bounds, (1.0, 2.0, 3.0) + ) def test_view_default_aggregation(self): reader = InMemoryMetricReader() @@ -55,7 +58,8 @@ def test_view_default_aggregation(self): ) meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( - "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + "testhistogram", + advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -69,14 +73,16 @@ def test_view_default_aggregation(self): ) metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] self.assertEqual(metric.name, "testhistogram") - self.assertEqual(metric.data.data_points[0].explicit_bounds, (1, 2, 3)) + self.assertEqual( + metric.data.data_points[0].explicit_bounds, (1.0, 2.0, 3.0) + ) def test_view_overrides_buckets(self): reader = InMemoryMetricReader() view = View( instrument_name="testhistogram", aggregation=ExplicitBucketHistogramAggregation( - boundaries=[10, 100, 1000] + boundaries=[10.0, 100.0, 1000.0] ), ) meter_provider = MeterProvider( @@ -85,7 +91,8 @@ def test_view_overrides_buckets(self): ) meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( - "testhistogram", advisory={"explicit_bucket_boundaries": [1, 2, 3]} + "testhistogram", + advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -100,5 +107,5 @@ def test_view_overrides_buckets(self): metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] self.assertEqual(metric.name, "testhistogram") self.assertEqual( - metric.data.data_points[0].explicit_bounds, (10, 100, 1000) + metric.data.data_points[0].explicit_bounds, (10.0, 100.0, 1000.0) ) diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index ccae0dfa1a0..fb84780422f 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -542,13 +542,14 @@ def test_create_histogram_with_advisory(self): "name", unit="unit", description="description", - advisory={"explicit_bucket_boundaries": [0, 1, 2]}, + advisory={"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, ) self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( - histogram._advisory, {"explicit_bucket_boundaries": [0, 1, 2]} + histogram._advisory, + {"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, ) def test_create_histogram_advisory_validation(self): @@ -557,7 +558,8 @@ def test_create_histogram_advisory_validation(self): {"explicit_bucket_boundaries": []}, {}, [], - {"explicit_bucket_boundaries": [1, 2.0, "3"]}, + {"explicit_bucket_boundaries": [1]}, + {"explicit_bucket_boundaries": ["1"]}, ] for advisory in advisories: with self.subTest(advisory=advisory): From 7f3f39a1c2ec560dcf7d252acab8cb6ce47f6bfb Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 11:39:10 +0100 Subject: [PATCH 19/41] Stop using assertNoLogs to have green tests on py < 3.10 --- opentelemetry-api/tests/metrics/test_meter.py | 28 ++++++++----------- .../tests/metrics/test_metrics.py | 18 ++++++------ 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index e74ecbaf87c..9fe06223e59 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -15,7 +15,7 @@ from logging import WARNING from unittest import TestCase -from unittest.mock import Mock +from unittest.mock import Mock, patch from opentelemetry.metrics import Meter, NoOpMeter @@ -82,8 +82,10 @@ def create_observable_up_down_counter( class TestMeter(TestCase): # pylint: disable=no-member - def test_repeated_instrument_names(self): - try: + # TODO: convert to assertNoLogs instead of mocking logger when 3.10 is baseline + @patch("opentelemetry.metrics._internal._logger") + def test_repeated_instrument_names(self, logger_mock): + with self.assertNotRaises(Exception): test_meter = NoOpMeter("name") test_meter.create_counter("counter") @@ -95,8 +97,6 @@ def test_repeated_instrument_names(self): test_meter.create_observable_up_down_counter( "observable_up_down_counter", Mock() ) - except Exception as error: # pylint: disable=broad-exception-caught - self.fail(f"Unexpected exception raised {error}") for instrument_name in [ "counter", @@ -104,23 +104,21 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertNoLogs(level=WARNING): - getattr(test_meter, f"create_{instrument_name}")( - instrument_name - ) + getattr(test_meter, f"create_{instrument_name}")(instrument_name) + logger_mock.warning.assert_not_called() for instrument_name in [ "observable_counter", "observable_gauge", "observable_up_down_counter", ]: - with self.assertNoLogs(level=WARNING): - getattr(test_meter, f"create_{instrument_name}")( - instrument_name, Mock() - ) + getattr(test_meter, f"create_{instrument_name}")( + instrument_name, Mock() + ) + logger_mock.warning.assert_not_called() def test_repeated_instrument_names_with_different_advisory(self): - try: + with self.assertNotRaises(Exception): test_meter = NoOpMeter("name") test_meter.create_counter("counter") @@ -132,8 +130,6 @@ def test_repeated_instrument_names_with_different_advisory(self): test_meter.create_observable_up_down_counter( "observable_up_down_counter", Mock() ) - except Exception as error: # pylint: disable=broad-exception-caught - self.fail(f"Unexpected exception raised {error}") for instrument_name in [ "counter", diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index fb84780422f..9ea94b61c1d 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -431,7 +431,9 @@ class TestMeter(TestCase): def setUp(self): self.meter = Meter(Mock(), Mock()) - def test_repeated_instrument_names(self): + # TODO: convert to assertNoLogs instead of mocking logger when 3.10 is baseline + @patch("opentelemetry.sdk.metrics._internal._logger") + def test_repeated_instrument_names(self, logger_mock): with self.assertNotRaises(Exception): self.meter.create_counter("counter") self.meter.create_up_down_counter("up_down_counter") @@ -453,20 +455,18 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertNoLogs(level=WARNING): - getattr(self.meter, f"create_{instrument_name}")( - instrument_name - ) + getattr(self.meter, f"create_{instrument_name}")(instrument_name) + logger_mock.warning.assert_not_called() for instrument_name in [ "observable_counter", "observable_gauge", "observable_up_down_counter", ]: - with self.assertNoLogs(level=WARNING): - getattr(self.meter, f"create_{instrument_name}")( - instrument_name, callbacks=[Mock()] - ) + getattr(self.meter, f"create_{instrument_name}")( + instrument_name, callbacks=[Mock()] + ) + logger_mock.warning.assert_not_called() def test_repeated_instrument_names_with_different_advisory(self): with self.assertNotRaises(Exception): From d9fb95400d42a5cc25304539ddcd0070fc3572bc Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 11:55:27 +0100 Subject: [PATCH 20/41] Fix api tests for real --- opentelemetry-api/tests/metrics/test_meter.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 9fe06223e59..5494cd519fb 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -85,7 +85,7 @@ class TestMeter(TestCase): # TODO: convert to assertNoLogs instead of mocking logger when 3.10 is baseline @patch("opentelemetry.metrics._internal._logger") def test_repeated_instrument_names(self, logger_mock): - with self.assertNotRaises(Exception): + try: test_meter = NoOpMeter("name") test_meter.create_counter("counter") @@ -97,6 +97,8 @@ def test_repeated_instrument_names(self, logger_mock): test_meter.create_observable_up_down_counter( "observable_up_down_counter", Mock() ) + except Exception as error: # pylint: disable=broad-exception-caught + self.fail(f"Unexpected exception raised {error}") for instrument_name in [ "counter", @@ -118,7 +120,7 @@ def test_repeated_instrument_names(self, logger_mock): logger_mock.warning.assert_not_called() def test_repeated_instrument_names_with_different_advisory(self): - with self.assertNotRaises(Exception): + try: test_meter = NoOpMeter("name") test_meter.create_counter("counter") @@ -130,6 +132,8 @@ def test_repeated_instrument_names_with_different_advisory(self): test_meter.create_observable_up_down_counter( "observable_up_down_counter", Mock() ) + except Exception as error: # pylint: disable=broad-exception-caught + self.fail(f"Unexpected exception raised {error}") for instrument_name in [ "counter", From 70e25bb5f66665a70dfc04fd7c6edb3766573521 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 17:17:56 +0100 Subject: [PATCH 21/41] Change interface to a dataclass instead of a typed dict --- docs/examples/metrics/instruments/example.py | 7 +++++-- .../metrics/_internal/__init__.py | 12 ++++++++---- .../metrics/_internal/instrument.py | 8 ++++++-- .../src/opentelemetry/util/types.py | 16 ++++++++++++++-- .../sdk/metrics/_internal/__init__.py | 9 ++++++--- .../sdk/metrics/_internal/aggregation.py | 2 +- ...est_histogram_advisory_explicit_buckets.py | 13 ++++++++++--- .../tests/metrics/test_aggregation.py | 6 ++++-- .../tests/metrics/test_metrics.py | 19 +++++++++++-------- 9 files changed, 65 insertions(+), 27 deletions(-) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index 7d40d7f3c61..d5975c33dad 100644 --- a/docs/examples/metrics/instruments/example.py +++ b/docs/examples/metrics/instruments/example.py @@ -11,6 +11,7 @@ ) from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader +from opentelemetry.util.types import MetricsHistogramAdvisory exporter = OTLPMetricExporter(insecure=True) reader = PeriodicExportingMetricReader(exporter) @@ -59,10 +60,12 @@ def observable_gauge_func(options: CallbackOptions) -> Iterable[Observation]: histogram.record(99.9) -# Histogram with explicit_bucket_boundaries advisory +# Histogram with explicit bucket boundaries advisory histogram = meter.create_histogram( "histogram_with_advisory", - advisory={"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=[0.0, 1.0, 2.0] + ), ) histogram.record(99.9) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index aea892ef9b9..1cd48a6e9a5 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -75,7 +75,11 @@ ) from opentelemetry.util._once import Once from opentelemetry.util._providers import _load_provider -from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory +from opentelemetry.util.types import ( + Attributes, + MetricsHistogramAdvisory, + MetricsInstrumentAdvisory, +) _logger = getLogger(__name__) @@ -411,7 +415,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> Histogram: """Creates a :class:`~opentelemetry.metrics.Histogram` instrument @@ -565,7 +569,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> Histogram: with self._lock: if self._real_meter: @@ -745,7 +749,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> Histogram: """Returns a no-op Histogram.""" _, status = self._register_instrument( diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 1759c10229d..6fc9ec83fac 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -35,7 +35,11 @@ from opentelemetry import metrics from opentelemetry.context import Context from opentelemetry.metrics._internal.observation import Observation -from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory +from opentelemetry.util.types import ( + Attributes, + MetricsHistogramAdvisory, + MetricsInstrumentAdvisory, +) _logger = getLogger(__name__) @@ -380,7 +384,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> None: self._name = name self._unit = unit diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index 7bf11ad3f95..781409091f1 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -12,7 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Mapping, Optional, Sequence, Tuple, TypedDict, Union +from dataclasses import dataclass +from typing import Mapping, Optional, Sequence, Tuple, Union # This is the implementation of the "Any" type as specified by the specifications of OpenTelemetry data model for logs. # For more details, refer to the OTel specification: @@ -57,5 +58,16 @@ ] -class MetricsInstrumentAdvisory(TypedDict): +@dataclass +class MetricsHistogramAdvisory: explicit_bucket_boundaries: Optional[Sequence[float]] + + +@dataclass +class MetricsCommonAdvisory: + pass + + +MetricsInstrumentAdvisory = Union[ + MetricsCommonAdvisory, MetricsHistogramAdvisory +] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 70e480dba57..68ecdaf0d22 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -64,7 +64,10 @@ from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.util.instrumentation import InstrumentationScope from opentelemetry.util._once import Once -from opentelemetry.util.types import Attributes, MetricsInstrumentAdvisory +from opentelemetry.util.types import ( + Attributes, + MetricsHistogramAdvisory, +) _logger = getLogger(__name__) @@ -215,12 +218,12 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> APIHistogram: if advisory is not None: invalid_advisory = False try: - boundaries = advisory["explicit_bucket_boundaries"] + boundaries = advisory.explicit_bucket_boundaries invalid_advisory = not ( boundaries and all(isinstance(e, float) for e in boundaries) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 4b67a16ec9b..ea78b35e2f2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1284,7 +1284,7 @@ def _create_aggregation( if isinstance(instrument, Histogram): boundaries: Optional[Sequence[float]] = ( - instrument._advisory.get("explicit_bucket_boundaries") + instrument._advisory.explicit_bucket_boundaries if instrument._advisory is not None else None ) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 2dc78448ba3..f48d24a1a55 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -20,6 +20,7 @@ ExplicitBucketHistogramAggregation, View, ) +from opentelemetry.util.types import MetricsHistogramAdvisory class TestHistogramAdvisory(TestCase): @@ -31,7 +32,9 @@ def test_default(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=[1.0, 2.0, 3.0] + ), ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -59,7 +62,9 @@ def test_view_default_aggregation(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=[1.0, 2.0, 3.0] + ), ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -92,7 +97,9 @@ def test_view_overrides_buckets(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory={"explicit_bucket_boundaries": [1.0, 2.0, 3.0]}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=[1.0, 2.0, 3.0] + ), ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 527d4bf80eb..23ac5d61d94 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -51,7 +51,7 @@ LastValueAggregation, SumAggregation, ) -from opentelemetry.util.types import Attributes +from opentelemetry.util.types import Attributes, MetricsHistogramAdvisory def measurement( @@ -635,7 +635,9 @@ def test_histogram_with_advisory(self): "name", Mock(), Mock(), - advisory={"explicit_bucket_boundaries": boundaries}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=boundaries + ), ), Mock(), _default_reservoir_factory, diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 9ea94b61c1d..dd776675ea7 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -46,6 +46,7 @@ from opentelemetry.sdk.resources import Resource from opentelemetry.test import TestCase from opentelemetry.test.concurrency_test import ConcurrencyTestBase, MockFunc +from opentelemetry.util.types import MetricsHistogramAdvisory class DummyMetricReader(MetricReader): @@ -542,24 +543,26 @@ def test_create_histogram_with_advisory(self): "name", unit="unit", description="description", - advisory={"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, + advisory=MetricsHistogramAdvisory( + explicit_bucket_boundaries=[0.0, 1.0, 2.0] + ), ) self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( histogram._advisory, - {"explicit_bucket_boundaries": [0.0, 1.0, 2.0]}, + MetricsHistogramAdvisory( + explicit_bucket_boundaries=[0.0, 1.0, 2.0] + ), ) def test_create_histogram_advisory_validation(self): advisories = [ - {"explicit_bucket_boundaries": None}, - {"explicit_bucket_boundaries": []}, - {}, - [], - {"explicit_bucket_boundaries": [1]}, - {"explicit_bucket_boundaries": ["1"]}, + MetricsHistogramAdvisory(explicit_bucket_boundaries=None), + MetricsHistogramAdvisory(explicit_bucket_boundaries=[]), + MetricsHistogramAdvisory(explicit_bucket_boundaries=[1]), + MetricsHistogramAdvisory(explicit_bucket_boundaries=["1"]), ] for advisory in advisories: with self.subTest(advisory=advisory): From 76a318185763c91d414fed7101093abd30b70a58 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 17 Jan 2025 17:19:36 +0100 Subject: [PATCH 22/41] Add default --- opentelemetry-api/src/opentelemetry/util/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index 781409091f1..db84894827a 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -60,7 +60,7 @@ @dataclass class MetricsHistogramAdvisory: - explicit_bucket_boundaries: Optional[Sequence[float]] + explicit_bucket_boundaries: Optional[Sequence[float]] = None @dataclass From 1ae32b2fb2a4740a121f56e2dea1aa34fbe2bffa Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 10:12:57 +0100 Subject: [PATCH 23/41] Return instrument_id in _InstrumentRegistrationStatus --- .../metrics/_internal/__init__.py | 21 ++++--- .../sdk/metrics/_internal/__init__.py | 63 +++++++------------ 2 files changed, 32 insertions(+), 52 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 1cd48a6e9a5..a6e3a998846 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -46,7 +46,7 @@ from logging import getLogger from os import environ from threading import Lock -from typing import Dict, List, Optional, Sequence, Tuple, Union, cast +from typing import Dict, List, Optional, Sequence, Union, cast from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER from opentelemetry.metrics._internal.instrument import ( @@ -184,6 +184,7 @@ def on_set_meter_provider(self, meter_provider: MeterProvider) -> None: @dataclass class _InstrumentRegistrationStatus: + instrument_id: str already_registered: bool conflict: bool current_advisory: Optional[MetricsInstrumentAdvisory] @@ -239,7 +240,7 @@ def _register_instrument( unit: str, description: str, advisory: Optional[MetricsInstrumentAdvisory] = None, - ) -> Tuple[str, _InstrumentRegistrationStatus]: + ) -> _InstrumentRegistrationStatus: """ Register an instrument with the name, type, unit and description as identifying keys and the advisory as value. @@ -269,8 +270,8 @@ def _register_instrument( self._instrument_ids[instrument_id] = advisory return ( - instrument_id, _InstrumentRegistrationStatus( + instrument_id=instrument_id, already_registered=already_registered, conflict=conflict, current_advisory=current_advisory, @@ -650,7 +651,7 @@ def create_counter( advisory: None = None, ) -> Counter: """Returns a no-op Counter.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpCounter, unit, description, advisory ) if status.conflict: @@ -674,7 +675,7 @@ def create_gauge( advisory: None = None, ) -> Gauge: """Returns a no-op Gauge.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpGauge, unit, description, advisory ) if status.conflict: @@ -698,7 +699,7 @@ def create_up_down_counter( advisory: None = None, ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpUpDownCounter, unit, description, advisory ) if status.conflict: @@ -723,7 +724,7 @@ def create_observable_counter( advisory: None = None, ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpObservableCounter, unit, description, advisory ) if status.conflict: @@ -752,7 +753,7 @@ def create_histogram( advisory: Optional[MetricsHistogramAdvisory] = None, ) -> Histogram: """Returns a no-op Histogram.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpHistogram, unit, description, advisory ) if status.conflict: @@ -779,7 +780,7 @@ def create_observable_gauge( advisory: None = None, ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpObservableGauge, unit, description, advisory ) if status.conflict: @@ -809,7 +810,7 @@ def create_observable_up_down_counter( advisory: None = None, ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" - _, status = self._register_instrument( + status = self._register_instrument( name, NoOpObservableUpDownCounter, unit, description, advisory ) if status.conflict: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 68ecdaf0d22..148db141801 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -93,10 +93,7 @@ def __init__( def create_counter( self, name, unit="", description="", advisory=None ) -> APICounter: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _Counter, unit, description, advisory ) @@ -116,7 +113,7 @@ def create_counter( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _Counter( name, @@ -127,16 +124,13 @@ def create_counter( ) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_up_down_counter( self, name, unit="", description="", advisory=None ) -> APIUpDownCounter: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _UpDownCounter, unit, description, advisory ) @@ -156,7 +150,7 @@ def create_up_down_counter( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _UpDownCounter( name, @@ -167,16 +161,13 @@ def create_up_down_counter( ) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_observable_counter( self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableCounter: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _ObservableCounter, unit, description, advisory ) @@ -196,7 +187,7 @@ def create_observable_counter( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _ObservableCounter( name, @@ -210,7 +201,7 @@ def create_observable_counter( self._measurement_consumer.register_asynchronous_instrument(instrument) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_histogram( @@ -237,10 +228,7 @@ def create_histogram( "Advisory must be a dict with explicit_bucket_boundaries key containing a sequence of numbers" ) - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _Histogram, unit, description, advisory ) @@ -260,7 +248,7 @@ def create_histogram( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _Histogram( name, @@ -271,16 +259,13 @@ def create_histogram( advisory, ) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_gauge( self, name, unit="", description="", advisory=None ) -> APIGauge: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _Gauge, unit, description, advisory ) @@ -300,7 +285,7 @@ def create_gauge( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _Gauge( name, @@ -311,16 +296,13 @@ def create_gauge( ) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_observable_gauge( self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableGauge: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _ObservableGauge, unit, description, advisory ) @@ -340,7 +322,7 @@ def create_observable_gauge( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _ObservableGauge( name, @@ -354,16 +336,13 @@ def create_observable_gauge( self._measurement_consumer.register_asynchronous_instrument(instrument) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument def create_observable_up_down_counter( self, name, callbacks=None, unit="", description="", advisory=None ) -> APIObservableUpDownCounter: - ( - instrument_id, - status, - ) = self._register_instrument( + status = self._register_instrument( name, _ObservableUpDownCounter, unit, description, advisory ) @@ -383,7 +362,7 @@ def create_observable_up_down_counter( ) if status.already_registered: with self._instrument_id_instrument_lock: - return self._instrument_id_instrument[instrument_id] + return self._instrument_id_instrument[status.instrument_id] instrument = _ObservableUpDownCounter( name, @@ -397,7 +376,7 @@ def create_observable_up_down_counter( self._measurement_consumer.register_asynchronous_instrument(instrument) with self._instrument_id_instrument_lock: - self._instrument_id_instrument[instrument_id] = instrument + self._instrument_id_instrument[status.instrument_id] = instrument return instrument From 6678f42475c057d3fbe74ff814fc78e2e5c0f04c Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 10:14:37 +0100 Subject: [PATCH 24/41] Don't use tuple assignment --- .../src/opentelemetry/metrics/_internal/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index a6e3a998846..214cdb7e1ab 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -257,7 +257,8 @@ def _register_instrument( [name.strip().lower(), type_.__name__, unit, description] ) - already_registered, conflict = False, False + already_registered = False + conflict = False current_advisory = None with self._instrument_ids_lock: From 2a3242d04bb7b64db1606938e2e2896c4eb7c303 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 10:16:47 +0100 Subject: [PATCH 25/41] Correct advisory check to match reality --- .../src/opentelemetry/sdk/metrics/_internal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 148db141801..45a274b19a5 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -225,7 +225,7 @@ def create_histogram( if invalid_advisory: advisory = None _logger.warning( - "Advisory must be a dict with explicit_bucket_boundaries key containing a sequence of numbers" + "Advisory must be a valid MetricsHistogramAdvisory with explicit_bucket_boundaries key containing a sequence of floats" ) status = self._register_instrument( From cf14e2fcaef62e7addfc2a055826210f4c4c545e Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 10:18:39 +0100 Subject: [PATCH 26/41] fixup typo --- .../src/opentelemetry/metrics/_internal/__init__.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 214cdb7e1ab..c5dc9b56773 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -270,13 +270,11 @@ def _register_instrument( else: self._instrument_ids[instrument_id] = advisory - return ( - _InstrumentRegistrationStatus( - instrument_id=instrument_id, - already_registered=already_registered, - conflict=conflict, - current_advisory=current_advisory, - ), + return _InstrumentRegistrationStatus( + instrument_id=instrument_id, + already_registered=already_registered, + conflict=conflict, + current_advisory=current_advisory, ) @abstractmethod From b2c1f1bb9d2102727d2e433a3f9330e461b7e027 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 10:47:04 +0100 Subject: [PATCH 27/41] Centralize instrument registration conflict logging --- .../metrics/_internal/__init__.py | 69 +++++++++---------- .../sdk/metrics/_internal/__init__.py | 49 ++++--------- 2 files changed, 48 insertions(+), 70 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index c5dc9b56773..1a8e13dd218 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -277,6 +277,25 @@ def _register_instrument( current_advisory=current_advisory, ) + @staticmethod + def _log_instrument_registration_conflict( + name: str, + instrumentation_type: str, + unit: str, + description: str, + status: _InstrumentRegistrationStatus, + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already with a " + "different advisory value %s.", + name, + Counter.__name__, + unit, + description, + status.current_advisory, + ) + @abstractmethod def create_counter( self, @@ -654,16 +673,14 @@ def create_counter( name, NoOpCounter, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, Counter.__name__, unit, description, - status.current_advisory, + status, ) + return NoOpCounter(name, unit=unit, description=description) def create_gauge( @@ -678,15 +695,12 @@ def create_gauge( name, NoOpGauge, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, Gauge.__name__, unit, description, - status.current_advisory, + status, ) return NoOpGauge(name, unit=unit, description=description) @@ -702,15 +716,12 @@ def create_up_down_counter( name, NoOpUpDownCounter, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, UpDownCounter.__name__, unit, description, - status.current_advisory, + status, ) return NoOpUpDownCounter(name, unit=unit, description=description) @@ -727,15 +738,12 @@ def create_observable_counter( name, NoOpObservableCounter, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, ObservableCounter.__name__, unit, description, - status.current_advisory, + status, ) return NoOpObservableCounter( name, @@ -756,15 +764,12 @@ def create_histogram( name, NoOpHistogram, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, Histogram.__name__, unit, description, - status.current_advisory, + status, ) return NoOpHistogram( name, unit=unit, description=description, advisory=advisory @@ -783,15 +788,12 @@ def create_observable_gauge( name, NoOpObservableGauge, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, ObservableGauge.__name__, unit, description, - status.current_advisory, + status, ) return NoOpObservableGauge( name, @@ -813,15 +815,12 @@ def create_observable_up_down_counter( name, NoOpObservableUpDownCounter, unit, description, advisory ) if status.conflict: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, ObservableUpDownCounter.__name__, unit, description, - status.current_advisory, + status, ) return NoOpObservableUpDownCounter( name, diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 45a274b19a5..5cc953509a9 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -101,15 +101,12 @@ def create_counter( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APICounter.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -138,15 +135,12 @@ def create_up_down_counter( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIUpDownCounter.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -175,15 +169,12 @@ def create_observable_counter( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIObservableCounter.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -236,15 +227,12 @@ def create_histogram( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIHistogram.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -273,15 +261,12 @@ def create_gauge( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIGauge.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -310,15 +295,12 @@ def create_observable_gauge( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIObservableGauge.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: @@ -350,15 +332,12 @@ def create_observable_up_down_counter( # FIXME #2558 go through all views here and check if this # instrument registration conflict can be fixed. If it can be, do # not log the following warning. - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already with a " - "different advisory value %s.", + self._log_instrument_registration_conflict( name, APIObservableUpDownCounter.__name__, unit, description, - status.current_advisory, + status, ) if status.already_registered: with self._instrument_id_instrument_lock: From e29d5d5edd7d67f301e9e37659e67e5aba2ed906 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 11:14:27 +0100 Subject: [PATCH 28/41] Add advisory parameter to all instrumentor constructor --- .../metrics/_internal/__init__.py | 39 ++++---- .../metrics/_internal/instrument.py | 97 ++++++++++++++----- .../tests/metrics/test_meter_provider.py | 14 +-- 3 files changed, 101 insertions(+), 49 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 1a8e13dd218..e63ec73d1c0 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -77,6 +77,7 @@ from opentelemetry.util._providers import _load_provider from opentelemetry.util.types import ( Attributes, + MetricsCommonAdvisory, MetricsHistogramAdvisory, MetricsInstrumentAdvisory, ) @@ -284,7 +285,7 @@ def _log_instrument_registration_conflict( unit: str, description: str, status: _InstrumentRegistrationStatus, - ): + ) -> None: _logger.warning( "An instrument with name %s, type %s, unit %s and " "description %s has been created already with a " @@ -302,7 +303,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: """Creates a `Counter` instrument @@ -319,7 +320,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: """Creates an `UpDownCounter` instrument @@ -337,7 +338,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: """Creates an `ObservableCounter` instrument @@ -450,7 +451,7 @@ def create_gauge( # type: ignore # pylint: disable=no-self-use name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: """Creates a ``Gauge`` instrument @@ -469,7 +470,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: """Creates an `ObservableGauge` instrument @@ -490,7 +491,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: """Creates an `ObservableUpDownCounter` instrument @@ -539,7 +540,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: with self._lock: if self._real_meter: @@ -553,7 +554,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: with self._lock: if self._real_meter: @@ -570,7 +571,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: with self._lock: if self._real_meter: @@ -604,7 +605,7 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: with self._lock: if self._real_meter: @@ -619,7 +620,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: with self._lock: if self._real_meter: @@ -638,7 +639,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: with self._lock: if self._real_meter: @@ -666,7 +667,7 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: """Returns a no-op Counter.""" status = self._register_instrument( @@ -688,7 +689,7 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: """Returns a no-op Gauge.""" status = self._register_instrument( @@ -709,7 +710,7 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" status = self._register_instrument( @@ -731,7 +732,7 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" status = self._register_instrument( @@ -781,7 +782,7 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" status = self._register_instrument( @@ -808,7 +809,7 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: None = None, + advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" status = self._register_instrument( diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 6fc9ec83fac..a207fc2f376 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -37,6 +37,7 @@ from opentelemetry.metrics._internal.observation import Observation from opentelemetry.util.types import ( Attributes, + MetricsCommonAdvisory, MetricsHistogramAdvisory, MetricsInstrumentAdvisory, ) @@ -76,6 +77,7 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: pass @@ -121,10 +123,12 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: self._name = name self._unit = unit self._description = description + self._advisory = advisory self._real_instrument: Optional[InstrumentT] = None def on_meter_set(self, meter: "metrics.Meter") -> None: @@ -147,8 +151,9 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: - super().__init__(name, unit, description) + super().__init__(name, unit, description, advisory=advisory) self._callbacks = callbacks @@ -166,8 +171,11 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsInstrumentAdvisory] = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) class Counter(Synchronous): @@ -191,8 +199,11 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) def add( self, @@ -214,7 +225,9 @@ def add( self._real_instrument.add(amount, attributes, context) def _create_real_instrument(self, meter: "metrics.Meter") -> Counter: - return meter.create_counter(self._name, self._unit, self._description) + return meter.create_counter( + self._name, self._unit, self._description, advisory=self._advisory + ) class UpDownCounter(Synchronous): @@ -238,8 +251,11 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) def add( self, @@ -262,7 +278,7 @@ def add( def _create_real_instrument(self, meter: "metrics.Meter") -> UpDownCounter: return meter.create_up_down_counter( - self._name, self._unit, self._description + self._name, self._unit, self._description, advisory=self._advisory ) @@ -281,8 +297,15 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, + ) class _ProxyObservableCounter( @@ -292,7 +315,11 @@ def _create_real_instrument( self, meter: "metrics.Meter" ) -> ObservableCounter: return meter.create_observable_counter( - self._name, self._callbacks, self._unit, self._description + self._name, + self._callbacks, + self._unit, + self._description, + advisory=self._advisory, ) @@ -312,8 +339,15 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, + ) class _ProxyObservableUpDownCounter( @@ -324,7 +358,11 @@ def _create_real_instrument( self, meter: "metrics.Meter" ) -> ObservableUpDownCounter: return meter.create_observable_up_down_counter( - self._name, self._callbacks, self._unit, self._description + self._name, + self._callbacks, + self._unit, + self._description, + advisory=self._advisory, ) @@ -340,7 +378,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> None: pass @@ -362,7 +400,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ) -> None: super().__init__( name, unit=unit, description=description, advisory=advisory @@ -378,7 +416,6 @@ def record( class _ProxyHistogram(_ProxyInstrument[Histogram], Histogram): - # pylint: disable=super-init-not-called def __init__( self, name: str, @@ -386,11 +423,9 @@ def __init__( description: str = "", advisory: Optional[MetricsHistogramAdvisory] = None, ) -> None: - self._name = name - self._unit = unit - self._description = description - self._advisory = advisory - self._real_instrument: Optional[Histogram] = None + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) def record( self, @@ -403,7 +438,7 @@ def record( def _create_real_instrument(self, meter: "metrics.Meter") -> Histogram: return meter.create_histogram( - self._name, self._unit, self._description, self._advisory + self._name, self._unit, self._description, advisory=self._advisory ) @@ -423,8 +458,15 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + advisory=advisory, + ) class _ProxyObservableGauge( @@ -435,7 +477,11 @@ def _create_real_instrument( self, meter: "metrics.Meter" ) -> ObservableGauge: return meter.create_observable_gauge( - self._name, self._callbacks, self._unit, self._description + self._name, + self._callbacks, + self._unit, + self._description, + advisory=self._advisory, ) @@ -460,8 +506,11 @@ def __init__( name: str, unit: str = "", description: str = "", + advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, unit=unit, description=description, advisory=advisory + ) def set( self, @@ -486,4 +535,6 @@ def set( self._real_instrument.set(amount, attributes, context) def _create_real_instrument(self, meter: "metrics.Meter") -> Gauge: - return meter.create_gauge(self._name, self._unit, self._description) + return meter.create_gauge( + self._name, self._unit, self._description, advisory=self._advisory + ) diff --git a/opentelemetry-api/tests/metrics/test_meter_provider.py b/opentelemetry-api/tests/metrics/test_meter_provider.py index 37910def0fa..a2099d2a421 100644 --- a/opentelemetry-api/tests/metrics/test_meter_provider.py +++ b/opentelemetry-api/tests/metrics/test_meter_provider.py @@ -268,25 +268,25 @@ def test_proxy_meter(self): real_meter: Mock = real_meter_provider.get_meter() real_meter.create_counter.assert_called_once_with( - name, unit, description + name, unit, description, advisory=None ) real_meter.create_up_down_counter.assert_called_once_with( - name, unit, description + name, unit, description, advisory=None ) real_meter.create_histogram.assert_called_once_with( - name, unit, description, None + name, unit, description, advisory=None ) real_meter.create_gauge.assert_called_once_with( - name, unit, description + name, unit, description, advisory=None ) real_meter.create_observable_counter.assert_called_once_with( - name, [callback], unit, description + name, [callback], unit, description, advisory=None ) real_meter.create_observable_up_down_counter.assert_called_once_with( - name, [callback], unit, description + name, [callback], unit, description, advisory=None ) real_meter.create_observable_gauge.assert_called_once_with( - name, [callback], unit, description + name, [callback], unit, description, advisory=None ) # The synchronous instrument measurement methods should call through to From 69f58a255d3804f31917edf6c446e9cb25153dea Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 20 Jan 2025 11:40:11 +0100 Subject: [PATCH 29/41] Accept ints as valid value for advisory explicit bucket boundaries --- .../src/opentelemetry/sdk/metrics/_internal/__init__.py | 4 ++-- opentelemetry-sdk/tests/metrics/test_metrics.py | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 5cc953509a9..377e1420c99 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -208,7 +208,7 @@ def create_histogram( boundaries = advisory.explicit_bucket_boundaries invalid_advisory = not ( boundaries - and all(isinstance(e, float) for e in boundaries) + and all(isinstance(e, (float, int)) for e in boundaries) ) except (KeyError, TypeError): invalid_advisory = True @@ -216,7 +216,7 @@ def create_histogram( if invalid_advisory: advisory = None _logger.warning( - "Advisory must be a valid MetricsHistogramAdvisory with explicit_bucket_boundaries key containing a sequence of floats" + "Advisory must be a valid MetricsHistogramAdvisory with explicit_bucket_boundaries key containing a sequence of numbers" ) status = self._register_instrument( diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index dd776675ea7..89cbef4beb0 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -544,7 +544,7 @@ def test_create_histogram_with_advisory(self): unit="unit", description="description", advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[0.0, 1.0, 2.0] + explicit_bucket_boundaries=[0.0, 1.0, 2] ), ) @@ -552,16 +552,13 @@ def test_create_histogram_with_advisory(self): self.assertEqual(histogram.name, "name") self.assertEqual( histogram._advisory, - MetricsHistogramAdvisory( - explicit_bucket_boundaries=[0.0, 1.0, 2.0] - ), + MetricsHistogramAdvisory(explicit_bucket_boundaries=[0.0, 1.0, 2]), ) def test_create_histogram_advisory_validation(self): advisories = [ MetricsHistogramAdvisory(explicit_bucket_boundaries=None), MetricsHistogramAdvisory(explicit_bucket_boundaries=[]), - MetricsHistogramAdvisory(explicit_bucket_boundaries=[1]), MetricsHistogramAdvisory(explicit_bucket_boundaries=["1"]), ] for advisory in advisories: From b7b4977fcb8d05543ee17d0a798f992848ccab5a Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 21 Jan 2025 15:36:50 +0100 Subject: [PATCH 30/41] Please mypy again --- .../metrics/_internal/instrument.py | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index a207fc2f376..da14fb14beb 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -29,6 +29,7 @@ Sequence, TypeVar, Union, + cast, ) # pylint: disable=unused-import; needed for typing and sphinx @@ -226,7 +227,10 @@ def add( def _create_real_instrument(self, meter: "metrics.Meter") -> Counter: return meter.create_counter( - self._name, self._unit, self._description, advisory=self._advisory + self._name, + self._unit, + self._description, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -278,7 +282,10 @@ def add( def _create_real_instrument(self, meter: "metrics.Meter") -> UpDownCounter: return meter.create_up_down_counter( - self._name, self._unit, self._description, advisory=self._advisory + self._name, + self._unit, + self._description, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -319,7 +326,7 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=self._advisory, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -362,7 +369,7 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=self._advisory, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -438,7 +445,10 @@ def record( def _create_real_instrument(self, meter: "metrics.Meter") -> Histogram: return meter.create_histogram( - self._name, self._unit, self._description, advisory=self._advisory + self._name, + self._unit, + self._description, + advisory=cast(MetricsHistogramAdvisory, self._advisory), ) @@ -481,7 +491,7 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=self._advisory, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -536,5 +546,8 @@ def set( def _create_real_instrument(self, meter: "metrics.Meter") -> Gauge: return meter.create_gauge( - self._name, self._unit, self._description, advisory=self._advisory + self._name, + self._unit, + self._description, + advisory=cast(MetricsCommonAdvisory, self._advisory), ) From 99e123e517bff8bc9900fe7669572972491b344a Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 21 Jan 2025 15:48:01 +0100 Subject: [PATCH 31/41] Move Advisory types to metrics _internal and export the public one --- docs/examples/metrics/instruments/example.py | 2 +- .../src/opentelemetry/metrics/__init__.py | 4 +++ .../metrics/_internal/__init__.py | 12 ++++----- .../metrics/_internal/instrument.py | 26 ++++++++++++++----- .../src/opentelemetry/util/types.py | 16 ------------ .../sdk/metrics/_internal/__init__.py | 3 +-- .../sdk/metrics/_internal/instrument.py | 5 ++-- ...est_histogram_advisory_explicit_buckets.py | 2 +- .../tests/metrics/test_aggregation.py | 3 ++- .../tests/metrics/test_metrics.py | 3 +-- 10 files changed, 37 insertions(+), 39 deletions(-) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index d5975c33dad..265b0bea6bd 100644 --- a/docs/examples/metrics/instruments/example.py +++ b/docs/examples/metrics/instruments/example.py @@ -5,13 +5,13 @@ ) from opentelemetry.metrics import ( CallbackOptions, + MetricsHistogramAdvisory, Observation, get_meter_provider, set_meter_provider, ) from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader -from opentelemetry.util.types import MetricsHistogramAdvisory exporter = OTLPMetricExporter(insecure=True) reader = PeriodicExportingMetricReader(exporter) diff --git a/opentelemetry-api/src/opentelemetry/metrics/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/__init__.py index 74284ad6e3f..394855c4988 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/__init__.py @@ -55,6 +55,8 @@ Counter, Histogram, Instrument, + MetricsCommonAdvisory, + MetricsHistogramAdvisory, NoOpCounter, NoOpHistogram, NoOpObservableCounter, @@ -129,4 +131,6 @@ "Observation", "CallbackT", "NoOpMeter", + "MetricsCommonAdvisory", + "MetricsHistogramAdvisory", ] diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index e63ec73d1c0..cdf2f4743b4 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -54,6 +54,8 @@ Counter, Gauge, Histogram, + MetricsCommonAdvisory, + MetricsHistogramAdvisory, NoOpCounter, NoOpGauge, NoOpHistogram, @@ -65,6 +67,7 @@ ObservableGauge, ObservableUpDownCounter, UpDownCounter, + _MetricsInstrumentAdvisory, _ProxyCounter, _ProxyGauge, _ProxyHistogram, @@ -77,9 +80,6 @@ from opentelemetry.util._providers import _load_provider from opentelemetry.util.types import ( Attributes, - MetricsCommonAdvisory, - MetricsHistogramAdvisory, - MetricsInstrumentAdvisory, ) _logger = getLogger(__name__) @@ -188,7 +188,7 @@ class _InstrumentRegistrationStatus: instrument_id: str already_registered: bool conflict: bool - current_advisory: Optional[MetricsInstrumentAdvisory] + current_advisory: Optional[_MetricsInstrumentAdvisory] class Meter(ABC): @@ -209,7 +209,7 @@ def __init__( self._version = version self._schema_url = schema_url self._instrument_ids: Dict[ - str, Optional[MetricsInstrumentAdvisory] + str, Optional[_MetricsInstrumentAdvisory] ] = {} self._instrument_ids_lock = Lock() @@ -240,7 +240,7 @@ def _register_instrument( type_: type, unit: str, description: str, - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> _InstrumentRegistrationStatus: """ Register an instrument with the name, type, unit and description as diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index da14fb14beb..9219411d16e 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -38,9 +38,6 @@ from opentelemetry.metrics._internal.observation import Observation from opentelemetry.util.types import ( Attributes, - MetricsCommonAdvisory, - MetricsHistogramAdvisory, - MetricsInstrumentAdvisory, ) _logger = getLogger(__name__) @@ -49,6 +46,21 @@ _unit_regex = re_compile(r"[\x00-\x7F]{0,63}") +@dataclass(frozen=True) +class MetricsHistogramAdvisory: + explicit_bucket_boundaries: Optional[Sequence[float]] = None + + +@dataclass(frozen=True) +class MetricsCommonAdvisory: + pass + + +_MetricsInstrumentAdvisory = Union[ + MetricsCommonAdvisory, MetricsHistogramAdvisory +] + + @dataclass(frozen=True) class CallbackOptions: """Options for the callback @@ -78,7 +90,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: pass @@ -124,7 +136,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: self._name = name self._unit = unit @@ -152,7 +164,7 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: super().__init__(name, unit, description, advisory=advisory) self._callbacks = callbacks @@ -172,7 +184,7 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: super().__init__( name, unit=unit, description=description, advisory=advisory diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index db84894827a..be311faf555 100644 --- a/opentelemetry-api/src/opentelemetry/util/types.py +++ b/opentelemetry-api/src/opentelemetry/util/types.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from dataclasses import dataclass from typing import Mapping, Optional, Sequence, Tuple, Union # This is the implementation of the "Any" type as specified by the specifications of OpenTelemetry data model for logs. @@ -56,18 +55,3 @@ ], ..., ] - - -@dataclass -class MetricsHistogramAdvisory: - explicit_bucket_boundaries: Optional[Sequence[float]] = None - - -@dataclass -class MetricsCommonAdvisory: - pass - - -MetricsInstrumentAdvisory = Union[ - MetricsCommonAdvisory, MetricsHistogramAdvisory -] diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 377e1420c99..d4f53253c0a 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -26,7 +26,7 @@ from opentelemetry.metrics import Histogram as APIHistogram from opentelemetry.metrics import Meter as APIMeter from opentelemetry.metrics import MeterProvider as APIMeterProvider -from opentelemetry.metrics import NoOpMeter +from opentelemetry.metrics import MetricsHistogramAdvisory, NoOpMeter from opentelemetry.metrics import ObservableCounter as APIObservableCounter from opentelemetry.metrics import ObservableGauge as APIObservableGauge from opentelemetry.metrics import ( @@ -66,7 +66,6 @@ from opentelemetry.util._once import Once from opentelemetry.util.types import ( Attributes, - MetricsHistogramAdvisory, ) _logger = getLogger(__name__) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index 6e2c62ce222..ee071e3f2c3 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -21,7 +21,7 @@ # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics from opentelemetry.context import Context, get_current -from opentelemetry.metrics import CallbackT +from opentelemetry.metrics import CallbackT, MetricsHistogramAdvisory from opentelemetry.metrics import Counter as APICounter from opentelemetry.metrics import Histogram as APIHistogram from opentelemetry.metrics import ObservableCounter as APIObservableCounter @@ -34,7 +34,6 @@ from opentelemetry.metrics._internal.instrument import CallbackOptions from opentelemetry.sdk.metrics._internal.measurement import Measurement from opentelemetry.sdk.util.instrumentation import InstrumentationScope -from opentelemetry.util.types import MetricsInstrumentAdvisory _logger = getLogger(__name__) @@ -227,7 +226,7 @@ def __init__( measurement_consumer: "opentelemetry.sdk.metrics.MeasurementConsumer", unit: str = "", description: str = "", - advisory: Optional[MetricsInstrumentAdvisory] = None, + advisory: Optional[MetricsHistogramAdvisory] = None, ): super().__init__( name, diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index f48d24a1a55..0526734e647 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -14,13 +14,13 @@ from unittest import TestCase +from opentelemetry.metrics import MetricsHistogramAdvisory from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import InMemoryMetricReader from opentelemetry.sdk.metrics.view import ( ExplicitBucketHistogramAggregation, View, ) -from opentelemetry.util.types import MetricsHistogramAdvisory class TestHistogramAdvisory(TestCase): diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 23ac5d61d94..2aab708efa0 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -21,6 +21,7 @@ from unittest.mock import Mock from opentelemetry.context import Context +from opentelemetry.metrics import MetricsHistogramAdvisory from opentelemetry.sdk.metrics._internal.aggregation import ( _ExplicitBucketHistogramAggregation, _LastValueAggregation, @@ -51,7 +52,7 @@ LastValueAggregation, SumAggregation, ) -from opentelemetry.util.types import Attributes, MetricsHistogramAdvisory +from opentelemetry.util.types import Attributes def measurement( diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 89cbef4beb0..ef704d6d7e9 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -21,7 +21,7 @@ from unittest.mock import MagicMock, Mock, patch from opentelemetry.attributes import BoundedAttributes -from opentelemetry.metrics import NoOpMeter +from opentelemetry.metrics import MetricsHistogramAdvisory, NoOpMeter from opentelemetry.sdk.environment_variables import OTEL_SDK_DISABLED from opentelemetry.sdk.metrics import ( Counter, @@ -46,7 +46,6 @@ from opentelemetry.sdk.resources import Resource from opentelemetry.test import TestCase from opentelemetry.test.concurrency_test import ConcurrencyTestBase, MockFunc -from opentelemetry.util.types import MetricsHistogramAdvisory class DummyMetricReader(MetricReader): From cf2163d8b7a03b766bcb3cbd2b70be08799b17d3 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 21 Jan 2025 15:57:19 +0100 Subject: [PATCH 32/41] Give an hint on instrument registration conflict that the other one will be used --- .../src/opentelemetry/metrics/_internal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index cdf2f4743b4..ef6e1d3348e 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -289,7 +289,7 @@ def _log_instrument_registration_conflict( _logger.warning( "An instrument with name %s, type %s, unit %s and " "description %s has been created already with a " - "different advisory value %s.", + "different advisory value %s and will be used instead.", name, Counter.__name__, unit, From 93db7ea96bd1667d5513fca47086ccd598fc78d6 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Thu, 23 Jan 2025 18:43:17 +0100 Subject: [PATCH 33/41] Update opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com> --- .../src/opentelemetry/metrics/_internal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index ef6e1d3348e..0e1cb1d0d7b 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -291,7 +291,7 @@ def _log_instrument_registration_conflict( "description %s has been created already with a " "different advisory value %s and will be used instead.", name, - Counter.__name__, + instrumentation_type unit, description, status.current_advisory, From 82e2f08b079e18a775b6fc07c7cf5641b1d47199 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Em=C3=ADdio=20Neto?= <9735060+emdneto@users.noreply.github.com> Date: Thu, 23 Jan 2025 14:55:58 -0300 Subject: [PATCH 34/41] Update opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py --- .../src/opentelemetry/metrics/_internal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 0e1cb1d0d7b..9c9ee1f092e 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -291,7 +291,7 @@ def _log_instrument_registration_conflict( "description %s has been created already with a " "different advisory value %s and will be used instead.", name, - instrumentation_type + instrumentation_type, unit, description, status.current_advisory, From 82536bc55b601a3c26e0e9b154ea292222eb707a Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 24 Jan 2025 18:07:08 +0100 Subject: [PATCH 35/41] Flattened advisory params --- docs/examples/metrics/instruments/example.py | 5 +- .../src/opentelemetry/metrics/__init__.py | 4 - .../metrics/_internal/__init__.py | 74 ++++++++----------- .../metrics/_internal/instrument.py | 68 +++++------------ opentelemetry-api/tests/metrics/test_meter.py | 65 ++++++---------- .../tests/metrics/test_meter_provider.py | 14 ++-- .../sdk/metrics/_internal/__init__.py | 63 ++++++++-------- .../sdk/metrics/_internal/aggregation.py | 4 +- .../sdk/metrics/_internal/instrument.py | 11 ++- ...est_histogram_advisory_explicit_buckets.py | 13 +--- .../tests/metrics/test_aggregation.py | 5 +- .../tests/metrics/test_metrics.py | 46 +++--------- 12 files changed, 136 insertions(+), 236 deletions(-) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index 265b0bea6bd..90a9f7fa234 100644 --- a/docs/examples/metrics/instruments/example.py +++ b/docs/examples/metrics/instruments/example.py @@ -5,7 +5,6 @@ ) from opentelemetry.metrics import ( CallbackOptions, - MetricsHistogramAdvisory, Observation, get_meter_provider, set_meter_provider, @@ -63,9 +62,7 @@ def observable_gauge_func(options: CallbackOptions) -> Iterable[Observation]: # Histogram with explicit bucket boundaries advisory histogram = meter.create_histogram( "histogram_with_advisory", - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[0.0, 1.0, 2.0] - ), + explicit_bucket_boundaries_advisory=[0.0, 1.0, 2.0], ) histogram.record(99.9) diff --git a/opentelemetry-api/src/opentelemetry/metrics/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/__init__.py index 394855c4988..74284ad6e3f 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/__init__.py @@ -55,8 +55,6 @@ Counter, Histogram, Instrument, - MetricsCommonAdvisory, - MetricsHistogramAdvisory, NoOpCounter, NoOpHistogram, NoOpObservableCounter, @@ -131,6 +129,4 @@ "Observation", "CallbackT", "NoOpMeter", - "MetricsCommonAdvisory", - "MetricsHistogramAdvisory", ] diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 9c9ee1f092e..f808d42d923 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -46,7 +46,7 @@ from logging import getLogger from os import environ from threading import Lock -from typing import Dict, List, Optional, Sequence, Union, cast +from typing import Any, Dict, List, Optional, Sequence, Union, cast from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER from opentelemetry.metrics._internal.instrument import ( @@ -54,8 +54,6 @@ Counter, Gauge, Histogram, - MetricsCommonAdvisory, - MetricsHistogramAdvisory, NoOpCounter, NoOpGauge, NoOpHistogram, @@ -67,7 +65,6 @@ ObservableGauge, ObservableUpDownCounter, UpDownCounter, - _MetricsInstrumentAdvisory, _ProxyCounter, _ProxyGauge, _ProxyHistogram, @@ -188,7 +185,7 @@ class _InstrumentRegistrationStatus: instrument_id: str already_registered: bool conflict: bool - current_advisory: Optional[_MetricsInstrumentAdvisory] + current_advisory: Optional[Any] class Meter(ABC): @@ -208,9 +205,7 @@ def __init__( self._name = name self._version = version self._schema_url = schema_url - self._instrument_ids: Dict[ - str, Optional[_MetricsInstrumentAdvisory] - ] = {} + self._instrument_ids: Dict[str, Optional[Any]] = {} self._instrument_ids_lock = Lock() @property @@ -240,7 +235,7 @@ def _register_instrument( type_: type, unit: str, description: str, - advisory: Optional[_MetricsInstrumentAdvisory] = None, + advisory: Optional[Any] = None, ) -> _InstrumentRegistrationStatus: """ Register an instrument with the name, type, unit and description as @@ -303,7 +298,6 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: """Creates a `Counter` instrument @@ -320,7 +314,6 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: """Creates an `UpDownCounter` instrument @@ -338,7 +331,6 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: """Creates an `ObservableCounter` instrument @@ -435,7 +427,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Creates a :class:`~opentelemetry.metrics.Histogram` instrument @@ -451,7 +443,6 @@ def create_gauge( # type: ignore # pylint: disable=no-self-use name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: """Creates a ``Gauge`` instrument @@ -470,7 +461,6 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: """Creates an `ObservableGauge` instrument @@ -491,7 +481,6 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: """Creates an `ObservableUpDownCounter` instrument @@ -540,7 +529,6 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: with self._lock: if self._real_meter: @@ -554,7 +542,6 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: with self._lock: if self._real_meter: @@ -571,7 +558,6 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: with self._lock: if self._real_meter: @@ -589,14 +575,19 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: with self._lock: if self._real_meter: return self._real_meter.create_histogram( - name, unit, description, advisory + name, + unit, + description, + explicit_bucket_boundaries_advisory, ) - proxy = _ProxyHistogram(name, unit, description, advisory) + proxy = _ProxyHistogram( + name, unit, description, explicit_bucket_boundaries_advisory + ) self._instruments.append(proxy) return proxy @@ -605,7 +596,6 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: with self._lock: if self._real_meter: @@ -620,7 +610,6 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: with self._lock: if self._real_meter: @@ -639,7 +628,6 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: with self._lock: if self._real_meter: @@ -667,11 +655,10 @@ def create_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Counter: """Returns a no-op Counter.""" status = self._register_instrument( - name, NoOpCounter, unit, description, advisory + name, NoOpCounter, unit, description ) if status.conflict: self._log_instrument_registration_conflict( @@ -689,12 +676,9 @@ def create_gauge( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> Gauge: """Returns a no-op Gauge.""" - status = self._register_instrument( - name, NoOpGauge, unit, description, advisory - ) + status = self._register_instrument(name, NoOpGauge, unit, description) if status.conflict: self._log_instrument_registration_conflict( name, @@ -710,11 +694,10 @@ def create_up_down_counter( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" status = self._register_instrument( - name, NoOpUpDownCounter, unit, description, advisory + name, NoOpUpDownCounter, unit, description ) if status.conflict: self._log_instrument_registration_conflict( @@ -732,11 +715,10 @@ def create_observable_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" status = self._register_instrument( - name, NoOpObservableCounter, unit, description, advisory + name, NoOpObservableCounter, unit, description ) if status.conflict: self._log_instrument_registration_conflict( @@ -758,11 +740,18 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Returns a no-op Histogram.""" + # FIXME: should be use a dataclass here instead for the advisory param? status = self._register_instrument( - name, NoOpHistogram, unit, description, advisory + name, + NoOpHistogram, + unit, + description, + { + "explicit_bucket_boundaries_advisory": explicit_bucket_boundaries_advisory + }, ) if status.conflict: self._log_instrument_registration_conflict( @@ -773,7 +762,10 @@ def create_histogram( status, ) return NoOpHistogram( - name, unit=unit, description=description, advisory=advisory + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, ) def create_observable_gauge( @@ -782,11 +774,10 @@ def create_observable_gauge( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" status = self._register_instrument( - name, NoOpObservableGauge, unit, description, advisory + name, NoOpObservableGauge, unit, description ) if status.conflict: self._log_instrument_registration_conflict( @@ -809,11 +800,10 @@ def create_observable_up_down_counter( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" status = self._register_instrument( - name, NoOpObservableUpDownCounter, unit, description, advisory + name, NoOpObservableUpDownCounter, unit, description ) if status.conflict: self._log_instrument_registration_conflict( diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 9219411d16e..5b0d5156fb2 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -29,7 +29,6 @@ Sequence, TypeVar, Union, - cast, ) # pylint: disable=unused-import; needed for typing and sphinx @@ -46,21 +45,12 @@ _unit_regex = re_compile(r"[\x00-\x7F]{0,63}") +# FIXME: remove this if we are not going to use it to store advisory in registered instruments @dataclass(frozen=True) class MetricsHistogramAdvisory: explicit_bucket_boundaries: Optional[Sequence[float]] = None -@dataclass(frozen=True) -class MetricsCommonAdvisory: - pass - - -_MetricsInstrumentAdvisory = Union[ - MetricsCommonAdvisory, MetricsHistogramAdvisory -] - - @dataclass(frozen=True) class CallbackOptions: """Options for the callback @@ -90,7 +80,6 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: pass @@ -136,12 +125,10 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: self._name = name self._unit = unit self._description = description - self._advisory = advisory self._real_instrument: Optional[InstrumentT] = None def on_meter_set(self, meter: "metrics.Meter") -> None: @@ -164,9 +151,8 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: - super().__init__(name, unit, description, advisory=advisory) + super().__init__(name, unit, description) self._callbacks = callbacks @@ -184,11 +170,8 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[_MetricsInstrumentAdvisory] = None, ) -> None: - super().__init__( - name, unit=unit, description=description, advisory=advisory - ) + super().__init__(name, unit=unit, description=description) class Counter(Synchronous): @@ -212,11 +195,8 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__( - name, unit=unit, description=description, advisory=advisory - ) + super().__init__(name, unit=unit, description=description) def add( self, @@ -242,7 +222,6 @@ def _create_real_instrument(self, meter: "metrics.Meter") -> Counter: self._name, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -267,11 +246,8 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__( - name, unit=unit, description=description, advisory=advisory - ) + super().__init__(name, unit=unit, description=description) def add( self, @@ -297,7 +273,6 @@ def _create_real_instrument(self, meter: "metrics.Meter") -> UpDownCounter: self._name, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -316,14 +291,12 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: super().__init__( name, callbacks, unit=unit, description=description, - advisory=advisory, ) @@ -338,7 +311,6 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -358,14 +330,12 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: super().__init__( name, callbacks, unit=unit, description=description, - advisory=advisory, ) @@ -381,7 +351,6 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -397,7 +366,7 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> None: pass @@ -419,10 +388,13 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> None: super().__init__( - name, unit=unit, description=description, advisory=advisory + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, ) def record( @@ -440,10 +412,11 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> None: - super().__init__( - name, unit=unit, description=description, advisory=advisory + super().__init__(name, unit=unit, description=description) + self._explicit_bucket_boundaries_advisory = ( + explicit_bucket_boundaries_advisory ) def record( @@ -460,7 +433,7 @@ def _create_real_instrument(self, meter: "metrics.Meter") -> Histogram: self._name, self._unit, self._description, - advisory=cast(MetricsHistogramAdvisory, self._advisory), + explicit_bucket_boundaries_advisory=self._explicit_bucket_boundaries_advisory, ) @@ -480,14 +453,12 @@ def __init__( callbacks: Optional[Sequence[CallbackT]] = None, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: super().__init__( name, callbacks, unit=unit, description=description, - advisory=advisory, ) @@ -503,7 +474,6 @@ def _create_real_instrument( self._callbacks, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) @@ -528,11 +498,8 @@ def __init__( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsCommonAdvisory] = None, ) -> None: - super().__init__( - name, unit=unit, description=description, advisory=advisory - ) + super().__init__(name, unit=unit, description=description) def set( self, @@ -561,5 +528,4 @@ def _create_real_instrument(self, meter: "metrics.Meter") -> Gauge: self._name, self._unit, self._description, - advisory=cast(MetricsCommonAdvisory, self._advisory), ) diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 5494cd519fb..4df0f939e03 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -24,59 +24,59 @@ class ChildMeter(Meter): # pylint: disable=signature-differs - def create_counter(self, name, unit="", description="", advisory=None): - super().create_counter( - name, unit=unit, description=description, advisory=advisory - ) + def create_counter(self, name, unit="", description=""): + super().create_counter(name, unit=unit, description=description) - def create_up_down_counter( - self, name, unit="", description="", advisory=None - ): + def create_up_down_counter(self, name, unit="", description=""): super().create_up_down_counter( - name, unit=unit, description=description, advisory=advisory + name, unit=unit, description=description ) def create_observable_counter( - self, name, callbacks, unit="", description="", advisory=None + self, name, callbacks, unit="", description="" ): super().create_observable_counter( name, callbacks, unit=unit, description=description, - advisory=advisory, ) - def create_histogram(self, name, unit="", description="", advisory=None): + def create_histogram( + self, + name, + unit="", + description="", + explicit_bucket_boundaries_advisory=None, + ): super().create_histogram( - name, unit=unit, description=description, advisory=advisory + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, ) - def create_gauge(self, name, unit="", description="", advisory=None): - super().create_gauge( - name, unit=unit, description=description, advisory=advisory - ) + def create_gauge(self, name, unit="", description=""): + super().create_gauge(name, unit=unit, description=description) def create_observable_gauge( - self, name, callbacks, unit="", description="", advisory=None + self, name, callbacks, unit="", description="" ): super().create_observable_gauge( name, callbacks, unit=unit, description=description, - advisory=advisory, ) def create_observable_up_down_counter( - self, name, callbacks, unit="", description="", advisory=None + self, name, callbacks, unit="", description="" ): super().create_observable_up_down_counter( name, callbacks, unit=unit, description=description, - advisory=advisory, ) @@ -123,37 +123,18 @@ def test_repeated_instrument_names_with_different_advisory(self): try: test_meter = NoOpMeter("name") - test_meter.create_counter("counter") - test_meter.create_up_down_counter("up_down_counter") - test_meter.create_observable_counter("observable_counter", Mock()) - test_meter.create_histogram("histogram") - test_meter.create_gauge("gauge") - test_meter.create_observable_gauge("observable_gauge", Mock()) - test_meter.create_observable_up_down_counter( - "observable_up_down_counter", Mock() + test_meter.create_histogram( + "histogram", explicit_bucket_boundaries_advisory=[1.0] ) except Exception as error: # pylint: disable=broad-exception-caught self.fail(f"Unexpected exception raised {error}") for instrument_name in [ - "counter", - "up_down_counter", "histogram", - "gauge", - ]: - with self.assertLogs(level=WARNING): - getattr(test_meter, f"create_{instrument_name}")( - instrument_name, advisory=Mock() - ) - - for instrument_name in [ - "observable_counter", - "observable_gauge", - "observable_up_down_counter", ]: with self.assertLogs(level=WARNING): getattr(test_meter, f"create_{instrument_name}")( - instrument_name, Mock(), advisory=Mock() + instrument_name, ) def test_create_counter(self): diff --git a/opentelemetry-api/tests/metrics/test_meter_provider.py b/opentelemetry-api/tests/metrics/test_meter_provider.py index a2099d2a421..dfaf94bcec2 100644 --- a/opentelemetry-api/tests/metrics/test_meter_provider.py +++ b/opentelemetry-api/tests/metrics/test_meter_provider.py @@ -268,25 +268,25 @@ def test_proxy_meter(self): real_meter: Mock = real_meter_provider.get_meter() real_meter.create_counter.assert_called_once_with( - name, unit, description, advisory=None + name, unit, description ) real_meter.create_up_down_counter.assert_called_once_with( - name, unit, description, advisory=None + name, unit, description ) real_meter.create_histogram.assert_called_once_with( - name, unit, description, advisory=None + name, unit, description, explicit_bucket_boundaries_advisory=None ) real_meter.create_gauge.assert_called_once_with( - name, unit, description, advisory=None + name, unit, description ) real_meter.create_observable_counter.assert_called_once_with( - name, [callback], unit, description, advisory=None + name, [callback], unit, description ) real_meter.create_observable_up_down_counter.assert_called_once_with( - name, [callback], unit, description, advisory=None + name, [callback], unit, description ) real_meter.create_observable_gauge.assert_called_once_with( - name, [callback], unit, description, advisory=None + name, [callback], unit, description ) # The synchronous instrument measurement methods should call through to diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index d4f53253c0a..06e5a8875c6 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -26,7 +26,7 @@ from opentelemetry.metrics import Histogram as APIHistogram from opentelemetry.metrics import Meter as APIMeter from opentelemetry.metrics import MeterProvider as APIMeterProvider -from opentelemetry.metrics import MetricsHistogramAdvisory, NoOpMeter +from opentelemetry.metrics import NoOpMeter from opentelemetry.metrics import ObservableCounter as APIObservableCounter from opentelemetry.metrics import ObservableGauge as APIObservableGauge from opentelemetry.metrics import ( @@ -89,12 +89,8 @@ def __init__( self._instrument_id_instrument = {} self._instrument_id_instrument_lock = Lock() - def create_counter( - self, name, unit="", description="", advisory=None - ) -> APICounter: - status = self._register_instrument( - name, _Counter, unit, description, advisory - ) + def create_counter(self, name, unit="", description="") -> APICounter: + status = self._register_instrument(name, _Counter, unit, description) if status.conflict: # FIXME #2558 go through all views here and check if this @@ -124,10 +120,10 @@ def create_counter( return instrument def create_up_down_counter( - self, name, unit="", description="", advisory=None + self, name, unit="", description="" ) -> APIUpDownCounter: status = self._register_instrument( - name, _UpDownCounter, unit, description, advisory + name, _UpDownCounter, unit, description ) if status.conflict: @@ -158,10 +154,14 @@ def create_up_down_counter( return instrument def create_observable_counter( - self, name, callbacks=None, unit="", description="", advisory=None + self, + name, + callbacks=None, + unit="", + description="", ) -> APIObservableCounter: status = self._register_instrument( - name, _ObservableCounter, unit, description, advisory + name, _ObservableCounter, unit, description ) if status.conflict: @@ -199,27 +199,34 @@ def create_histogram( name: str, unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> APIHistogram: - if advisory is not None: + # FIXME: should pack everything into the class instead? + if explicit_bucket_boundaries_advisory is not None: invalid_advisory = False try: - boundaries = advisory.explicit_bucket_boundaries invalid_advisory = not ( - boundaries - and all(isinstance(e, (float, int)) for e in boundaries) + explicit_bucket_boundaries_advisory + and all( + isinstance(e, (float, int)) + for e in explicit_bucket_boundaries_advisory + ) ) except (KeyError, TypeError): invalid_advisory = True if invalid_advisory: - advisory = None + explicit_bucket_boundaries_advisory = None _logger.warning( - "Advisory must be a valid MetricsHistogramAdvisory with explicit_bucket_boundaries key containing a sequence of numbers" + "explicit_bucket_boundaries_advisory must be a sequence of numbers" ) status = self._register_instrument( - name, _Histogram, unit, description, advisory + name, + _Histogram, + unit, + description, + explicit_bucket_boundaries_advisory, ) if status.conflict: @@ -243,18 +250,14 @@ def create_histogram( self._measurement_consumer, unit, description, - advisory, + explicit_bucket_boundaries_advisory, ) with self._instrument_id_instrument_lock: self._instrument_id_instrument[status.instrument_id] = instrument return instrument - def create_gauge( - self, name, unit="", description="", advisory=None - ) -> APIGauge: - status = self._register_instrument( - name, _Gauge, unit, description, advisory - ) + def create_gauge(self, name, unit="", description="") -> APIGauge: + status = self._register_instrument(name, _Gauge, unit, description) if status.conflict: # FIXME #2558 go through all views here and check if this @@ -284,10 +287,10 @@ def create_gauge( return instrument def create_observable_gauge( - self, name, callbacks=None, unit="", description="", advisory=None + self, name, callbacks=None, unit="", description="" ) -> APIObservableGauge: status = self._register_instrument( - name, _ObservableGauge, unit, description, advisory + name, _ObservableGauge, unit, description ) if status.conflict: @@ -321,10 +324,10 @@ def create_observable_gauge( return instrument def create_observable_up_down_counter( - self, name, callbacks=None, unit="", description="", advisory=None + self, name, callbacks=None, unit="", description="" ) -> APIObservableUpDownCounter: status = self._register_instrument( - name, _ObservableUpDownCounter, unit, description, advisory + name, _ObservableUpDownCounter, unit, description ) if status.conflict: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index ea78b35e2f2..e3f0cd897f9 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1284,8 +1284,8 @@ def _create_aggregation( if isinstance(instrument, Histogram): boundaries: Optional[Sequence[float]] = ( - instrument._advisory.explicit_bucket_boundaries - if instrument._advisory is not None + instrument._explicit_bucket_boundaries_advisory + if instrument._explicit_bucket_boundaries_advisory is not None else None ) return _ExplicitBucketHistogramAggregation( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index ee071e3f2c3..db31ec1baba 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -16,12 +16,12 @@ from logging import getLogger from time import time_ns -from typing import Dict, Generator, Iterable, List, Optional, Union +from typing import Dict, Generator, Iterable, List, Optional, Sequence, Union # This kind of import is needed to avoid Sphinx errors. import opentelemetry.sdk.metrics from opentelemetry.context import Context, get_current -from opentelemetry.metrics import CallbackT, MetricsHistogramAdvisory +from opentelemetry.metrics import CallbackT from opentelemetry.metrics import Counter as APICounter from opentelemetry.metrics import Histogram as APIHistogram from opentelemetry.metrics import ObservableCounter as APIObservableCounter @@ -226,7 +226,7 @@ def __init__( measurement_consumer: "opentelemetry.sdk.metrics.MeasurementConsumer", unit: str = "", description: str = "", - advisory: Optional[MetricsHistogramAdvisory] = None, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ): super().__init__( name, @@ -235,7 +235,10 @@ def __init__( instrumentation_scope=instrumentation_scope, measurement_consumer=measurement_consumer, ) - self._advisory = advisory + # FIXME: should the dataclass instead? + self._explicit_bucket_boundaries_advisory = ( + explicit_bucket_boundaries_advisory + ) def __new__(cls, *args, **kwargs): if cls is Histogram: diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 0526734e647..19661a7ab4d 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -14,7 +14,6 @@ from unittest import TestCase -from opentelemetry.metrics import MetricsHistogramAdvisory from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import InMemoryMetricReader from opentelemetry.sdk.metrics.view import ( @@ -32,9 +31,7 @@ def test_default(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[1.0, 2.0, 3.0] - ), + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -62,9 +59,7 @@ def test_view_default_aggregation(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[1.0, 2.0, 3.0] - ), + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) @@ -97,9 +92,7 @@ def test_view_overrides_buckets(self): meter = meter_provider.get_meter("testmeter") histogram = meter.create_histogram( "testhistogram", - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[1.0, 2.0, 3.0] - ), + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], ) histogram.record(1, {"label": "value"}) histogram.record(2, {"label": "value"}) diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 2aab708efa0..0bee8b3c180 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -21,7 +21,6 @@ from unittest.mock import Mock from opentelemetry.context import Context -from opentelemetry.metrics import MetricsHistogramAdvisory from opentelemetry.sdk.metrics._internal.aggregation import ( _ExplicitBucketHistogramAggregation, _LastValueAggregation, @@ -636,9 +635,7 @@ def test_histogram_with_advisory(self): "name", Mock(), Mock(), - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=boundaries - ), + explicit_bucket_boundaries_advisory=boundaries, ), Mock(), _default_reservoir_factory, diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index ef704d6d7e9..85ac0d92b04 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -21,7 +21,7 @@ from unittest.mock import MagicMock, Mock, patch from opentelemetry.attributes import BoundedAttributes -from opentelemetry.metrics import MetricsHistogramAdvisory, NoOpMeter +from opentelemetry.metrics import NoOpMeter from opentelemetry.sdk.environment_variables import OTEL_SDK_DISABLED from opentelemetry.sdk.metrics import ( Counter, @@ -470,39 +470,16 @@ def test_repeated_instrument_names(self, logger_mock): def test_repeated_instrument_names_with_different_advisory(self): with self.assertNotRaises(Exception): - self.meter.create_counter("counter") - self.meter.create_up_down_counter("up_down_counter") - self.meter.create_observable_counter( - "observable_counter", callbacks=[Mock()] - ) - self.meter.create_histogram("histogram") - self.meter.create_gauge("gauge") - self.meter.create_observable_gauge( - "observable_gauge", callbacks=[Mock()] - ) - self.meter.create_observable_up_down_counter( - "observable_up_down_counter", callbacks=[Mock()] + self.meter.create_histogram( + "histogram", explicit_bucket_boundaries_advisory=[1.0] ) for instrument_name in [ - "counter", - "up_down_counter", "histogram", - "gauge", - ]: - with self.assertLogs(level=WARNING): - getattr(self.meter, f"create_{instrument_name}")( - instrument_name, advisory=Mock() - ) - - for instrument_name in [ - "observable_counter", - "observable_gauge", - "observable_up_down_counter", ]: with self.assertLogs(level=WARNING): getattr(self.meter, f"create_{instrument_name}")( - instrument_name, callbacks=[Mock()], advisory=Mock() + instrument_name ) def test_create_counter(self): @@ -542,23 +519,20 @@ def test_create_histogram_with_advisory(self): "name", unit="unit", description="description", - advisory=MetricsHistogramAdvisory( - explicit_bucket_boundaries=[0.0, 1.0, 2] - ), + explicit_bucket_boundaries_advisory=[0.0, 1.0, 2], ) self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( - histogram._advisory, - MetricsHistogramAdvisory(explicit_bucket_boundaries=[0.0, 1.0, 2]), + histogram._explicit_bucket_boundaries_advisory, + [0.0, 1.0, 2], ) def test_create_histogram_advisory_validation(self): advisories = [ - MetricsHistogramAdvisory(explicit_bucket_boundaries=None), - MetricsHistogramAdvisory(explicit_bucket_boundaries=[]), - MetricsHistogramAdvisory(explicit_bucket_boundaries=["1"]), + {"explicit_bucket_boundaries_advisory": []}, + {"explicit_bucket_boundaries_advisory": ["1"]}, ] for advisory in advisories: with self.subTest(advisory=advisory): @@ -567,7 +541,7 @@ def test_create_histogram_advisory_validation(self): "name", unit="unit", description="description", - advisory=advisory, + **advisory, ) def test_create_observable_gauge(self): From 6a8b8a74da566e5632a2b5c4cd2541474312a104 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 27 Jan 2025 10:36:08 +0100 Subject: [PATCH 36/41] Keep using _MetricsHistogramAdvisory for storing advisory internally --- .../metrics/_internal/__init__.py | 18 ++++++++++-------- .../metrics/_internal/instrument.py | 3 +-- .../sdk/metrics/_internal/__init__.py | 1 - .../sdk/metrics/_internal/aggregation.py | 4 ++-- .../sdk/metrics/_internal/instrument.py | 10 ++++++---- .../tests/metrics/test_metrics.py | 2 +- 6 files changed, 20 insertions(+), 18 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index f808d42d923..d24268dd757 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -46,7 +46,7 @@ from logging import getLogger from os import environ from threading import Lock -from typing import Any, Dict, List, Optional, Sequence, Union, cast +from typing import Dict, List, Optional, Sequence, Union, cast from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER from opentelemetry.metrics._internal.instrument import ( @@ -65,6 +65,7 @@ ObservableGauge, ObservableUpDownCounter, UpDownCounter, + _MetricsHistogramAdvisory, _ProxyCounter, _ProxyGauge, _ProxyHistogram, @@ -185,7 +186,7 @@ class _InstrumentRegistrationStatus: instrument_id: str already_registered: bool conflict: bool - current_advisory: Optional[Any] + current_advisory: Optional[_MetricsHistogramAdvisory] class Meter(ABC): @@ -205,7 +206,9 @@ def __init__( self._name = name self._version = version self._schema_url = schema_url - self._instrument_ids: Dict[str, Optional[Any]] = {} + self._instrument_ids: Dict[ + str, Optional[_MetricsHistogramAdvisory] + ] = {} self._instrument_ids_lock = Lock() @property @@ -235,7 +238,7 @@ def _register_instrument( type_: type, unit: str, description: str, - advisory: Optional[Any] = None, + advisory: Optional[_MetricsHistogramAdvisory] = None, ) -> _InstrumentRegistrationStatus: """ Register an instrument with the name, type, unit and description as @@ -743,15 +746,14 @@ def create_histogram( explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Returns a no-op Histogram.""" - # FIXME: should be use a dataclass here instead for the advisory param? status = self._register_instrument( name, NoOpHistogram, unit, description, - { - "explicit_bucket_boundaries_advisory": explicit_bucket_boundaries_advisory - }, + _MetricsHistogramAdvisory( + explicit_bucket_boundaries=explicit_bucket_boundaries_advisory + ), ) if status.conflict: self._log_instrument_registration_conflict( diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 5b0d5156fb2..0d5ec951074 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -45,9 +45,8 @@ _unit_regex = re_compile(r"[\x00-\x7F]{0,63}") -# FIXME: remove this if we are not going to use it to store advisory in registered instruments @dataclass(frozen=True) -class MetricsHistogramAdvisory: +class _MetricsHistogramAdvisory: explicit_bucket_boundaries: Optional[Sequence[float]] = None diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 06e5a8875c6..e656dc30605 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -201,7 +201,6 @@ def create_histogram( description: str = "", explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> APIHistogram: - # FIXME: should pack everything into the class instead? if explicit_bucket_boundaries_advisory is not None: invalid_advisory = False try: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index e3f0cd897f9..96aff5e6823 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1284,8 +1284,8 @@ def _create_aggregation( if isinstance(instrument, Histogram): boundaries: Optional[Sequence[float]] = ( - instrument._explicit_bucket_boundaries_advisory - if instrument._explicit_bucket_boundaries_advisory is not None + instrument._advisory.explicit_bucket_boundaries + if instrument._advisory.explicit_bucket_boundaries is not None else None ) return _ExplicitBucketHistogramAggregation( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index db31ec1baba..e2f76865396 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -31,7 +31,10 @@ ) from opentelemetry.metrics import UpDownCounter as APIUpDownCounter from opentelemetry.metrics import _Gauge as APIGauge -from opentelemetry.metrics._internal.instrument import CallbackOptions +from opentelemetry.metrics._internal.instrument import ( + CallbackOptions, + _MetricsHistogramAdvisory, +) from opentelemetry.sdk.metrics._internal.measurement import Measurement from opentelemetry.sdk.util.instrumentation import InstrumentationScope @@ -235,9 +238,8 @@ def __init__( instrumentation_scope=instrumentation_scope, measurement_consumer=measurement_consumer, ) - # FIXME: should the dataclass instead? - self._explicit_bucket_boundaries_advisory = ( - explicit_bucket_boundaries_advisory + self._advisory = _MetricsHistogramAdvisory( + explicit_bucket_boundaries=explicit_bucket_boundaries_advisory ) def __new__(cls, *args, **kwargs): diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 85ac0d92b04..1afe005c35a 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -525,7 +525,7 @@ def test_create_histogram_with_advisory(self): self.assertIsInstance(histogram, Histogram) self.assertEqual(histogram.name, "name") self.assertEqual( - histogram._explicit_bucket_boundaries_advisory, + histogram._advisory.explicit_bucket_boundaries, [0.0, 1.0, 2], ) From a79030d643970c34cc0c2627ca09d26e2b62e02c Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 27 Jan 2025 11:10:08 +0100 Subject: [PATCH 37/41] Make explicit_bucket_boundaries_advisory kwargs only --- .../src/opentelemetry/metrics/_internal/__init__.py | 5 ++++- .../src/opentelemetry/sdk/metrics/_internal/__init__.py | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index d24268dd757..3c25d517066 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py @@ -430,6 +430,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Creates a :class:`~opentelemetry.metrics.Histogram` instrument @@ -578,6 +579,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: with self._lock: @@ -586,7 +588,7 @@ def create_histogram( name, unit, description, - explicit_bucket_boundaries_advisory, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, ) proxy = _ProxyHistogram( name, unit, description, explicit_bucket_boundaries_advisory @@ -743,6 +745,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Returns a no-op Histogram.""" diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index e656dc30605..2ca3511cce3 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -199,6 +199,7 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> APIHistogram: if explicit_bucket_boundaries_advisory is not None: From 89c8391fb378eeacb027456579b7c8233bb5f5da Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 27 Jan 2025 11:41:43 +0100 Subject: [PATCH 38/41] Fix wrong params in create_histogram --- opentelemetry-api/tests/metrics/test_meter.py | 1 + 1 file changed, 1 insertion(+) diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 4df0f939e03..5a7ef3bc8b2 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -47,6 +47,7 @@ def create_histogram( name, unit="", description="", + *, explicit_bucket_boundaries_advisory=None, ): super().create_histogram( From 95e77fbd52374553d9b96d5a28398533c28b2f06 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 28 Jan 2025 10:30:15 +0100 Subject: [PATCH 39/41] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3675179ec6..e2b1d9b9d3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [BREAKING] semantic-conventions: Remove `opentelemetry.semconv.attributes.network_attributes.NETWORK_INTERFACE_NAME` introduced by mistake in the wrong module. ([#4391](https://github.com/open-telemetry/opentelemetry-python/pull/4391)) -- Add support for explicit bucket_boundaries advisory for Histograms +- Add support for explicit bucket boundaries advisory for Histograms ([#4361](https://github.com/open-telemetry/opentelemetry-python/pull/4361)) - semantic-conventions: Bump to 1.30.0 ([#4337](https://github.com/open-telemetry/opentelemetry-python/pull/4397)) From 9ef6701636e2069605913205662c7b244a6743c8 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 28 Jan 2025 20:41:44 +0100 Subject: [PATCH 40/41] Update opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Co-authored-by: Aaron Abbott --- .../src/opentelemetry/sdk/metrics/_internal/aggregation.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 96aff5e6823..3e17b6d3f64 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1283,11 +1283,7 @@ def _create_aggregation( ) if isinstance(instrument, Histogram): - boundaries: Optional[Sequence[float]] = ( - instrument._advisory.explicit_bucket_boundaries - if instrument._advisory.explicit_bucket_boundaries is not None - else None - ) + boundaries = instrument._advisory.explicit_bucket_boundaries return _ExplicitBucketHistogramAggregation( attributes, reservoir_builder=reservoir_factory( From 3826348ea784396d3eb57590dcd91b0eee7398a4 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 28 Jan 2025 21:04:43 +0100 Subject: [PATCH 41/41] Consider empty bucket boundaries as valid --- .../sdk/metrics/_internal/__init__.py | 18 +++++++------- ...est_histogram_advisory_explicit_buckets.py | 24 +++++++++++++++++++ .../tests/metrics/test_metrics.py | 2 +- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 2ca3511cce3..faa0959fce2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -204,15 +204,17 @@ def create_histogram( ) -> APIHistogram: if explicit_bucket_boundaries_advisory is not None: invalid_advisory = False - try: - invalid_advisory = not ( - explicit_bucket_boundaries_advisory - and all( - isinstance(e, (float, int)) - for e in explicit_bucket_boundaries_advisory + if isinstance(explicit_bucket_boundaries_advisory, Sequence): + try: + invalid_advisory = not ( + all( + isinstance(e, (float, int)) + for e in explicit_bucket_boundaries_advisory + ) ) - ) - except (KeyError, TypeError): + except (KeyError, TypeError): + invalid_advisory = True + else: invalid_advisory = True if invalid_advisory: diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 19661a7ab4d..945abf572e2 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -49,6 +49,30 @@ def test_default(self): metric.data.data_points[0].explicit_bounds, (1.0, 2.0, 3.0) ) + def test_empty_buckets(self): + reader = InMemoryMetricReader() + meter_provider = MeterProvider( + metric_readers=[reader], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", + explicit_bucket_boundaries_advisory=[], + ) + histogram.record(1, {"label": "value"}) + histogram.record(2, {"label": "value"}) + histogram.record(3, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric.name, "testhistogram") + self.assertEqual(metric.data.data_points[0].explicit_bounds, ()) + def test_view_default_aggregation(self): reader = InMemoryMetricReader() view = View(instrument_name="testhistogram") diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 1afe005c35a..3991fd6e154 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -531,7 +531,7 @@ def test_create_histogram_with_advisory(self): def test_create_histogram_advisory_validation(self): advisories = [ - {"explicit_bucket_boundaries_advisory": []}, + {"explicit_bucket_boundaries_advisory": "hello"}, {"explicit_bucket_boundaries_advisory": ["1"]}, ] for advisory in advisories: