Clarify SDK behavior for Instrument Advisory Parameter #4389
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
@@ -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. | ||
| [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: | ||
|
|
@@ -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. 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) | ||
|
|
@@ -945,7 +950,7 @@ Meter MUST treat it the same as an empty description string. | |
|
|
||
| ### Instrument advisory parameters | ||
|
|
||
| **Status**: [Stable](../document-status.md), except where otherwise specified | ||
|
|
||
| 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 | ||
|
|
@@ -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` | ||
|
There was a problem hiding this comment. 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, 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). There was a problem hiding this comment. 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. | ||
|
There was a problem hiding this comment. 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) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could you please update PR title and/or add a changelog entry that explicit bucket boundaries are now stable in the SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was stable already, but it was missed to update this section when it got stabilized. That's why I put this as merely editorial change...
(PR desc says this fixes #4317 to cover this..)
Let me know if this is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Probably #4317 should be addressed before - to make clear that either the SDK portion is stable or not yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosalberto Can you clarify more?
SDK part is stable, just missed to update the status in the spec (that is my understanding, happy to correct if that was not the case.) This PR is fixing that.