Clarify SDK behavior for Instrument Advisory Parameter #4389
Conversation
specification/metrics/sdk.md
Outdated
| 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.
Co-authored-by: Trask Stalnaker <[email protected]>
| 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,
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).
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.
| exist. | ||
| exist. If any configuration is provided via View and [Instrument advisory | ||
| parameters](#instrument-advisory-parameters), then the View configuration | ||
| MUST take precedence. |
There was a problem hiding this comment.
Suppose the view sets the aggregation to be explicit bucket histogram and the advice says that the explicit bucket boundaries should be [1, 5, 10].
Does this language suggest that the output metric should be: 1. explicit bucket histogram with [1, 5, 10] or 2. explicit bucket histogram with default boundaries?
I believe it should be option 2, since if option 1 was defined it would mean that there is some sort of merging of the buckets from advisory parameters with the aggregation from the view.
There was a problem hiding this comment.
+1, I think it should be option 2.
There was a problem hiding this comment.
Yes 2 is the expected behavior. There is no "merge" advice+view behavior today. And this PR is clarifying/documenting existing behavior only.
Do either of you suggest we should explicitly mention this?
Also Fixes #4317
Changes
Mostly editorial, to specify SDK behavior for Advisory Parameters, explicitly calling out that SDK MUST give precedence to View config over InstrumentAdvisory. I believe this is already the intention, but was less specified. Though some wordings exist in the View spec about attribute-keys, no mention about Histogram buckets..
CHANGELOG.mdfile updated for non-trivial changesspec-compliance-matrix.mdupdated if necessary