Clarify configuring aggregations with MetricExporter

[legendecas]

What are you trying to achieve?

The spec currently defines that:

The aggregation and temporality properties used by the OpenTelemetry Metric SDK are determined when registering Metric Exporters through their associated MetricReader. OpenTelemetry language implementations MAY support automatically configuring the MetricReader to use for an Exporter.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter

Currently, SDKs take their own interpretation on this:

MetricExporters have no involvement in determining the aggregation:

• Python: https://github.com/open-telemetry/opentelemetry-python/blob/776b150660136f894f0c93033218a2c023754ef6/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py#L68
• DotNet: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs#L48

MetricExporters have a method to select aggregation for instruments:

• Java: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExporter.java#L37
• Go: https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/metric/export/metric.go#L149

This is a bit ambiguous on what MetricExporter's role is in configuring the aggregation with MetricReader. Should MetricReader ask a MetricExporter for the aggregation if MetricReader's optional aggregation selector function is absent?

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.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader

From reading the spec, default aggregation should be used when the optional aggregation selector is not configured for the MetricReader. There are no words mentioned if a MetricExporter should be involved in the aggregation selection.

Additional context.

We have received complaints about too many "unrelated" methods that need to be implemented on a MetricExporter, like a temporality selector, as it should already be configured with MetricReaders, it can be burdensome for vendors to duplicate the works. (see open-telemetry/opentelemetry-js#3060 (comment)).

@jsuereth mentioned an ordering of selecting an aggregation at #2643 (comment) that gives another interpretation of the spec. Maybe @jsuereth can also chime in here? Also, I'd believe @jmacd has an opinion on this.

Refs: open-telemetry/opentelemetry-js#3153 (comment)

[dyladan]

it can be burdensome for vendors to duplicate the works

IMO it is better to burden vendors with duplicating this method than to burden users with having to understand which temporalities their vendor supports for each instrument type and how to configure them

[legendecas]

@rbailey7210 would you mind expanding on what information I can help to elaborate to triage this issue? thank you <3

[jack-berg]

We have received complaints about too many "unrelated" methods that need to be implemented on a MetricExporter, like a temporality selector, as it should already be configured with MetricReaders, it can be burdensome for vendors to duplicate the works.

Agree with @dyladan. Temporality selection is a pretty essential decision for exporters / backends, and not a question that an exporter should side step.

In java, while these methods are implementable by MetricReaders, PeriodicMetricReader (i.e. the main MetricReader implementation) simply delegates to its configured MetricExporter, so they're really only configured in one place.

[tigrannajaryan]

Can one of @open-telemetry/specs-metrics-approvers take the assignment of this? I don't know much about this topic.

[MrAlias]

FWIW, the Go SDK has switched its approach currently. It configures the temporality and aggreation default selectors with options passed to readers on their creation:

https://github.com/open-telemetry/opentelemetry-go/blob/510910e92d5cc8d18c38e4cc88f1bc7c48f6465f/sdk/metric/reader.go#L128-L133

https://github.com/open-telemetry/opentelemetry-go/blob/510910e92d5cc8d18c38e4cc88f1bc7c48f6465f/sdk/metric/reader.go#L176-L197

Though, there is a proposal to change back to having exporters implement methods that set this configuration value: open-telemetry/opentelemetry-go#3260

[MrAlias]

It was suggested here that this should be considered a language implementation choice.

I think the way the specification is currently defined it is ambiguous enough that both interpretations are currently allowed. Therefore, it looks like it could technically be considered this.

However, the question that I think needs to be resolved is if different languages implement this differently, will OpenTelemetry users be impacted? Will they not be able to achieve the same functionality in one language implementation as the other?

It seems to me that if a user of one language implementation uses an exporter that has a preference for temporality/aggregations that differ from the defaults and are able to setup their SDK by just registering it with a reader and they move to another language implementation that requires them to know they also need to configure different temporality/aggregations with a reader that they register a similar exporter with they are going to be frustrated. Both by the fact that things are not working, but also by the fact that OTel is different enough that when they do effectively the same thing in one language as they did in another things don't work.

Based on this, I think this falls under the purpose of the specification. It needs to be defined so OpenTelemetry provides users with a consistent and hassle free usage.

Furthermore, when a configuration file format is defined, how will it be defined regarding default aggregators/temporality? If all implementations define this with an exporter it would be different than if all implementations define this with a reader. If there is a mismatch, configuration file formats would not be compatible language-to-language.

[MrAlias]

Looking at the fact that multiple language implementations have made stable releases (.NET, Java, Python) and they are not all consistent with regard to this, it is likely not possible to uniformly update the specification with one normative statement of how these parameters should be set.

That or we will need to update the specification and have non-conforming implementations deprecate their old approach.

[tsloughter]

When .NET, Java and Python went GA was there a similar process as when languages went GA with tracing?

---

That is to say, if they were reviewed by separate people who signed off on different interpretations it is a pretty clear sign that the spec is not clear :)

[jmacd]

I've called this question a debate about an implementation detail.

@MrAlias wrote:

I think the way the specification is currently defined it is ambiguous enough that both interpretations are currently allowed. Therefore, it looks like it could technically be considered this.

@tsloughter wrote:

That is to say, if they were reviewed by separate people who signed off on different interpretations it is a pretty clear sign that the spec is not clear :)

Although it is not clear, when I wrote the specification I meant to allow both interpretations. It was intentional ambiguity, at least. I do recognize there is a problem, given this debate, but I'm not sure whether the better solution is to (a) clearly state that both approaches are OK, (b) try to somehow remove one of the options, or (c) see below.

@MrAlias makes two suggestions:

1. User could be frustrated if in one SDK they "register configured exporter with a reader" and in another they "register exporter with a configured reader".
2. Configuration will have different structures in different SDKs?

Although I'm starting to sympathize with the first of these points, it makes me wonder whether users of the DotNet and Python SDKs have actually experienced this frustration. Are they frustrated that it is difficult to pair the exporter and its reader settings?

For the second of these points, I would not expect the configuration structure to exactly match SDK setup in any language. I'm familiar with the OTel Collector's configuration mechanism and how its factories produce exporters using an exporterhelper package that implements a swath of standard behavior. The configuration of the standard things implemented by the exporterhelper is at a lower level in the configuration, and the factory pattern inverts the relationship. The factory parses the common exporter configuration from alongside the specific exporter configuration, configures an exporterhelper with its configuration with an exporter with its configuration.

This pattern suggests a user-friendly solution to the intentional ambiguity described above. Let each exporter package provide a factory for its class of exporters that, given a list of common Reader options will return the necessary MetricReader, properly configured. Exporters would be expected to configure their intended temporality and default aggregations via their Factory; the actual Exporter would not be required to support an interface to report preferred temporality like the one Java does.