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.

Sign up for GitHub

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

Clarify SDK behavior for Instrument Advisory Parameter #4389

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Clarify SDK behavior for Instrument Advisory Parameter #4389

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69b7573
Clarify SDK behavior for Instrument Advisory Parameter
cijothomas Jan 28, 2025
ee844ac
fix TOC
cijothomas Jan 28, 2025
f243877
Update specification/metrics/sdk.md
cijothomas Jan 28, 2025
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
48 changes: 43 additions & 5 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ linkTitle: SDK
* [Instrument unit](#instrument-unit)
* [Instrument description](#instrument-description)
* [Instrument advisory parameters](#instrument-advisory-parameters)
+ [Instrument advisory parameter: `ExplicitBucketBoundaries`](#instrument-advisory-parameter-explicitbucketboundaries)
+ [Instrument advisory parameter: `Attributes`](#instrument-advisory-parameter-attributes)
* [Instrument enabled](#instrument-enabled)
- [Attribute limits](#attribute-limits)
- [Exemplar](#exemplar)
Expand Down Expand Up @@ -418,9 +420,10 @@ made with an Instrument:

* Determine the `MeterProvider` which "owns" the Instrument.
* If the `MeterProvider` has no `View` registered, take the Instrument
and apply the default Aggregation on the basis of instrument kind
according to the [MetricReader](#metricreader) instance's
`aggregation` property.
and apply the default Aggregation on the basis of instrument kind according
to the [MetricReader](#metricreader) instance's `aggregation` property.
[Instrument advisory parameters](#instrument-advisory-parameters), if any,
MUST be honored.
* If the `MeterProvider` has one or more `View`(s) registered:
* If the Instrument could match the instrument selection criteria, for each
View:
Expand All @@ -436,7 +439,9 @@ made with an Instrument:
View sets an asynchronous instrument to use the [Explicit bucket
histogram aggregation](#explicit-bucket-histogram-aggregation)) the
implementation SHOULD emit a warning and proceed as if the View did not
exist.
exist. If any configuration is provided via View and [Instrument advisory
parameters](#instrument-advisory-parameters), then the View configuration
MUST take precedence.
* If the Instrument could not match with any of the registered `View`(s), the
SDK SHOULD enable the instrument using the default aggregation and temporality.
Users can configure match-all Views using [Drop aggregation](#drop-aggregation)
Expand Down Expand Up @@ -945,7 +950,7 @@ Meter MUST treat it the same as an empty description string.

### Instrument advisory parameters

**Status**: [Development](../document-status.md)
**Status**: [Stable](../document-status.md)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

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
Expand All @@ -956,6 +961,39 @@ 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](#duplicate-instrument-registration).

If a [View](#view) is configured with the same settings as advisory parameters,
the View MUST take precedence over the advisory parameters.

#### Instrument advisory parameter: `ExplicitBucketBoundaries`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally haven't explicitly documented the API methods or parameters in the SDK specification. It is already stated that the SDK is an implementation of the API. I think the general statement above "If a View is configured with the same settings as advisory parameters,
the View MUST take precedence over the advisory parameters." is already enough to clarify the precedence ordering.

I don't think this section, or the one below, is necessary unless is is clarifying something additional (other than precedence or what is already in the API).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-advisory-parameters The API doc says "OpenTelemetry SDKs MUST handle advisory parameters as described here", and that here is what this section.

I am okay with just keeping the general statement about view takes precedency alone and remove rest. Will see if there are other feedback on this. I have seen recently that we try to be more explicit in SDK specs (eg: Logger.Enabled), but does not necessarily have to do it for advisory.


**Status**: [Stable](../document-status.md)

This parameter is applicable when the [Explicit Bucket
Histogram](#explicit-bucket-histogram-aggregation) aggregation is utilized.

`ExplicitBucketBoundaries` (`double[]`) specifies the recommended set of bucket
boundaries to use during the [Explicit Bucket
Histogram](#explicit-bucket-histogram-aggregation) aggregation. If the user has
configured a View with specific bucket boundaries, those boundaries should take
precedence. In the absence of such a configuration, the advisory parameter
should be used. If neither is provided, the default bucket boundaries must be
applied.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the last sentence may be too obvious that it may be omitted...But I am okay with being more explicit.


#### Instrument advisory parameter: `Attributes`

**Status**: [Development](../document-status.md)

This parameter is applicable to all aggregations.

`Attributes` (a list of [attribute keys](../common/README.md#attribute))
specifies the recommended set of attribute keys for measurements aggregated to
produce a metric stream.

If the user has provided attribute keys via a View, those keys should take
precedence. If no View is configured, the advisory parameter configured on the
instrument should be used. If the `Attributes` advisory parameter is also
absent, all attributes must be retained.

### Instrument enabled

**Status**: [Development](../document-status.md)
Expand Down