feat(sdk-metrics): implement MetricProducer specification

[aabmass]

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #4006

Short description of the changes

Allows adding additional MetricProducers to a MetricReader which can be used to produce additional data beyond what the SDK produces

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added new tests cases

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[aabmass]

Note the specification asks for a RegisterMetricProducer function https://github.com/open-telemetry/opentelemetry-specification/blob/v1.23.0/specification/metrics/sdk.md#registerproducermetricproducer

However, I since we already have a public setMetricProducer which assumes it is receiving the SDK's MetricCollector, this seems like the cleanest solution without breaking anything.

I will also check with the spec on this.

[aabmass]

Opened open-telemetry/opentelemetry-specification#3611

[pichlermarc]

I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂

[aabmass]

open-telemetry/opentelemetry-specification#3613 has a few approvals now

[codecov]

Codecov Report

Merging #4007 (694b411) into main (48fb158) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 694b411 differs from pull request most recent head f095627. Consider uploading reports for the commit f095627 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4007   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         321      320    -1     
  Lines        9264     9251   -13     
  Branches     1968     1963    -5     
=======================================
- Hits         8557     8545   -12     
+ Misses        707      706    -1     

Files Changed	Coverage Δ
packages/sdk-metrics/src/export/MetricReader.ts	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[legendecas]

I think we should add multiple resources support to the metric reader and metric exporter instead of dropping resources silently -- the registered metric producer can be different meter providers that have different resources.

[aabmass]

I'm not sure how we could do that without making a breaking change. Also see this part of the spec:

If the MeterProvider is an instance of MetricProducer, this MAY be used to register the MeterProvider, but MUST NOT allow multiple MeterProviders to be registered with the same MetricReader.

That's not exactly the case in the JS implementation, but I think the spirit of it is to avoid this happening. I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

[aabmass]

Fyi open-telemetry/opentelemetry-specification#3636

[pichlermarc]

I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

I like this idea. However, I wonder if we could do this without breaking the MeterProvider as it also implements the MetricProducer interface. 🤔

It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers config - I think in that case it would be okay to change the behavior later.

[aabmass]

Is this OK with you for now @legendecas?

[pichlermarc]

Thank you for working on this. 🙂

A few questions came up while I was reviewing:

is it always expected that people use MetricProducer with an SDK (MeterProvider), or also without one? 🤔

In that case, the PeriodicExportingMetricReader would only start once setMetricProducer() is called manually, right? Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

Would it make sense to have similar ergonomics to a MeterProvider to handle this registration? 🤔

[pichlermarc]

I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

I like this idea. However, I wonder if we could do this without breaking the MeterProvider as it also implements the MetricProducer interface. 🤔

It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers config - I think in that case it would be okay to change the behavior later.

[pichlermarc]

I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂

[legendecas]

Thank you for working on this!

[aabmass]

Thanks for the review everyone. I noticed you left a few questions I forgot to answer @pichlermarc

Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

I don't think we should allow this, IMO passing the MetricProducers when creating the MeterProvider is cleaner for the callers. Actually I think I should add a doc comment to setMetricProducer() like "this should only be called internally by the SDK". Wdyt?

[pichlermarc]

Thanks for the review everyone. I noticed you left a few questions I forgot to answer @pichlermarc

Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

I don't think we should allow this, IMO passing the MetricProducers when creating the MeterProvider is cleaner for the callers.

Hmm, the reason I was asking is mostly regarding a possible stand-alone use of a MetricProducer: is this something that is intended? If we decide against using the setMetricProducer() we most likely need some other mechanism to start the PeriodicExportingMetricReader. Right now on the PeriodicExportingMetricReader onInitialized() is only called when the MeterProvider is set via setMetricProducer(). 🤔

Actually I think I should add a doc comment to setMetricProducer() like "this should only be called internally by the SDK". Wdyt?

I think this is a good idea. 👍 I think there's also a jsdoc annotation @internal (or similar) that could be used for that. 🤔

[aabmass]

I think this is a good idea. 👍 I think there's also a jsdoc annotation @internal (or similar) that could be used for that. 🤔

Pushed f095627 with this change.

[pichlermarc]

Hmm, the reason I was asking is mostly regarding a possible stand-alone use of a MetricProducer: is this something that is intended? If we decide against using the setMetricProducer() we most likely need some other mechanism to start the PeriodicExportingMetricReader. Right now on the PeriodicExportingMetricReader onInitialized() is only called when the MeterProvider is set via setMetricProducer(). 🤔

Summary from the SIG: It's like this on purpose to discourage circumventing the SDK. Users shouldn't use the MetricProducer without a MeterProvider, therefore changes to the PeriodicExportingMetricReader are not necessary. 🙂