diff --git a/CHANGELOG.md b/CHANGELOG.md index dd449ee0cc1..e2b1d9b9d3c 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)) - semantic-conventions: Bump to 1.30.0 ([#4337](https://github.com/open-telemetry/opentelemetry-python/pull/4397)) diff --git a/docs/examples/metrics/instruments/example.py b/docs/examples/metrics/instruments/example.py index fde20308a21..90a9f7fa234 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", + explicit_bucket_boundaries_advisory=[0.0, 1.0, 2.0], +) +histogram.record(99.9) + # Async Gauge gauge = meter.create_observable_gauge("gauge", [observable_gauge_func]) diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py index 84b4a6a8747..3c25d517066 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, Union, cast from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER from opentelemetry.metrics._internal.instrument import ( @@ -64,6 +65,7 @@ ObservableGauge, ObservableUpDownCounter, UpDownCounter, + _MetricsHistogramAdvisory, _ProxyCounter, _ProxyGauge, _ProxyHistogram, @@ -74,7 +76,9 @@ ) 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, +) _logger = getLogger(__name__) @@ -177,6 +181,14 @@ def on_set_meter_provider(self, meter_provider: MeterProvider) -> None: meter.on_set_meter_provider(meter_provider) +@dataclass +class _InstrumentRegistrationStatus: + instrument_id: str + already_registered: bool + conflict: bool + current_advisory: Optional[_MetricsHistogramAdvisory] + + class Meter(ABC): """Handles instrument creation. @@ -194,7 +206,9 @@ def __init__( self._name = name self._version = version self._schema_url = schema_url - self._instrument_ids: Set[str] = set() + self._instrument_ids: Dict[ + str, Optional[_MetricsHistogramAdvisory] + ] = {} self._instrument_ids_lock = Lock() @property @@ -218,31 +232,68 @@ 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[_MetricsHistogramAdvisory] = None, + ) -> _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 = False + conflict = 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) + self._instrument_ids[instrument_id] = advisory - return (result, instrument_id) + return _InstrumentRegistrationStatus( + instrument_id=instrument_id, + already_registered=already_registered, + conflict=conflict, + current_advisory=current_advisory, + ) + + @staticmethod + def _log_instrument_registration_conflict( + name: str, + instrumentation_type: str, + 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 " + "different advisory value %s and will be used instead.", + name, + instrumentation_type, + unit, + description, + status.current_advisory, + ) @abstractmethod def create_counter( @@ -379,6 +430,8 @@ 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 @@ -526,13 +579,20 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, + 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 + name, + unit, + description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, ) - proxy = _ProxyHistogram(name, unit, description) + proxy = _ProxyHistogram( + name, unit, description, explicit_bucket_boundaries_advisory + ) self._instruments.append(proxy) return proxy @@ -602,17 +662,18 @@ def create_counter( description: str = "", ) -> Counter: """Returns a no-op Counter.""" - if self._is_instrument_registered( + status = self._register_instrument( name, NoOpCounter, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + ) + if status.conflict: + self._log_instrument_registration_conflict( name, Counter.__name__, unit, description, + status, ) + return NoOpCounter(name, unit=unit, description=description) def create_gauge( @@ -622,16 +683,14 @@ def create_gauge( description: str = "", ) -> Gauge: """Returns a no-op Gauge.""" - if self._is_instrument_registered(name, NoOpGauge, unit, description)[ - 0 - ]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + status = self._register_instrument(name, NoOpGauge, unit, description) + if status.conflict: + self._log_instrument_registration_conflict( name, Gauge.__name__, unit, description, + status, ) return NoOpGauge(name, unit=unit, description=description) @@ -642,16 +701,16 @@ def create_up_down_counter( description: str = "", ) -> UpDownCounter: """Returns a no-op UpDownCounter.""" - if self._is_instrument_registered( + status = self._register_instrument( name, NoOpUpDownCounter, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + ) + if status.conflict: + self._log_instrument_registration_conflict( name, UpDownCounter.__name__, unit, description, + status, ) return NoOpUpDownCounter(name, unit=unit, description=description) @@ -663,16 +722,16 @@ def create_observable_counter( description: str = "", ) -> ObservableCounter: """Returns a no-op ObservableCounter.""" - if self._is_instrument_registered( + status = self._register_instrument( name, NoOpObservableCounter, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + ) + if status.conflict: + self._log_instrument_registration_conflict( name, ObservableCounter.__name__, unit, description, + status, ) return NoOpObservableCounter( name, @@ -686,20 +745,33 @@ def create_histogram( name: str, unit: str = "", description: str = "", + *, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> Histogram: """Returns a no-op Histogram.""" - if self._is_instrument_registered( - name, NoOpHistogram, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + status = self._register_instrument( + name, + NoOpHistogram, + unit, + description, + _MetricsHistogramAdvisory( + explicit_bucket_boundaries=explicit_bucket_boundaries_advisory + ), + ) + if status.conflict: + self._log_instrument_registration_conflict( name, Histogram.__name__, unit, description, + status, ) - return NoOpHistogram(name, unit=unit, description=description) + return NoOpHistogram( + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, + ) def create_observable_gauge( self, @@ -709,16 +781,16 @@ def create_observable_gauge( description: str = "", ) -> ObservableGauge: """Returns a no-op ObservableGauge.""" - if self._is_instrument_registered( + status = self._register_instrument( name, NoOpObservableGauge, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + ) + if status.conflict: + self._log_instrument_registration_conflict( name, ObservableGauge.__name__, unit, description, + status, ) return NoOpObservableGauge( name, @@ -735,16 +807,16 @@ def create_observable_up_down_counter( description: str = "", ) -> ObservableUpDownCounter: """Returns a no-op ObservableUpDownCounter.""" - if self._is_instrument_registered( + status = self._register_instrument( name, NoOpObservableUpDownCounter, unit, description - )[0]: - _logger.warning( - "An instrument with name %s, type %s, unit %s and " - "description %s has been created already.", + ) + if status.conflict: + self._log_instrument_registration_conflict( name, ObservableUpDownCounter.__name__, unit, description, + status, ) return NoOpObservableUpDownCounter( name, diff --git a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py index 2250c8b6fdd..0d5ec951074 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py +++ b/opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py @@ -35,7 +35,9 @@ 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, +) _logger = getLogger(__name__) @@ -43,6 +45,11 @@ _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 CallbackOptions: """Options for the callback @@ -210,7 +217,11 @@ 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, + ) class UpDownCounter(Synchronous): @@ -258,7 +269,9 @@ 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, ) @@ -278,7 +291,12 @@ def __init__( unit: str = "", description: str = "", ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + ) class _ProxyObservableCounter( @@ -288,7 +306,10 @@ 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, ) @@ -309,7 +330,12 @@ def __init__( unit: str = "", description: str = "", ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + ) class _ProxyObservableUpDownCounter( @@ -320,7 +346,10 @@ 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, ) @@ -330,6 +359,16 @@ class Histogram(Synchronous): histograms, summaries, and percentile. """ + @abstractmethod + def __init__( + self, + name: str, + unit: str = "", + description: str = "", + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, + ) -> None: + pass + @abstractmethod def record( self, @@ -348,8 +387,14 @@ def __init__( name: str, unit: str = "", description: str = "", + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, ) -> None: - super().__init__(name, unit=unit, description=description) + super().__init__( + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, + ) def record( self, @@ -361,6 +406,18 @@ def record( class _ProxyHistogram(_ProxyInstrument[Histogram], Histogram): + def __init__( + self, + name: str, + unit: str = "", + description: str = "", + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, + ) -> None: + super().__init__(name, unit=unit, description=description) + self._explicit_bucket_boundaries_advisory = ( + explicit_bucket_boundaries_advisory + ) + def record( self, amount: Union[int, float], @@ -372,7 +429,10 @@ 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, + explicit_bucket_boundaries_advisory=self._explicit_bucket_boundaries_advisory, ) @@ -393,7 +453,12 @@ def __init__( unit: str = "", description: str = "", ) -> None: - super().__init__(name, callbacks, unit=unit, description=description) + super().__init__( + name, + callbacks, + unit=unit, + description=description, + ) class _ProxyObservableGauge( @@ -404,7 +469,10 @@ 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, ) @@ -455,4 +523,8 @@ 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, + ) diff --git a/opentelemetry-api/src/opentelemetry/util/types.py b/opentelemetry-api/src/opentelemetry/util/types.py index d9490ff08c9..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 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. diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 4fb9e047b60..5a7ef3bc8b2 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 @@ -36,11 +36,26 @@ def create_observable_counter( self, name, callbacks, unit="", description="" ): super().create_observable_counter( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, ) - def create_histogram(self, name, unit="", description=""): - super().create_histogram(name, unit=unit, description=description) + def create_histogram( + self, + name, + unit="", + description="", + *, + explicit_bucket_boundaries_advisory=None, + ): + super().create_histogram( + name, + unit=unit, + description=description, + explicit_bucket_boundaries_advisory=explicit_bucket_boundaries_advisory, + ) def create_gauge(self, name, unit="", description=""): super().create_gauge(name, unit=unit, description=description) @@ -49,20 +64,28 @@ def create_observable_gauge( self, name, callbacks, unit="", description="" ): super().create_observable_gauge( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, ) def create_observable_up_down_counter( self, name, callbacks, unit="", description="" ): super().create_observable_up_down_counter( - name, callbacks, unit=unit, description=description + name, + callbacks, + unit=unit, + description=description, ) class TestMeter(TestCase): # pylint: disable=no-member - def test_repeated_instrument_names(self): + # 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): try: test_meter = NoOpMeter("name") @@ -84,19 +107,35 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertLogs(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", + ]: + 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: + test_meter = NoOpMeter("name") + + 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 [ + "histogram", ]: with self.assertLogs(level=WARNING): getattr(test_meter, f"create_{instrument_name}")( - instrument_name, 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 6f1989e208a..dfaf94bcec2 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, explicit_bucket_boundaries_advisory=None ) real_meter.create_gauge.assert_called_once_with( name, unit, description diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 334dd01b10a..faa0959fce2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -11,6 +11,7 @@ # 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 logging import getLogger @@ -63,7 +64,9 @@ 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, +) _logger = getLogger(__name__) @@ -87,25 +90,22 @@ def __init__( self._instrument_id_instrument_lock = Lock() def create_counter(self, name, unit="", description="") -> APICounter: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered(name, _Counter, unit, description) + status = self._register_instrument(name, _Counter, unit, description) - 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.", + self._log_instrument_registration_conflict( name, APICounter.__name__, unit, description, + status, ) + 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, @@ -116,33 +116,30 @@ def create_counter(self, name, unit="", description="") -> APICounter: ) 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="" ) -> APIUpDownCounter: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered( + status = self._register_instrument( name, _UpDownCounter, unit, description ) - 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.", + self._log_instrument_registration_conflict( name, APIUpDownCounter.__name__, unit, description, + status, ) + 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, @@ -153,33 +150,34 @@ 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="" + self, + name, + callbacks=None, + unit="", + description="", ) -> APIObservableCounter: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered( + status = self._register_instrument( name, _ObservableCounter, unit, description ) - 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.", + self._log_instrument_registration_conflict( name, APIObservableCounter.__name__, unit, description, + status, ) + 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, @@ -193,29 +191,60 @@ 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(self, name, unit="", description="") -> APIHistogram: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered(name, _Histogram, unit, description) + def create_histogram( + self, + name: str, + unit: str = "", + description: str = "", + *, + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, + ) -> APIHistogram: + if explicit_bucket_boundaries_advisory is not None: + invalid_advisory = False + 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): + invalid_advisory = True + else: + invalid_advisory = True + + if invalid_advisory: + explicit_bucket_boundaries_advisory = None + _logger.warning( + "explicit_bucket_boundaries_advisory must be a sequence of numbers" + ) + + status = self._register_instrument( + name, + _Histogram, + unit, + description, + explicit_bucket_boundaries_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.", + self._log_instrument_registration_conflict( name, APIHistogram.__name__, unit, description, + status, ) + 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, @@ -223,31 +252,29 @@ def create_histogram(self, name, unit="", description="") -> APIHistogram: self._measurement_consumer, unit, description, + explicit_bucket_boundaries_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="") -> APIGauge: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered(name, _Gauge, unit, description) + status = self._register_instrument(name, _Gauge, unit, description) - 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.", + self._log_instrument_registration_conflict( name, APIGauge.__name__, unit, description, + status, ) + 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, @@ -258,33 +285,30 @@ def create_gauge(self, name, unit="", description="") -> APIGauge: ) 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="" ) -> APIObservableGauge: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered( + status = self._register_instrument( name, _ObservableGauge, unit, description ) - 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.", + self._log_instrument_registration_conflict( name, APIObservableGauge.__name__, unit, description, + status, ) + 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, @@ -298,33 +322,30 @@ 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="" ) -> APIObservableUpDownCounter: - ( - is_instrument_registered, - instrument_id, - ) = self._is_instrument_registered( + status = self._register_instrument( name, _ObservableUpDownCounter, unit, description ) - 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.", + self._log_instrument_registration_conflict( name, APIObservableUpDownCounter.__name__, unit, description, + status, ) + 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, @@ -338,7 +359,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 diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index a579e3072e9..3e17b6d3f64 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -21,7 +21,15 @@ 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, +) from opentelemetry.metrics import ( Asynchronous, @@ -437,6 +445,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 +471,13 @@ 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 +1283,7 @@ def _create_aggregation( ) if isinstance(instrument, Histogram): + boundaries = instrument._advisory.explicit_bucket_boundaries return _ExplicitBucketHistogramAggregation( attributes, reservoir_builder=reservoir_factory( @@ -1276,6 +1292,7 @@ def _create_aggregation( instrument_aggregation_temporality=( AggregationTemporality.DELTA ), + boundaries=boundaries, start_time_unix_nano=start_time_unix_nano, ) @@ -1347,25 +1364,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..e2f76865396 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -16,7 +16,7 @@ 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 @@ -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 @@ -219,6 +222,26 @@ 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 = "", + explicit_bucket_boundaries_advisory: Optional[Sequence[float]] = None, + ): + super().__init__( + name, + unit=unit, + description=description, + instrumentation_scope=instrumentation_scope, + measurement_consumer=measurement_consumer, + ) + self._advisory = _MetricsHistogramAdvisory( + explicit_bucket_boundaries=explicit_bucket_boundaries_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/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..945abf572e2 --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -0,0 +1,135 @@ +# 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", + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], + ) + 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.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") + meter_provider = MeterProvider( + metric_readers=[reader], + views=[view], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], + ) + 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.0, 2.0, 3.0) + ) + + def test_view_overrides_buckets(self): + reader = InMemoryMetricReader() + view = View( + instrument_name="testhistogram", + aggregation=ExplicitBucketHistogramAggregation( + boundaries=[10.0, 100.0, 1000.0] + ), + ) + meter_provider = MeterProvider( + metric_readers=[reader], + views=[view], + ) + meter = meter_provider.get_meter("testmeter") + histogram = meter.create_histogram( + "testhistogram", + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], + ) + 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.0, 100.0, 1000.0) + ) diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 4a625908afb..0bee8b3c180 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -628,6 +628,22 @@ def test_histogram(self): ) self.assertIsInstance(aggregation, _ExplicitBucketHistogramAggregation) + def test_histogram_with_advisory(self): + boundaries = [1.0, 2.0, 3.0] + aggregation = self.default_aggregation._create_aggregation( + _Histogram( + "name", + Mock(), + Mock(), + explicit_bucket_boundaries_advisory=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..3991fd6e154 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,19 +455,31 @@ def test_repeated_instrument_names(self): "histogram", "gauge", ]: - with self.assertLogs(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", + ]: + 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): + self.meter.create_histogram( + "histogram", explicit_bucket_boundaries_advisory=[1.0] + ) + + for instrument_name in [ + "histogram", ]: with self.assertLogs(level=WARNING): getattr(self.meter, f"create_{instrument_name}")( - instrument_name, callbacks=[Mock()] + instrument_name ) def test_create_counter(self): @@ -500,6 +514,36 @@ 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", + explicit_bucket_boundaries_advisory=[0.0, 1.0, 2], + ) + + self.assertIsInstance(histogram, Histogram) + self.assertEqual(histogram.name, "name") + self.assertEqual( + histogram._advisory.explicit_bucket_boundaries, + [0.0, 1.0, 2], + ) + + def test_create_histogram_advisory_validation(self): + advisories = [ + {"explicit_bucket_boundaries_advisory": "hello"}, + {"explicit_bucket_boundaries_advisory": ["1"]}, + ] + for advisory in advisories: + with self.subTest(advisory=advisory): + with self.assertLogs(level=WARNING): + self.meter.create_histogram( + "name", + unit="unit", + description="description", + **advisory, + ) + def test_create_observable_gauge(self): observable_gauge = self.meter.create_observable_gauge( "name", callbacks=[Mock()], unit="unit", description="description" @@ -589,10 +633,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" )