Metrics SDK: allow several metric readers to share the same metric exporter

[marcalff]

What are you trying to achieve?

For my application, I need to export metrics to an endpoint at different frequencies.

To do so, several PeriodicMetricReaders are defined, for example:

• Reader A exports data every 15 seconds
• Reader B exports data every 15 minutes

Because each reader owns an associated exporter, I now need to define:

• An exporter for reader A
• An exporter for reader B

even when these exporters have the same configuration and points to the same metric endpoint.

This leads to:

• duplicated exporter instances, which each have their own memory consumption
• duplicated network connections from my process to the endpoint
• duplicated SSL sessions from each exporter to the endpoint

which is a concern for:

• performances overhead in the instrumented application
• network session overhead in the endpoint, with multiple connections originating from the same place

What did you expect to see?

Instead, I would like to define, in the SDK:

• a unique exporter E
• configure reader A to use exporter E
• configure reader B to use exporter E

In other words, relax the requirement that a reader owns an exporter, and let the initialization code that sets up the SDK to decide (programmatically) how to deploy the pipelines, sharing (or not) exporters between readers. For example:

• reader A uses exporter E
• reader B uses exporter E
• reader C uses exporter F

Consequences:

For a reader flush operation, the flush can be forwarded to the shared exporter, which may, as a side effect, also flush data from other readers. This is acceptable in my understanding.

For a reader shutdown operation, the shutdown is forwarded to the shared exporter. The exporter is to implement a refcount, and only effectively shutdown the exporter when the last reader is shutdown, to avoid side effects between readers.

Additional context.

Add any other context about the problem here. If you followed an existing documentation, please share the link to it.

[austinlparker]

Can you confirm that this isn't working, and in which language(s)? This doesn't sound like how things are supposed to work (e.g., exporters aren't owned by readers since exporters are stateless)

[pellared]

This doesn't sound like how things are supposed to work (e.g., exporters aren't owned by readers since exporters are stateless)

This is how it is supposed to work according to the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md

Metric Exporters always have an associated MetricReader. [...] There could be multiple Push Metric Exporters or Pull Metric Exporters or even a mixture of both configured at the same time on a given MeterProvider using one MetricReader for each exporter.

One, not many.

Exporters can be stateful:

• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#forceflush-2
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#shutdown-2

Side note: I think there is a similar problem when one would like to use the same exporter with different span/log processors.

[trask]

In the Java SDK you can create multiple metric readers from the same metric exporter.

[marcalff]

I confirm this is not working for C++, and forces the application to duplicate exporters.

So, what is the outcome for this issue then ?

According to the spec, there is one reader for each exporter, not many.
The java implementation is not compliant then.
The C++ implementation follows the spec, which makes it difficult to use in my application.

I am asking to relax the spec to allow N readers using the same exporter, so that:

• opentelemetry-java becomes compliant
• opentelemetry-cpp can implement the change

[pellared]

For Go SDK it would be a problem only for stateful exporters. I think this change will be difficult to implement to have it out if the box. The problem is with synchronizing the shutdown of the stateful exporters. I think that in such scenario the exporter shutdown must be delayed until all readers are shut down.

Couldn't the described problem be solved by implementing "an exporter provider" which returns a new instance of exporter for each reader while under the hood there would be a reference counter and (a pool of) resources that are shared across all returned exporters? This would not require any chances in the SDKs and specification.

The other possiblilty is that exporters provided by SDK use shared resources.

[marcalff]

For Go SDK it would be a problem only for stateful exporters. I think this change will be difficult to implement to have it out if the box. The problem is with synchronizing the shutdown of the stateful exporters. I think that in such scenario the exporter shutdown must be delayed until all readers are shut down.

Exactly, this is my suggestion to implement a refcount in the exporter, to effectively delay the shutdown when all readers are shutdown.

Couldn't the described problem be solved by implementing "an exporter provider" which returns a new instance of exporter for each reader while under the hood there would be a reference counter and (a pool of) resources that are shared across all returned exporters? This would not require any chances in the SDKs and specification.

I fail to see the logic here:

• the spec is broken (or too restrictive for no apparent reason),
• some implementations (java) are not compliant already,
• the suggestion is to have more non compliant implementations,

and all this to avoid changing the broken spec ?

[pellared]

Exactly, this is my suggestion to implement a refcount in the exporter, to effectively delay the shutdown when all readers are shutdown

Exporter is an interface. This would be still just an implementation detail of an exporter? Which one? Or the provider? How would we solve sharing an exporter across multiple providers? How would an exporter know that it is applied to a reader?

all this to avoid changing the broken spec

Nope. Mentioning to evaluate multiple options.

[marcalff]

Blocks implementation of:

• [Performance] Avoid duplication of metric exporters. opentelemetry-cpp#2710

[svrnm]

@open-telemetry/technical-committee please take a look

[jack-berg]

Talked about in the 7/31/24 TC meeting..

• Metric exporters can't be associated with multiple readers because the spec doesn't require export to be thread safe. The motivation for this seems to be to promote increased simplicity of implementations.
• This use case seems important, and is already possible in several languages.
• We think that this is reasonable to pursue, and that someone should drive evolving the spec to accommodate this in a backwards compatible way. Updating the status to traige:accepted:needs-sponsor to reflect this.

[marcalff]

This use case seems important, and is already possible in several languages.

@jack-berg

I think the long lasting consequences of this should not be underestimated, which is why the spec should be relaxed now.

So, "several languages", including Java, allows a configuration which is not allowed (in fact, explicitly forbidden) per the spec wording.

Meanwhile, the opentelemetry-configuration project defines a yaml schema to represent what a configuration is, based on the current spec.

The schema does not allow to share a metric exporter between several metrics readers, because the exporter configuration is included -- by value -- as a child of the metric reader configuration.

If we do nothing now, in 3 years from now, we will find out that some people using a shared exporter, by using the programatic API for the SDK configuration (say, in Java), will be unable to adopt and migrate to the yaml configuration provided by opentelemetry-configuration, because the yaml schema is too restrictive and does not allow to represent configurations that already exists today for some SIG.

In 3 years, when the yaml implementation of opentelemetry-configuration is already implemented in each SIG, and already used in the wild, it will be too late to change the schema to accommodate this use case.

[jack-berg]

The schema does not allow to share a metric exporter between several metrics readers, because the exporter configuration is included -- by value -- as a child of the metric reader configuration.

Are you suggesting adjusting the schema so that exporter is configured separately from reader, and associating the exporter with the reader by some sort of reference? (This is analogous to how the collector configures receivers, processors, and exporters, and separately composes them into pipelines.) If so, this was previously considered and rejected in early discussions (see MrAlias/otel-schema#12, MrAlias/otel-schema#10). The configuration schema is still experimental and we could of course revisit that decision, but that's a pretty disruptive shift and would need strong arguments.

The set of people that would stand to be impacted by this are the intersection of people who want to have multiple periodic readers at different collection intervals AND have applications which are performance sensitive enough that the overhead of not sharing exporter resources is a non-starter. I think that may occur, but won't be common.

Putting that aside, I do think we should find a way to adjust the spec to support this since we couldn't think of any good reasons to justify this restriction.

[marcalff]

Are you suggesting adjusting the schema so that exporter is configured separately from reader, and associating the exporter with the reader by some sort of reference?

So that the exporter may be configured separately, yes.

(This is analogous to how the collector configures receivers, processors, and exporters, and separately composes them into pipelines.)

No, it can be done differently.

A schema change does not have to be disruptive, this is an assumption on the solution.