Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for metric instrument advisory attributes (per OTel Metrics API spec) by plumbing an _attributes_advisory parameter through the API and SDK instrument creation paths, and applying it during stream attribute selection when a View does not explicitly configure attribute_keys.
Changes:
- Introduces
_MetricsAdvisory(attributes=…, explicit_bucket_boundaries=…)as the unified advisory container and threads it through instrument construction/registration. - Applies advisory attribute filtering in
_ViewInstrumentMatchwhenView.attribute_keysis not set. - Adds unit + integration test coverage for advisory attribute behavior and invalid advisory handling; updates changelog entry.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| opentelemetry-sdk/tests/metrics/test_view_instrument_match.py | Adds unit tests validating attribute selection precedence between View attribute_keys and instrument advisory attributes. |
| opentelemetry-sdk/tests/metrics/test_aggregation.py | Updates histogram advisory test to pass the new _MetricsAdvisory container. |
| opentelemetry-sdk/tests/metrics/integration_test/test_attributes_advisory.py | New integration tests covering advisory filtering across sync/async instruments, View interactions, and invalid advisory warnings. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py | Adds _advisory storage on SDK instrument base classes and removes histogram-specific advisory init override. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py | Adjusts histogram aggregation boundary lookup to tolerate missing/non-histogram advisories. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py | Implements advisory-based attribute key filtering when View does not provide attribute_keys. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/init.py | Adds _attributes_advisory parameters to SDK Meter methods and validates/constructs _MetricsAdvisory for instruments. |
| opentelemetry-api/tests/metrics/test_meter.py | Updates API meter tests to include _attributes_advisory and verifies registration conflict behavior when advisory differs. |
| opentelemetry-api/tests/metrics/test_meter_provider.py | Updates proxy-meter forwarding assertions to include _attributes_advisory=None. |
| opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py | Introduces _MetricsAdvisory and threads _attributes_advisory through proxy instrument creation. |
| opentelemetry-api/src/opentelemetry/metrics/_internal/init.py | Extends Meter/NoOpMeter/proxy meter signatures to accept _attributes_advisory and includes advisory in registration/conflict tracking. |
| .changelog/5444.added | Adds changelog entry for advisory attributes support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
Pull request dashboard statusWaiting on maintainers · refreshed 2026-11-03 04:17 UTC Merge when ready. Status above doesn't look right?
|
|
/dashboard route:author So many PRs thank you. Please could the conflicts be resolved. |
|
@tammy-baylis-swi, |
…visory-attributes
…butes # Conflicts: # opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py
|
/dashboard route:maintainers |
|
@tammy-baylis-swi, |
|
/dashboard route:reviewers |
|
@herin049, this pull request was routed to reviewers. The handoff remains active across pushes until newer actionable human feedback arrives. Top-level feedback through this request will not return; unresolved review threads remain open. |
| name, | ||
| unit, | ||
| description, | ||
| _attributes_advisory=_attributes_advisory, |
There was a problem hiding this comment.
Small thing: the proxy meter now always passes _attributes_advisory to the real meter. A third-party Meter that doesn't accept it would raise a TypeError. Is that fine since it's a private param, or should we only pass it when it's set?
There was a problem hiding this comment.
This is something I should have called out in the PR description. Technically adding these private params make this a breaking changes for custom SDK implementations. Even a basic create_counter with the new private attributes will result in a runtime error if the underlying SDK implementation doesn't support it. E.g.
# TypeError if API implementation doesn't accept `_attributes_advisory`
counter = meter.create_counter("foo", _attributes_advisory=("fizz, buzz"))That being said, this might be acceptable as long as we call it out. We've done similar changes before (e.g. in #4361).
There was a problem hiding this comment.
I think this falls into the same category of breakage we encountered in #3710. Indeed, #4361 introduced a similar change, though i don’t recall any issues being reported. Since this PR affects all instrument types, the potential impact is broader than 4361. maybe Could we omit _attributes_advisory when it’s None to preserve compatibility with existing Meter implementations and adopt this strategy moving forward? A proper notice in the changelog would be nice either way
There was a problem hiding this comment.
However, the latter case is much more acceptable.
Could you say more? Is it just that it's not breaking until the caller actually starts passing the new param?
(BTW, I think we might want to consider making the signatures in the Logging API forward compatible to avoid this kind of problem)
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Is it just that it's not breaking until the caller actually starts passing the new param?
Yes exactly, say I install opentelemetry-api==1.46.0 I see that I can use the new _attributes_advisory (the signature says I can). The ProxyMeter will initially accept it, but once the true Meter is resolved, it will crash.
counter = meter.create_counter("foo", _attributes_advisory=("fizz, buzz"))This is more acceptable in my eyes, and should be very rare in practice. This can't happen between the API and SDK for example because we pin the API version we use (i.e. API and SDK are in lockstep). This only would occur with third-party SDKs. And if you're using a third-party SDK, you will probably know not to use functionality which you haven't implemented in that third-party SDK anyway.
I've updated the PR to also only forward non-None additional keyword arguments in the ProxyMeter, this makes the more dangerous case impossible now.
There was a problem hiding this comment.
(BTW, I think we might want to consider making the signatures in the Logging API forward compatible to avoid this kind of problem)
I was thinking along similar lines 😆
| class HistogramOptions(InstrumentOptions): | ||
| """Optional keyword arguments accepted by ``Meter.create_histogram``.""" | ||
|
|
||
| explicit_bucket_boundaries_advisory: NotRequired[Sequence[float]] |
There was a problem hiding this comment.
Nit: Should the typing be NotRequired[Sequence[float]] | None to accept None values as well, just to be consistent with the previous typing?
There was a problem hiding this comment.
Unless we want to allow type checking to pass when users manually use explicit_bucket_boundaries_advisory=None, this should be correct. NotRequired[T] allows omitting the key from the kwargs dict, but if it is provided it must be of type T. If we want to allow the prior case, we should make it NotRequired[Sequence[float] | None].
There was a problem hiding this comment.
LGTM! I audited the exemplar behavior and it correctly preserves dropped attributes in filtered_attributes, which aligns with open-telemetry/opentelemetry-specification#5392.
Description
Adds support for Advisory Attributes metric instrument parameter.
Fixes #5443
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: