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

Add max_scale option #3323

Merged
merged 2 commits into from
Jun 13, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Unreleased

- Add max_scale option to Exponential Bucket Histogram Aggregation [#3323](https://github.com/open-telemetry/opentelemetry-python/pull/3323))
- Use BoundedAttributes instead of raw dict to extract attributes from LogRecord and Support dropped_attributes_count in LogRecord ([#3310](https://github.com/open-telemetry/opentelemetry-python/pull/3310))

## Version 1.18.0/0.39b0 (2023-05-04)

- Select histogram aggregation with an environment variable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ def __init__(
# See the derivation here:
# https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exponential-bucket-histogram-aggregation)
max_size: int = 160,
max_scale: int = 20,
):
super().__init__(attributes)
# max_size is the maximum capacity of the positive and negative
Expand All @@ -403,6 +404,7 @@ def __init__(
)

self._max_size = max_size
self._max_scale = max_scale

# _sum is the sum of all the values aggregated by this aggregator.
self._sum = 0
Expand All @@ -428,7 +430,14 @@ def __init__(

# _mapping corresponds to the current scale, is shared by both the
# positive and negative buckets.
self._mapping = LogarithmMapping(LogarithmMapping._max_scale)

if self._max_scale > 20:
_logger.warning(
"max_scale is set to %s which is "
"larger than the recommended value of 20",
self._max_scale,
)
self._mapping = LogarithmMapping(self._max_scale)

self._instrument_temporality = AggregationTemporality.DELTA
self._start_time_unix_nano = start_time_unix_nano
Expand Down Expand Up @@ -941,9 +950,10 @@ class ExponentialBucketHistogramAggregation(Aggregation):
def __init__(
self,
max_size: int = 160,
max_scale: int = 20,
):

self._max_size = max_size
self._max_scale = max_scale

def _create_aggregation(
self,
Expand All @@ -955,6 +965,7 @@ def _create_aggregation(
attributes,
start_time_unix_nano,
max_size=self._max_size,
max_scale=self._max_scale,
)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

from itertools import permutations
from logging import WARNING
from math import ldexp
from sys import float_info
from types import MethodType
Expand All @@ -37,6 +38,9 @@
LogarithmMapping,
)
from opentelemetry.sdk.metrics._internal.measurement import Measurement
from opentelemetry.sdk.metrics.view import (
ExponentialBucketHistogramAggregation,
)


def get_counts(buckets: Buckets) -> int:
Expand Down Expand Up @@ -77,6 +81,39 @@ def swap(


class TestExponentialBucketHistogramAggregation(TestCase):
@patch("opentelemetry.sdk.metrics._internal.aggregation.LogarithmMapping")
def test_create_aggregation(self, mock_logarithm_mapping):
exponential_bucket_histogram_aggregation = (
ExponentialBucketHistogramAggregation()
)._create_aggregation(Mock(), Mock(), Mock())

self.assertEqual(
exponential_bucket_histogram_aggregation._max_scale, 20
)

mock_logarithm_mapping.assert_called_with(20)

exponential_bucket_histogram_aggregation = (
ExponentialBucketHistogramAggregation(max_scale=10)
)._create_aggregation(Mock(), Mock(), Mock())

self.assertEqual(
exponential_bucket_histogram_aggregation._max_scale, 10
)

mock_logarithm_mapping.assert_called_with(10)

with self.assertLogs(level=WARNING):
exponential_bucket_histogram_aggregation = (
ExponentialBucketHistogramAggregation(max_scale=100)
)._create_aggregation(Mock(), Mock(), Mock())

self.assertEqual(
exponential_bucket_histogram_aggregation._max_scale, 100
)

mock_logarithm_mapping.assert_called_with(100)

def assertInEpsilon(self, first, second, epsilon):
self.assertLessEqual(first, (second * (1 + epsilon)))
self.assertGreaterEqual(first, (second * (1 - epsilon)))
Expand Down
Loading