feat(sdk-metrics)!: remove MeterProvider.addMetricReader() in favor of constructor option

[pichlermarc]

Which problem is this PR solving?

Currently a MetricReader is added to a MeterProvider via addMetricReader(). As Meters (and through it instruments) can be created before adding a MetricReader. This gives the false impression that the order of calls does not matter. This PR removes the addMetricReader() method in favor of a constructor option as it is the case for most SDKs:

• see Java
  • does not allow adding a MetricReader after building the MeterProvider
• see Python
  • does not allow adding a MetricReader after building the MeterProvider
• see C++
  • the comment there seems to indicate that they are in a similar situation as we are (only meter data created after adding the MetricReader is respected.
• see .NET [1] [2]
  • similar to Java, they have a builder that allows it, but adding it after building the MeterProvider is not possible

An alternative would be to allow adding a MetricReader like in the current interface, but re-configuring all instruments, their aggregations, assocaited views and their temporality. This however, is not required by the spec and would drastically increase complexity of an already very complex Metrics SDK.

Fixes #4112
References #open-telemetry/opentelemetry-js-contrib#1900

Short description of the changes

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

• Adapted existing tests

[codecov]

Codecov Report

Merging #4419 (0be7c90) into next (543f0b4) will increase coverage by 0.03%.
Report is 8 commits behind head on next.
The diff coverage is 96.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4419      +/-   ##
==========================================
+ Coverage   92.24%   92.27%   +0.03%     
==========================================
  Files         332      332              
  Lines        9437     9436       -1     
  Branches     1999     1996       -3     
==========================================
+ Hits         8705     8707       +2     
+ Misses        732      729       -3     

Files	Coverage Δ
...erimental/packages/api-logs/src/types/LogRecord.ts	100.00% <ø> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts	93.75% <100.00%> (+0.20%)	⬆️
...-otlp-http/src/platform/node/OTLPMetricExporter.ts	100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts	100.00% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts	94.75% <100.00%> (+0.83%)	⬆️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	92.80% <100.00%> (+0.05%)	⬆️
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts	95.23% <100.00%> (ø)
...mental/packages/otlp-transformer/src/logs/index.ts	100.00% <ø> (ø)
experimental/packages/sdk-logs/src/LogRecord.ts	98.05% <100.00%> (+0.07%)	⬆️
...ckages/opentelemetry-core/src/common/attributes.ts	93.18% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

[legendecas]

Can we deprecate MeterProvider.addMetricReader instead while adding the new constructor option? In this way, we can avoid breaking all users of the sdk-metrics and allow them to migrate to the new option.

[pichlermarc]

Can we deprecate MeterProvider.addMetricReader instead while adding the new constructor option? In this way, we can avoid breaking all users of the sdk-metrics and allow them to migrate to the new option.

Yes. This PR is targeting the next branch so it's already intended for 2.0 (that's why it's breaking).
Once this is merged, I'll open a PR to backport the constructor option and tests to main and deprecate MeterProvider.addMetricReader there 🙂

[pichlermarc]

@legendecas, I opened another PR that targets main just now: #4427

[legendecas]

Oh, I see. Thank you for clarifying. I didn't notice the target branch 😅