MetricProducers are passed via config to MetricReaders instead of RegisterProducer

[dashpole]

Fixes #3611

Changes

This change moves the registration of MetricProducers from a RegisterProducer function to a configuration option on the MetricReader.

The only potentially breaking impact of this change is that a user can no longer register new MetricProducers after the MetricReader is created. For example, it wouldn't be possible to dynamically register new MetricProducers. I don't know of any use-cases for this.

Explanation

Since it isn't feasible for JS to implement the spec as currently written, so we need to at least allow passing MetricProducers as config instead of via RegisterProducer.

However, it would be best if we had a consistent implantation across languages, and when I prototyped using configuration in Go, it simplified the implementation, and improved the user experience.

Given that both Go and JS would prefer to have them passed as configuration, rather than registration, and no other prototypes or implementations exist, I think it makes more sense to adapt the specification to require using configuration.

History

The RegisterProducer method was originally proposed and discussed here: #2722 (comment). I've spoken with the author after reviewing the prototype, and we both agree that configuration is actually a better way to pass MetricProducers.

[dashpole]

cc @aabmass @MrAlias

[reyang]

LGTM.

[jsuereth]

So the "RegisterProducer" is actually a mechanical thing for how Java passes the metric producer TO a metric reader.

Effectively, in Java we do something like:

meterProviderBuilder.register(myMetricReader);

This allows the SDK to hide the details of the internal MetricProducer mechanics. If we go with the option proposed here, you'll ALSO need to expose (from the SDK) access to the MetricProducer of a given MeterProvider. Something like:

new MyMetricReader(MetricReaderConfig.builder()
.addMetricProducer(meterProvider.getMetricProducer())
...

This has some problematic aspects, e.g. right now as MetricReader is registered to the MeterProducer, we can keep track of WHO owns which MetricProducer and isolate them.

I'd suggest we find a way to achieve the following two goals:

• Users can register a new MetricProducer to the SDK, so Readers can leverage more than one.
• Allow SDKs to continue hiding the internal details of their MetricProducer and continue to give them access to the MetricReader. E.g. Every MetricReader should be registered with the MeterProvider so that flush and shutdown can work across all of them.

As such, I'd suggest leaving RegisterProducer, but providing additional MetricProducer to the MeterProvider to join as needed when MetricReaders are registered.

[dashpole]

If we go with the option proposed here, you'll ALSO need to expose (from the SDK) access to the MetricProducer of a given MeterProvider.

I don't think this is necessarily true. Java (and Go) can continue to use the register functions to register the SDK as they do today. All this does is leave the register function unspecified (as it was before metric producers were in the spec), and require the ability to provide MetricProducers when constructing a MetricReader. You are more than welcome to implement the config option using register(). Here is my bad attempt at a partial prototype in java.

[dashpole]

@jsuereth Here is a more complete java prototype: open-telemetry/opentelemetry-java#5678

[jack-berg]

I don't think this is necessarily true. Java (and Go) can continue to use the register functions to register the SDK as they do today.

As long as we're under this shared understanding, I'm on board with this. It's probably worth it to keep the existing language on MetricReader#RegisterProducer method, but as a "MAY".

[dashpole]

As long as we're under this shared understanding, I'm on board with this. It's probably worth it to keep the existing language on MetricReader#RegisterProducer method, but as a "MAY".

I think it might need more changes to make sense. FWIW, I didn't implement registerProducer(Producer) in my java PoC (although I could have). The MetricProducers are passed via configuration: https://github.com/open-telemetry/opentelemetry-java/pull/5678/files#diff-afaf4d9b0605df68781d9b9befbaa9950b6f79c4c39a757a8efab8d18e82d6ecR74. The register() method is only used for the SDK, and wouldn't apply to MetricProducers as defined by the spec. So it would need to be something like MetricReader#Register(MeterProvider), rather than MetricReader#RegisterProducer(MetricProducer. Let me know if you still want to keep it.