Implement ExplicitBucketBoundaries advisory for Histograms #4361

xrmx · 2024-12-17T10:38:43Z

Description

This adds basic support for the advisory attribute of Instruments and implements ExplicitBucketBoundaries advisory for Histograms.

Fixes #4140
Fixes #3042

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

emdneto

Thanks. Any clue on the docs CI error? I would say to add to nitpick_ignore.

xrmx · 2024-12-19T10:32:17Z

Thanks. Any clue on the docs CI error? I would say to add to nitpick_ignore.

Nope, it's really hard for me to understand where this is coming from. Tried using "AnyValue" in util/types.py but does not change anything.

opentelemetry-api/src/opentelemetry/util/types.py

xrmx · 2024-12-19T16:04:55Z

Appear to work fine with the flask implementation after updating the create_histogram calls for HTTP_SERVER_REQUEST_DURATION:

                        {
                            "name": "http.server.request.duration",
                            "description": "Duration of HTTP server requests.",
                            "unit": "s",
                            "data": {
                                "data_points": [
                                    {
                                        "attributes": {
                                            "http.request.method": "GET",
                                            "url.scheme": "http",
                                            "network.protocol.version": "1.1",
                                            "http.response.status_code": 200,
                                            "http.route": "/rolldice"
                                        },
                                        "start_time_unix_nano": 1734623881505049269,
                                        "time_unix_nano": 1734624051781113265,
                                        "count": 9,
                                        "sum": 0.00882425531744957,
                                        "bucket_counts": [
                                            9,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0,
                                            0
                                        ],
                                        "explicit_bounds": [
                                            0.005,
                                            0.01,
                                            0.025,
                                            0.05,
                                            0.075,
                                            0.1,
                                            0.25,
                                            0.5,
                                            0.75,
                                            1,
                                            2.5,
                                            5,
                                            7.5,
                                            10
                                        ],
                                        "min": 0.0004944326356053352,
                                        "max": 0.001611161045730114,
                                        "exemplars": []
                                    }
                                ],
                                "aggregation_temporality": 2
                            }
                        }

emdneto · 2024-12-19T16:13:14Z

Thanks. Any clue on the docs CI error? I would say to add to nitpick_ignore.

Nope, it's really hard for me to understand where this is coming from. Tried using "AnyValue" in util/types.py but does not change anything.

We can probably use a TypedDict or just add ("py:class", "AnyValue"), to nitpick_ignore and see if it works:

diff --git a/docs/conf.py b/docs/conf.py
index 965a806d..997b5784 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -96,6 +96,7 @@ nitpicky = True
 # Container supposedly were fixed, but does not work
 # /~https://github.com/sphinx-doc/sphinx/pull/3744
 nitpick_ignore = [
+    ("py:class", "AnyValue"),
     ("py:class", "ValueT"),
     ("py:class", "CarrierT"),
     ("py:obj", "opentelemetry.propagators.textmap.CarrierT"),

xrmx · 2024-12-19T17:00:12Z

Thanks. Any clue on the docs CI error? I would say to add to nitpick_ignore.

Nope, it's really hard for me to understand where this is coming from. Tried using "AnyValue" in util/types.py but does not change anything.

We can probably use a TypedDict or just add ("py:class", "AnyValue"), to nitpick_ignore and see if it works:

Moved to TypedDict, thanks for the hint!

opentelemetry-api/src/opentelemetry/util/types.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py

aabmass · 2025-01-15T15:24:45Z

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py

-            7500.0,
-            10000.0,
-        ),
+        boundaries: Optional[Sequence[float]] = None,


Any reason to not leave the default as _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES? I'm wondering because this allows explicitly passing None now

@aabmass Yeah, now you can pass None boundaries but then the defaults boundaries are used. Also this is more pythonic I guess?

What's the reason user would pass None instead of allowing the default?

There's no reason the user would pass None, I expected to just call it without passing the parameter as currently doing. But no big deal in keeping thing as is.

