Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ExplicitBucketBoundaries advisory for Histograms #4361

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#4364](https://github.com/open-telemetry/opentelemetry-python/pull/4364))
- Add Python 3.13 support
([#4353](https://github.com/open-telemetry/opentelemetry-python/pull/4353))
- 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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -379,6 +379,7 @@ def create_histogram(
name: str,
unit: str = "",
description: str = "",
advisory: Optional[MetricsInstrumentAdvisory] = None,
) -> Histogram:
"""Creates a :class:`~opentelemetry.metrics.Histogram` instrument

Expand Down Expand Up @@ -526,13 +527,14 @@ def create_histogram(
name: str,
unit: str = "",
description: str = "",
advisory: Optional[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

Expand Down Expand Up @@ -686,6 +688,7 @@ def create_histogram(
name: str,
unit: str = "",
description: str = "",
advisory: Optional[MetricsInstrumentAdvisory] = None,
) -> Histogram:
"""Returns a no-op Histogram."""
if self._is_instrument_registered(
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -330,6 +330,16 @@ class Histogram(Synchronous):
histograms, summaries, and percentile.
"""

@abstractmethod
def __init__(
self,
name: str,
unit: str = "",
description: str = "",
advisory: Optional[MetricsInstrumentAdvisory] = None,
) -> None:
pass

@abstractmethod
def record(
self,
Expand All @@ -348,8 +358,11 @@ def __init__(
name: str,
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
)

def record(
self,
Expand All @@ -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: Optional[MetricsInstrumentAdvisory] = None,
) -> None:
self._name = name
self._unit = unit
self._description = description
self._advisory = advisory
self._real_instrument: Optional[Histogram] = None

def record(
self,
amount: Union[int, float],
Expand All @@ -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
)


Expand Down
7 changes: 5 additions & 2 deletions opentelemetry-api/src/opentelemetry/util/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 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:
Expand Down Expand Up @@ -56,3 +55,7 @@
],
...,
]


class MetricsInstrumentAdvisory(TypedDict):
explicit_bucket_boundaries: Optional[AnyValue]
2 changes: 1 addition & 1 deletion opentelemetry-api/tests/metrics/test_meter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=""):
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-api/tests/metrics/test_meter_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -63,7 +64,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__)

Expand Down Expand Up @@ -196,7 +197,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: Optional[MetricsInstrumentAdvisory] = None,
) -> APIHistogram:
if advisory is not None:
raise_error = False
try:
boundaries = advisory["explicit_bucket_boundaries"]
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 explicit_bucket_boundaries key containing a sequence of numbers"
)

(
is_instrument_registered,
instrument_id,
Expand All @@ -223,6 +246,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,32 +437,40 @@ 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,
attributes: Attributes,
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(
Expand Down Expand Up @@ -1268,6 +1276,11 @@ def _create_aggregation(
)

if isinstance(instrument, Histogram):
boundaries: Optional[Sequence[float]] = (
instrument.advisory.get("explicit_bucket_boundaries")
if instrument.advisory is not None
else None
)
return _ExplicitBucketHistogramAggregation(
attributes,
reservoir_builder=reservoir_factory(
Expand All @@ -1276,6 +1289,7 @@ def _create_aggregation(
instrument_aggregation_temporality=(
AggregationTemporality.DELTA
),
boundaries=boundaries,
start_time_unix_nano=start_time_unix_nano,
)

Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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: Optional[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.")
Expand Down
17 changes: 17 additions & 0 deletions opentelemetry-sdk/tests/metrics/test_aggregation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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={"explicit_bucket_boundaries": 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(
Expand Down
Loading
Loading