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

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!

aabmass · 2025-01-15T14:59:59Z

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

@@ -56,3 +55,7 @@
    ],
    ...,
 ]
+
+
+class MetricsInstrumentAdvisory(TypedDict):


Did you consider using a dataclass instead of typed dict? IMO dataclass is a little cleaner and has validation for users not using typing, but maybe there are some tradeoffs

Nope, I'll fix the other comments and then look at this

aabmass · 2025-01-15T15:17:53Z

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

+            if raise_error:
+                raise ValueError(
+                    "Advisory must be a dict with explicit_bucket_boundaries key containing a sequence of numbers"
+                )


Copied from the spec, emphasis mine

When a Meter creates an instrument, it SHOULD validate the instrument advisory parameters. If an advisory parameter is not valid, the Meter SHOULD emit an error notifying the user and proceed as if the parameter was not provided.

Corrected, thanks!

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?

aabmass · 2025-01-15T15:26:20Z

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

+            instrumentation_scope=instrumentation_scope,
+            measurement_consumer=measurement_consumer,
+        )
+        self.advisory = advisory


This is a new part of the public API right? Should we make it protected or do users have any reason to read it?

Made it private

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

opentelemetry-sdk/tests/metrics/test_aggregation.py

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

aabmass · 2025-01-15T15:40:03Z

CHANGELOG.md

Is this covered already?

If multiple identical Instruments are created with different advisory parameters, the Meter MUST return an instrument using the first-seen advisory parameters and log an appropriate error as described in duplicate instrument registrations.

Our duplicate instrument check is already wrong and printing the warning when two identical instruments are created instead of conflicting ones. I've a local branch updating it here

I see tests are red there, will fix and open a PR tomorrow

aabmass · 2025-01-15T15:42:43Z

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

Is this requirement covered when using Default aggregation?

Explicit Bucket Histogram Aggregation, with the ExplicitBucketBoundaries advisory parameter if provided

I think we use it already

aabmass · 2025-01-15T15:50:57Z

CHANGELOG.md

I also think we are missing tests for the new behavior. Would be nice to see integration tests for the cross product of

views/no views

advisory/no advisory

I can add tests for interaction with views for sure. What kind of integration tests are you thinking for advisory? I've added a test for create_histogram method that is what instrumentations are using for creating them.

Something high level like this test https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py. I guess just to capture the possible use cases and make sure we don't have unexpected behavior e.g.

User does nothing -> advisory buckets are used

User passes default aggregation to histogram -> advisory buckets are used

User passes ExplicitBucketHistogramAggregation -> view buckets are used

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

xrmx added 13 commits January 15, 2025 17:24

opentelemetry-api: add advisory parameters to Histograms

8572886

Update sdk

f1ea904

No need to use Sequence from collections

36b1c1c

Fix typing in proxy

9a03965

Rewrote MetricsInstrumentAdvisory as TypedDict

a759751

Rename ExplicitBucketBoundaries to explicit_bucket_boundaries

76bd525

Add changelog

a5d9906

Add an example in docs

859a146

parameter validation should just report an error, not fail

a7778ff

Make test reproducible

3d17403

Fix type of MetricsInstrumentAdvisory.explicit_bucket_boundaries

dd5adcf

Make advisory attribute private

3284c00

Permit also a sequence of ints as boundaries

c148618

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

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

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 15, 2025

aabmass Jan 15, 2025

xrmx Jan 15, 2025

aabmass Jan 15, 2025

aabmass Jan 15, 2025

xrmx Jan 15, 2025

aabmass Jan 15, 2025

xrmx Jan 15, 2025 •

edited

Loading

xrmx Jan 15, 2025

aabmass Jan 15, 2025

xrmx Jan 15, 2025

aabmass Jan 15, 2025

xrmx Jan 15, 2025 •

edited

Loading

aabmass Jan 15, 2025

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

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

Choose a reason for hiding this comment

xrmx Jan 15, 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

Choose a reason for hiding this comment

xrmx Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdneto commented Dec 19, 2024 •

edited

Loading

xrmx Jan 15, 2025 •

edited

Loading

xrmx Jan 15, 2025 •

edited

Loading