Clarify configuring aggregations with MetricReaders

[legendecas]

https://github.com/open-telemetry/opentelemetry-specification/pull/2404 updated the requirements to construct a metric reader:

https://github.com/open-telemetry/opentelemetry-specification/blob/v1.12.0/specification/metrics/sdk.md#metricreader

To construct a MetricReader when setting up an SDK, the caller SHOULD provide at least the following: ...

• The default output aggregation (optional), a function of instrument kind. If not configured, the default aggregation SHOULD be used.
• The default output temporality (optional), a function of instrument kind. If not configured, the Cumulative temporality SHOULD be used.

It stated that a MetricReader can be constructed with a function to select aggregations for an instrument kind. However, the spec also says that aggregation is configured via the view APIs:

https://github.com/open-telemetry/opentelemetry-specification/blob/v1.12.0/specification/metrics/sdk.md#aggregation

An Aggregation, as configured via the View, informs the SDK on the ways and means to compute Aggregated Metrics from incoming Instrument Measurements.

AFAICT, aggregations configured with views are visible to all metric readers. There are two problems with configuring aggregation with MetricReaders:

1. It is not clear if the aggregation configured by a MetricReader should be visible to other metric readers, as it is said that MetricReader.collect should not cause side-effects to other metric readers, but not mentioning other MetricReader methods like the constructor.
2. How to handle conflicts since MetricReader can not change the name of the instrument like views?

The SDK MUST support multiple MetricReader instances to be registered on the same MeterProvider, and the MetricReader.Collect invocation on one MetricReader instance SHOULD NOT introduce side-effects to other MetricReader instances.

[legendecas]

@jsuereth hi, thank you for taking a look at this issue. But I'm not sure what the label "triaged-accepted" means -- as there aren't any resolutions settled yet for this issue.

[jsuereth]

It means this is an issue and should get worked on. We started a spec-triaging meeting Fridays to help get through the backlog of Specification issues. The result is either:

• Closing the bug as invalid/won't fix (triage-rejected)
• Asking for more information via (triage-needmoreinfo)
• Accepting the bug (triage-accepted) - the Owner/Reporter should work together to fix the bug.

Hope that helps!

[jsuereth]

It is not clear if the aggregation configured by a MetricReader should be visible to other metric readers, as it is said that MetricReader.collect should not cause side-effects to other metric readers, but not mentioning other MetricReader methods like the constructor.

It (generally) should not be visible. The naive implementation of this specification would lead to a per-reader "in memory storage" of metrics. If SDKs can optimise metric storage to "share" between readers, that's up to them. However, practically, with these changes I'd almost think we're (mostly) forced into per-metric-reader storage.

@reyang had a design that allowed sharing of partial-aggregated data between metric readers however I think, in practice, this is at odds with the real goal of #2404 - Minimizing the amount of memory consumed based on who is using the metric, and letting the "best" knowledgeable component make that decision (the exporter).

In more succinct terms:

• The default aggregation of a metric should be chosen by the exporter, as this will lead to optimal memory overhead.
• Views are a means to override default aggregation by users. They were designed before per-exporter aggregation, but they are still meant to be the user's last line of "no I know better" over defaults.
• I think the open question in the specification now is whether Views should be aware of exporter, given their intended role in the SDK.

How to handle conflicts since MetricReader can not change the name of the instrument like views?

@reyang can confirm, but I think the intention here was always this order:

• View configuration always wins
• Once they exist, API hints would be used at this level
• Next, Exporter defaults are used.
• Lastly, there's an overriding OTEL default

So to me that leaves two open AIs if you agree:

• [ ] Clarify in the specification the override order of aggregation (View, then Exporter, then Pure Default)
• [ ] Decide (or reject) whether or not views should have access to Exporter when determining aggregation (i.e. make it explicit they override Exporter config).

[jmacd]

See also https://github.com/open-telemetry/opentelemetry-specification/issues/2288

[jsuereth]

Discussed in Specificaiton SiG:

Action Items:

• A PR to the specification to make it clear override semantics of Views + Reader defaults.
• Defer discussion of question (2) "Decide whether or not views should have access to Reader/Exporter" to #2288

[jmacd]

I propose to close this issue in favor of https://github.com/open-telemetry/opentelemetry-specification/issues/2746 and https://github.com/open-telemetry/opentelemetry-specification/issues/2810.

[legendecas]

https://github.com/open-telemetry/opentelemetry-specification/issues/2643#issuecomment-1181900859 A PR to the specification to make it clear override semantics of Views + Reader defaults.

@jmacd I believe this action item is still applicable and distinct to this issue.