Well, it looks like we have someone passing None:

def __init__(self, boundaries: Sequence[float], **kwargs) -> None: > super().__init__(len(boundaries) + 1, **kwargs) E TypeError: object of type 'NoneType' has no len() opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py:296: TypeError

and

> self._boundaries = tuple(boundaries) E TypeError: 'NoneType' object is not iterable opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py:488: TypeError

I'm fine with whatever you want to do here

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py

opentelemetry-sdk/tests/metrics/test_aggregation.py

opentelemetry-api/src/opentelemetry/util/types.py

CHANGELOG.md

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py

CHANGELOG.md

opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py

aabmass · 2025-01-17T17:06:25Z

opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py


    @abstractmethod
    def create_counter(
        self,
        name: str,
        unit: str = "",
        description: str = "",
+        advisory: None = None,


Wdyt of adding doing this?

Suggested change

advisory: None = None,

*,

advisory: None = None,

to force any new arguments to be passed by name explicitly

getting to having a lot of arguments here and IMO it is less error prone for users

that would allow us to reorder the argument list if needed in the future

if we also added a trailing **kwargs, we could even remove parameters in the future if they get deprecated. Maybe lets discuss in the SIG but wanted to get your thoughts.

I'm fine with that

aabmass · 2025-01-17T17:14:23Z

opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py

+    # pylint: disable=super-init-not-called
+    def __init__(


Is it because of multiple inheritance?

I think this is because I haven't updated _ProxyInstrument and all the other instrument classes to get the advisory parameter.

Adding them broke mypy ofc 😓

Fixed in 0b6fc1f

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py

aabmass · 2025-01-17T17:39:28Z

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py

-            7500.0,
-            10000.0,
-        ),
+        boundaries: Optional[Sequence[float]] = None,


I'm fine with whatever you want to do here

First to take into account advisory and then to avoid printing warnings if there's not duplicate for already registered instrument.

…ill be used

aabmass

💯

aabmass · 2025-01-21T15:47:02Z

opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py

@@ -394,6 +451,7 @@ def create_gauge(  # type: ignore # pylint: disable=no-self-use
        name: str,
        unit: str = "",
        description: str = "",
+        advisory: Optional[MetricsCommonAdvisory] = None,


What if the spec adds specification adds a new advisory type for another instrument, say MetricsGaugeAdvisory? I think if we changed the type here, that would be a breaking change

So are you suggesting to add preemptively a custom type for each instrumenting or having the specific dataclass inherit from the common be enough? In other words, is the issue about the name of the type or its structure?

I don't mind so much as long as we avoid introducing breaking changes. I looked at Go and Java, they took more of a "flat" approach where advisory parameters are flattened into the builder or options type.

I guess the flat approach is for this very same reason of avoiding breaking compatibility by changing the layout of the advisory

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py

xrmx requested a review from a team as a code owner December 17, 2024 10:38

emdneto reviewed Dec 18, 2024

View reviewed changes

xrmx commented Dec 19, 2024

View reviewed changes

opentelemetry-api/src/opentelemetry/util/types.py Outdated Show resolved Hide resolved

emdneto added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Dec 19, 2024

xrmx force-pushed the histogram-advisory branch from 3c8b98e to 181596b Compare December 23, 2024 14:25

xrmx requested a review from emdneto December 23, 2024 14:29

emdneto approved these changes Dec 23, 2024

View reviewed changes

aabmass requested changes Jan 15, 2025

View reviewed changes

aabmass reviewed Jan 15, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

xrmx force-pushed the histogram-advisory branch from 22b40b0 to c148618 Compare January 15, 2025 16:36

aabmass reviewed Jan 17, 2025

View reviewed changes

xrmx force-pushed the histogram-advisory branch from e8c6a4d to d3a345f Compare January 20, 2025 10:32

xrmx added 14 commits January 21, 2025 14:53

opentelemetry-api: add advisory parameters to Histograms

26177de

Update sdk

ea2f57a

No need to use Sequence from collections

b96dfb4

Fix typing in proxy

575976e

Rewrote MetricsInstrumentAdvisory as TypedDict

e1b75ee

Rename ExplicitBucketBoundaries to explicit_bucket_boundaries

ef15047

Add changelog

97db117

Add an example in docs

9d746c6

parameter validation should just report an error, not fail

094d7e4

Make test reproducible

bf54940

Fix type of MetricsInstrumentAdvisory.explicit_bucket_boundaries

eb5be5b

Make advisory attribute private

eba20d9

Permit also a sequence of ints as boundaries

f333b2d

Rework conflicting instrument registration check

3a3ce19

First to take into account advisory and then to avoid printing warnings if there's not duplicate for already registered instrument.

xrmx added 15 commits January 21, 2025 14:53

Fix typing for api instrumentors

3b1ab28

Fixup lint

532f27a

Add integration tests

45a02de

Make explicit boundaries float only

ee7350a

Stop using assertNoLogs to have green tests on py < 3.10

27664ae

Fix api tests for real

b35e75c

Change interface to a dataclass instead of a typed dict

5d46308

Add default

c3c6194

Return instrument_id in _InstrumentRegistrationStatus

85be148

Don't use tuple assignment

97b2669

Correct advisory check to match reality

3e77f89

fixup typo

d3e029a

Centralize instrument registration conflict logging

91022e1

Add advisory parameter to all instrumentor constructor

1464313

Accept ints as valid value for advisory explicit bucket boundaries

005295a

xrmx force-pushed the histogram-advisory branch from d3a345f to 005295a Compare January 21, 2025 13:53

xrmx added 2 commits January 21, 2025 15:36

Please mypy again

0b6fc1f

Move Advisory types to metrics _internal and export the public one

5cd650a

xrmx force-pushed the histogram-advisory branch from 1372540 to 5cd650a Compare January 21, 2025 14:53

Give an hint on instrument registration conflict that the other one w…

027da73

…ill be used

aabmass approved these changes Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ExplicitBucketBoundaries advisory for Histograms #4361

Implement ExplicitBucketBoundaries advisory for Histograms #4361

xrmx commented Dec 17, 2024 •

edited

Loading

emdneto left a comment

xrmx commented Dec 19, 2024

xrmx commented Dec 19, 2024

emdneto commented Dec 19, 2024 •

edited

Loading

xrmx commented Dec 19, 2024

aabmass Jan 15, 2025

xrmx Jan 15, 2025

aabmass Jan 15, 2025

xrmx Jan 16, 2025

xrmx Jan 17, 2025

aabmass Jan 17, 2025

aabmass Jan 17, 2025

xrmx Jan 20, 2025

aabmass Jan 17, 2025

xrmx Jan 20, 2025 •

edited

Loading

xrmx Jan 20, 2025

xrmx Jan 21, 2025

aabmass Jan 17, 2025

aabmass left a comment

aabmass Jan 21, 2025

xrmx Jan 21, 2025 •

edited

Loading

aabmass Jan 21, 2025

xrmx Jan 22, 2025 •

edited

Loading

Implement ExplicitBucketBoundaries advisory for Histograms #4361

Are you sure you want to change the base?

Implement ExplicitBucketBoundaries advisory for Histograms #4361

Conversation

xrmx commented Dec 17, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

emdneto left a comment

Choose a reason for hiding this comment

xrmx commented Dec 19, 2024

xrmx commented Dec 19, 2024

emdneto commented Dec 19, 2024 • edited Loading

xrmx commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabmass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

xrmx commented Dec 17, 2024 •

edited

Loading

emdneto commented Dec 19, 2024 •

edited

Loading

xrmx Jan 20, 2025 •

edited

Loading

xrmx Jan 21, 2025 •

edited

Loading

xrmx Jan 22, 2025 •

edited

Loading