feat: Explicitly define include/exclude attributes for views#4951
feat: Explicitly define include/exclude attributes for views#4951thompson-tomo wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
specification/metrics/api.md
Outdated
| `Attributes` (a list of [attribute keys](../common/README.md#attribute)) is | ||
| the recommended set of attribute keys to be used for the resulting metrics. | ||
|
|
||
| ##### Instrument advisory parameter: `Exclude_Attributes` |
There was a problem hiding this comment.
don't need this in the API/advisory. if something should not be used, then instrumentation can not provide it at all.
There was a problem hiding this comment.
Wouldn't opt-in/deprecated attributes reside in this advisory? That way if stream config doesn't specify included/excluded attributes the advisory can be used to exclude them with 0 zero config.
There was a problem hiding this comment.
Opt-in attributes are not emitted by instrumentation (because SDK respects it) by default — they only appear when the user explicitly opts in via Views. So there's nothing to exclude in the zero-config case.
For exclude_attributes, the instrumentation library itself should stop emitting them (or the semantic conventions should guide the migration). The fix belongs at the instrumentation layer, not as an SDK-side exclude hint.
The existing Attributes (include) advisory already covers the zero-config story: the instrumentation author lists the recommended attribute keys, and anything not on that list is implicitly not recommended.
dashpole
left a comment
There was a problem hiding this comment.
As @cijothomas points out, this is already supported by the Attributes advisory parameter.
|
Separately, I realized that Declarative Config already includes support for this - https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema/meter_provider.yaml#L605-L609 Spec wording is a bit hand-wavy/flexible, so I think it makes sense to make it more concrete. |
|
That is part of the SDK's View, not part of the API. |
bcc605a to
2cbc29a
Compare
2cbc29a to
e0f4d2b
Compare
|
@thompson-tomo is there an issue? We don't take new features unless there is an issue and the issue is triaged and accepted. |
reyang
left a comment
There was a problem hiding this comment.
Need an issue + follow the issue workflow https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change.
|
@cijothomas / @dashpole I have descoped this PR to only touch the sdk view config. This pr now bring across the definition from declarative config ie include & exclude of attributes. I have left the existing attribute. |
|
@reyang i was asked to raise a pr for this by a spec sponser @cijothomas as per https://cloud-native.slack.com/archives/C01N7PP1THC/p1773713324834269?thread_ts=1773667594.006789&channel=C01N7PP1THC&message_ts=1773713324.834269 hence I didn't think an issue was needed. As identified by the sponser this functionality is actually already defined in stable declarative config so is it actually a new feature? And at the same time the spec already mentioned attributes can be excluded. |
This is what I was using to determine for this PR:
|
|
Ok, I understand but in the case here it could just as easily be classified as a fix given that the spec itself mentions the ability to exclude attributes and it is in released stable declarative config. If you really want it could be classified as a corrective feature given it is fixing 2 things. Anyway the issue is #4960 |
Fixes #
Changes
Explicitly define method to exclude metric attributes which was already mentioned functionality in the attributes property description in the spec. This exclude functionality was already defined in the stable declarative config. The include is added to avoid changing the behaviour of the existing property.
Discussed on slack -> https://cloud-native.slack.com/archives/C01N7PP1THC/p1773667594006789
For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check